-
Notifications
You must be signed in to change notification settings - Fork 64
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
Update CI and docs to remove references to default admin creds #365
Conversation
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
@dhrubo-os @greaa-aws can you folks take a look here and provide any feedback? This change will be happening for 2.12.0. Thanks! |
Signed-off-by: Derek Ho <dxho@amazon.com>
@@ -1294,7 +1294,7 @@ | |||
"outputs": [ | |||
{ | |||
"data": { |
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.
not sure what happened here, looks like new line diff?
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #365 +/- ##
=======================================
Coverage 91.53% 91.53%
=======================================
Files 42 42
Lines 4395 4395
=======================================
Hits 4023 4023
Misses 372 372 ☔ View full report in Codecov by Sentry. |
This change should have failed multiple tests as test client still has "admin" as password. opensearch-py-ml/tests/__init__.py Line 38 in 5c93154
I haven't looked into it properly. But, will take a look and respond. EDIT: Just realised this will affect from versions >= 2.12.0. |
@dhrubo-os @greaa-aws @ylwu-amzn Can we get some reviews on this PR? |
Could you please resolve the merge conflict? |
Signed-off-by: Derek Ho <derek01778@gmail.com>
Signed-off-by: Derek Ho <dxho@amazon.com>
Done |
Merging this PR will require a issue in backlog so that we take care of updating passwords in the testcases and in other required places when we migrate to Opensearch>=2.12.0 Edit: Seems like there's already one created - #371 |
@rawwar Thanks for the call out. Could you please create an issue for py-ml too? Thanks |
@greaa-aws @ylwu-amzn @b4sjoo @jngz-es @rbhavna Could we get 1 more review on this? Is there anything blocking from getting this merged? We are targeting 2.12 version release for this change. |
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.
LGTM
Description
Starting in 2.12.0 release, security plugin is mandating an initial admin password to be set via env variable. This PR updates the docs and CI to accommodate this change.
Issues Resolved
[List any issues this PR will resolve]
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.