Closed derek-ho closed 5 months ago
All modified and coverable lines are covered by tests :white_check_mark:
Project coverage is 72.14%. Comparing base (
ba715b9
) to head (9d2c999
). Report is 26 commits behind head on main.:exclamation: Current head 9d2c999 differs from pull request most recent head e5cd848
Please upload reports for the commit e5cd848 to get more accurate results.
:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.
Hello @derek-ho,
Is this pull request ready for review?
Some logic may need to be added after 2.12.0 release in test files like here: https://github.com/opensearch-project/opensearch-py/blob/main/opensearchpy/helpers/test.py#L37 to replace this with new password conditionally, based on the version the CI is running against.
@saimedhi @VachaShah this PR is ready for review now - I am not sure what approach you folks think is appropriate. I think this PR should be able to be merged as is, but after 2.12.0 I would expect some tests to fail for 2.12.0 release, such as here: https://github.com/opensearch-project/opensearch-py/blob/main/opensearchpy/helpers/test.py#L37. What do you folks think?
@saimedhi @VachaShah this PR is ready for review now - I am not sure what approach you folks think is appropriate. I think this PR should be able to be merged as is, but after 2.12.0 I would expect some tests to fail for 2.12.0 release, such as here: https://github.com/opensearch-project/opensearch-py/blob/main/opensearchpy/helpers/test.py#L37. What do you folks think?
If it's not urgent, we can wait until the 2.12.0 release to make the code changes. This allows us to align with the latest release. @VachaShah what do you think?
I am wondering why we can't make all the changes now since we already test this client against the developing 2.x line. @derek-ho I believe you should be able to add the version checks in as well and test the code out. LMK if I am missing something here.
I am wondering why we can't make all the changes now since we already test this client against the developing 2.x line. @derek-ho I believe you should be able to add the version checks in as well and test the code out. LMK if I am missing something here.
@VachaShah I believe those tests are run against 2.x OpenSearch without security, so thus we are not seeing those fail yet.
@saimedhi @VachaShah this PR is ready for review now - I am not sure what approach you folks think is appropriate. I think this PR should be able to be merged as is, but after 2.12.0 I would expect some tests to fail for 2.12.0 release, such as here: https://github.com/opensearch-project/opensearch-py/blob/main/opensearchpy/helpers/test.py#L37. What do you folks think?
If it's not urgent, we can wait until the 2.12.0 release to make the code changes. This allows us to align with the latest release. @VachaShah what do you think?
I am ok with waiting for now, if the regular CI is not broken. I will turn this PR to draft for now.
@VachaShah I believe those tests are run against 2.x OpenSearch without security, so thus we are not seeing those fail yet.
Are you able to help enable that?
Now that 2.12 is released this should be unblocked. Would the maintainers review and merge this?
@derek-ho Is this ready for review?
@DarshitChanpura also needs a rebase, please
@derek-ho Would you please rebase this and mark as ready for review? I don't have write access to push to your branch.
tests without security are still failing
@derek-ho Please fix these failing tests https://github.com/opensearch-project/opensearch-py/actions/runs/8072055726/job/22087332746
@derek-ho Please fix these failing tests https://github.com/opensearch-project/opensearch-py/actions/runs/8072055726/job/22087332746
I am pretty sure the without security tests are not because of this change - I see them failing on other PRs as well. I am working on fixing the 2.12 with security test case, but running into issues with extracting version information within the test, as it seems to only be available outside of the test case. I will try a bit longer, but may need maintainers help for that.
@derek-ho Please fix these failing tests https://github.com/opensearch-project/opensearch-py/actions/runs/8072055726/job/22087332746
I am pretty sure the without security tests are not because of this change - I see them failing on other PRs as well. I am working on fixing the 2.12 with security test case, but running into issues with extracting version information within the test, as it seems to only be available outside of the test case. I will try a bit longer, but may need maintainers help for that.
Yes, other failures are unrelated to this PR. Apologies for any confusion caused. I specifically referred to version (2.12.0, true) as seen here: link. Thank you :)
Hello @derek-ho,
Could you please take a look at finishing this PR when you have a moment?
Thanks a lot!
Hello @derek-ho, could you please take a look at this PR and let me know if it's ready for review? opensearch-py currently lacks compatibility with the latest OpenSearch versions 2.12.0, 2.13.0, and 2.14.0. It would be great if we could resolve this issue and get the PR merged soon. Are you facing any difficulties or need assistance in this regard?
@derek-ho I'm good with what's here if you can get it to green.
Without trying it, I think we can specify the username/password in .github workflow and remove all the logic from the .sh scripts. We can also do this later.
Agree @dblock there is probably a simpler way to do it, but I am not an expert in Python and just focused on getting it to a good state that we can start testing against the newer versions. It can be cleaned up in a follow up, but I may need assistance for that part.
Agree @dblock there is probably a simpler way to do it, but I am not an expert in Python and just focused on getting it to a good state that we can start testing against the newer versions. It can be cleaned up in a follow up, but I may need assistance for that part.
I'm good with whatever works. We can improve later.
@dblock the CI seems to pass with the latest changes, can we merge this as is? Do you want me to create a follow up issue to clean up the logic/code for CI?
Created follow up issue here: https://github.com/opensearch-project/opensearch-py/issues/764
Description
Starting with 2.12.0, security plugin requires an initial admin password to be set via env. variable when installing the demo configuration. This PR is to update documentation and CI to accommodate that change.
Issues Resolved
Closes: https://github.com/opensearch-project/opensearch-py/issues/759
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.