Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Extracted the user attr handling methods from ConfigModelV7 into its own class #4416

Merged

Conversation

nibix
Copy link
Collaborator

@nibix nibix commented Jun 8, 2024

Description

This code change is just in preparation for the change in #4380 as requested in #4380 (comment) .

The code for user attribute handling is moved from the class ConfigModelV7 into its own class, as other new code will need to use it. Additionally, it enhances the structuring of the code by moving code with a specific purpose into its own class.

Personal note: A thorough re-design of this code might be worthwhile.

  • Category: Refactoring
  • Why these changes are required? New code introduced in Optimized Privilege Evaluation #4380 needs to use this code.
  • What is the old behavior before changes and new behavior after changes? No behavioral changes.

Issues Resolved

Is this a backport? If so, please add backport PR # and/or commits #

Testing

[Please provide details of testing done: unit testing, integration testing and manual testing]

Check List

  • New functionality includes testing - no new functionality
  • New functionality has been documented - no new functionality
  • Commits are signed per the DCO using --signoff

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.
For more information on following Developer Certificate of Origin and signing off your commits, please check here.

…own class

Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
Copy link

codecov bot commented Jun 8, 2024

Codecov Report

Attention: Patch coverage is 78.57143% with 6 lines in your changes missing coverage. Please review.

Project coverage is 65.45%. Comparing base (8688d6b) to head (4695280).

Additional details and impacted files

Impacted file tree graph

@@            Coverage Diff             @@
##             main    #4416      +/-   ##
==========================================
+ Coverage   65.42%   65.45%   +0.02%     
==========================================
  Files         310      311       +1     
  Lines       22013    22014       +1     
  Branches     3556     3556              
==========================================
+ Hits        14402    14409       +7     
+ Misses       5841     5836       -5     
+ Partials     1770     1769       -1     
Files Coverage Δ
...pensearch/security/securityconf/ConfigModelV7.java 67.92% <100.00%> (-0.47%) ⬇️
...opensearch/security/privileges/UserAttributes.java 76.00% <76.00%> (ø)

... and 5 files with indirect coverage changes

Copy link
Contributor

@shikharj05 shikharj05 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I also like the idea of breaking #4380 down into such PRs to make the review faster. Feel free to raise such incremental PRs :)

Copy link
Member

@peternied peternied left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this is a refactor, let's just move in place.

@shikharj05 - want to make a follow up PR for some of those items you called out.

@peternied peternied merged commit 00c31e9 into opensearch-project:main Jun 10, 2024
82 checks passed
@peternied peternied added the backport 2.x backport to 2.x branch label Jun 10, 2024
opensearch-trigger-bot bot pushed a commit that referenced this pull request Jun 10, 2024
…own class (#4416)

Signed-off-by: Nils Bandener <nils.bandener@eliatra.com>
(cherry picked from commit 00c31e9)
Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 2.x backport to 2.x branch
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants