-
Notifications
You must be signed in to change notification settings - Fork 285
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
Support security demo script changes #4304
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #4304 +/- ##
=======================================
Coverage 91.26% 91.27%
=======================================
Files 189 189
Lines 6124 6140 +16
=======================================
+ Hits 5589 5604 +15
- Misses 535 536 +1 ☔ View full report in Codecov by Sentry. |
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.
Can you please make sure piplock is not updating all other versions as well? And only the necessary package is installed?
I only added the new version, I am not sure how I can stop pipenv to not update other dependencies. I believe we have appropriate version restrictions on each requirements and pipenv honors that while updating. Should be safe. |
Sometimes that break too. Mostly because of the underlying docker images during runs being different depending on the platform. We can merge this as if and monitor for any failures. |
def test_execute_security_enabled(self) -> None: | ||
benchmark_test_suite = BenchmarkTestSuite(endpoint=self.endpoint, security=True, args=self.args) | ||
benchmark_test_suite = BenchmarkTestSuite(endpoint=self.endpoint, security=True, distribution_version='2.10.0', args=self.args) |
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.
Can we try with 2.3.0
? 😅
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.
:) Done!
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 like code cov is complaining about need_strong_password
. Maybe just add a small assert for that? Looks good overall. Need to get similar change in for cluster cdk as well.
Signed-off-by: Rishabh Singh <[email protected]>
Yeah, it can nit-picky. The condition is actually tested in tests/tests_test_workflow/test_benchmark_workflow/benchmark_test/test_benchmark_test_suite.py where I am asserting a string password is present in the docker command. |
Description
There were recent changes to security demo configuration setup which now requires a custom admin Password to be setup for OpenSearch version 2.12.0 and above.
This PR adds appropriate code changes to support the nightly benchmark runs.
Issues Resolved
opensearch-project/security#3624
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.