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

Add override for code generator to change indices.put_alias argument order #804

Merged
merged 3 commits into from
Aug 22, 2024
Merged

Add override for code generator to change indices.put_alias argument order #804

merged 3 commits into from
Aug 22, 2024

Conversation

Tenzer
Copy link
Contributor

@Tenzer Tenzer commented Aug 22, 2024

Description

This solves an issue introduced in v2.7.0 where the argument order of the indices.put_alias() method were altered, resulting in a breaking change for any user that called the function without naming the arguments provided.

Issues Resolved

Closes #803.

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.

Copy link

codecov bot commented Aug 22, 2024

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 70.32%. Comparing base (ba715b9) to head (4be23a6).
Report is 54 commits behind head on main.

Files Patch % Lines
opensearchpy/_async/client/indices.py 66.66% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #804      +/-   ##
==========================================
- Coverage   71.95%   70.32%   -1.64%     
==========================================
  Files          91      113      +22     
  Lines        8001     8868     +867     
==========================================
+ Hits         5757     6236     +479     
- Misses       2244     2632     +388     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@dblock dblock left a comment

Choose a reason for hiding this comment

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

Works for me! Thanks for fixing this.

@dblock
Copy link
Member

dblock commented Aug 22, 2024

@Tenzer did you happen to go through a diff of other APIs that might have this problem? Maybe there's a way to do this systematically and add tests, including in the generator 🤔 I want to double check before cutting a newer release.

@dblock dblock merged commit e26f3fc into opensearch-project:main Aug 22, 2024
38 of 40 checks passed
@Tenzer
Copy link
Contributor Author

Tenzer commented Aug 22, 2024

You're welcome! No, I haven't looked through changes to other APIs. I only noticed the issue with put_alias() because we used that as part of our test code.

@dblock
Copy link
Member

dblock commented Aug 22, 2024

I looked at the 2.6.0 - 2.7.0 diff and found one more change in post_dashboards_info, v2.6.0...v2.7.0#diff-d373e8c1b532ced908538444607c89e5a80cfe7a24ef5c81a3708e0fb2cf48fdL2173, I think it's cause it's missing a request body in the spec.

cc: @DarshitChanpura

@DarshitChanpura
Copy link
Member

@dblock this is interesting, post is exposed but doesn't really consume any body for the request.
https://github.com/opensearch-project/security/blob/main/src/main/java/org/opensearch/security/rest/DashboardsInfoAction.java#L94
We only seem to be using get request:
https://github.com/opensearch-project/security-dashboards-plugin/blob/main/server/routes/index.ts#L584
https://github.com/opensearch-project/security-dashboards-plugin/blob/main/public/utils/dashboards-info-utils.tsx#L21-L34

Also, could you point the exact line change for post_dashboards_info that might be causing this. v2.6.0...v2.7.0#diff-d373e8c1b532ced908538444607c89e5a80cfe7a24ef5c81a3708e0fb2cf48fdL2173 doesn't seem to match any text when searching for "dashboards_info". Maybe I missed something?

@dblock
Copy link
Member

dblock commented Aug 22, 2024

@DarshitChanpura Thanks. I think we're all good, since this method doesn't take a body it was a bug fix.

I am sure where this is evaluated in the client code generator, but you can dig through https://github.com/opensearch-project/opensearch-py/blob/main/utils/generate_api.py.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[BUG] v2.7.0 brings undocumented breaking change to put_index()
3 participants