-
Notifications
You must be signed in to change notification settings - Fork 275
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
Fix Bug with Install demo configuration running in cluster mode with -y #3935
Conversation
…assword Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
Related issue: #2402 |
@derek-ho @DarshitChanpura What should the behavior of It looks like the recent switch to implementing the demo configuration installer to Java fixed the bug, but it then introduced issues in the distribution build. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Approving as this is inline with previous behavior.
I added another test to verify this behavior. |
Signed-off-by: Derek Ho <[email protected]>
src/test/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurerTests.java
Outdated
Show resolved
Hide resolved
src/test/java/org/opensearch/security/tools/democonfig/SecuritySettingsConfigurerTests.java
Outdated
Show resolved
Hide resolved
Signed-off-by: Derek Ho <[email protected]>
Signed-off-by: Derek Ho <[email protected]>
This what I assumed to when writing the Java code. But turns out that the original script did nothing to
Agreed that -y should inherently mark other options two options as yes, but since it is breaking builds rn, I'm happy to revert that bug fix and address it later. After this change is merged, -y will only be reduced to choosing yes for installing demo certificates and the other two prompts will be assumed as no. An ideal usage would be to either use -y ( which automatically assumes yes for demo certs, cluster_mode and init_security) OR pass individual options : -d for demo certs (new option), -c for cluster mode and -i for init security modules. where -y will always take priority. But maybe that is for another release. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good. This existing bug which was addressed in original PR and is being reverted here, should be addressed in future.
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #3935 +/- ##
=======================================
Coverage 65.16% 65.17%
=======================================
Files 298 298
Lines 21218 21216 -2
Branches 3457 3457
=======================================
Hits 13827 13827
+ Misses 5676 5675 -1
+ Partials 1715 1714 -1
|
…-y (#3935) Previous behavior prior to 2.11 would be that init security and cluster mode would only be set if they were explicitly passed in. There is a small bug that is setting them to be true if -y is passed in. This is causing a few failures with duplicate keys in integration tests when -y is passed in. https://github.com/opensearch-project/security/blob/2.11/tools/install_demo_configuration.sh#L73-L98 --------- Signed-off-by: Derek Ho <[email protected]> (cherry picked from commit f217fa8) Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…ster mode with -y (#3936) Backport f217fa8 from #3935. Signed-off-by: Derek Ho <[email protected]> Signed-off-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com> Co-authored-by: github-actions[bot] <github-actions[bot]@users.noreply.github.com>
…-y (opensearch-project#3935) Previous behavior prior to 2.11 would be that init security and cluster mode would only be set if they were explicitly passed in. There is a small bug that is setting them to be true if -y is passed in. This is causing a few failures with duplicate keys in integration tests when -y is passed in. https://github.com/opensearch-project/security/blob/2.11/tools/install_demo_configuration.sh#L73-L98 --------- Signed-off-by: Derek Ho <[email protected]>
Description
Previous behavior prior to 2.11 would be that init security and cluster mode would only be set if they were explicitly passed in. There is a small bug that is setting them to be true if -y is passed in. This is causing a few failures with duplicate keys in integration tests when -y is passed in.
https://github.com/opensearch-project/security/blob/2.11/tools/install_demo_configuration.sh#L73-L98
Issues Resolved
[List any issues this PR will resolve]
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
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.