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

Upgrade code to use openapi-generator-cli v7.0.1.1 #8652

Merged
merged 4 commits into from
Feb 18, 2025

Conversation

nopcoder
Copy link
Contributor

@nopcoder nopcoder commented Feb 12, 2025

Close #8558

- Update fixes from v7.0.1
- Apply patch for python - Encode list query params (#20148)[OpenAPITools/openapi-generator#20148]
- Minor Python markdown spaces for the docs
@nopcoder nopcoder self-assigned this Feb 12, 2025
@nopcoder nopcoder requested a review from N-o-Z February 12, 2025 22:07
@nopcoder nopcoder added include-changelog PR description should be included in next release changelog area/sdk/python labels Feb 12, 2025
Copy link
Member

@N-o-Z N-o-Z left a comment

Choose a reason for hiding this comment

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

I'm worried that I might be missing something as this is a big change.
Here's my proposal:
Lets try to do it iteratively. By that I mean lets start with updating only the open API generator image for the python client. This way we reduce the risk to introduce breaking changes into the Java client.
We can then test the python client intensively (suggest to start by running the python wrapper unit and integration tests using that client).
WDYT?

@nopcoder
Copy link
Contributor Author

nopcoder commented Feb 13, 2025

@N-o-Z nothing to worry about the change the Java SDK - it was just removing unused import java.util.Map.Entry.

If there was an easy way to do tell the diff - hide this change from all files, it would make generated code review much easier.

Will look into check the original issue that was opened for the python sdk and the conflict that I am more worried about as it seems that the generated code was modified.

@nopcoder nopcoder requested a review from N-o-Z February 13, 2025 11:48
@N-o-Z
Copy link
Member

N-o-Z commented Feb 13, 2025

@nopcoder running the python wrapper utests using the new SDK code I get the following error:

________________________ ERROR collecting tests/utests/test_tag.py ________________________
../../../../venv/python-wrapper-test1/lib/python3.10/site-packages/_pytest/python.py:617: in _importtestmodule
    mod = import_path(self.path, mode=importmode, root=self.config.rootpath)
../../../../venv/python-wrapper-test1/lib/python3.10/site-packages/_pytest/pathlib.py:567: in import_path
    importlib.import_module(module_name)
/usr/lib/python3.10/importlib/__init__.py:126: in import_module
    return _bootstrap._gcd_import(name[level:], package, level)
<frozen importlib._bootstrap>:1050: in _gcd_import
    ???
<frozen importlib._bootstrap>:1027: in _find_and_load
    ???
<frozen importlib._bootstrap>:1006: in _find_and_load_unlocked
    ???
<frozen importlib._bootstrap>:688: in _load_unlocked
    ???
../../../../venv/python-wrapper-test1/lib/python3.10/site-packages/_pytest/assertion/rewrite.py:186: in exec_module
    exec(co, module.__dict__)
utests/test_tag.py:1: in <module>
    from tests.utests.common import get_test_client
utests/common.py:6: in <module>
    import lakefs_sdk
../../python/lakefs_sdk/__init__.py:21: in <module>
    from lakefs_sdk.api.actions_api import ActionsApi
../../python/lakefs_sdk/api/__init__.py:4: in <module>
    from lakefs_sdk.api.actions_api import ActionsApi
../../python/lakefs_sdk/api/actions_api.py:37: in <module>
    from lakefs_sdk.api_client import ApiClient
E     File "/home/niro/dev/lakefs/clients/python/lakefs_sdk/api_client.py", line 504
E       new_params.extend((k, quote(str(value)) for value in v)
E                                               ^^^
E   SyntaxError: invalid syntax

@nopcoder
Copy link
Contributor Author

@N-o-Z fixed the generator image, pushed a fix to this branch and tested utests.

Copy link

E2E Test Results - DynamoDB Local - Local Block Adapter

14 passed

Copy link

E2E Test Results - Quickstart

11 passed

Copy link
Member

@N-o-Z N-o-Z left a comment

Choose a reason for hiding this comment

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

Verified Python wrapper utests and integration tests with new SDK

@nopcoder nopcoder merged commit f0bdd50 into master Feb 18, 2025
40 checks passed
@nopcoder nopcoder deleted the chore/update-api-codegen-7.0.1.1 branch February 18, 2025 16:21
@joostsijm
Copy link

Thanks for the support. FYI, also submitted a solution quote bug to the OpenAPI-generator python-pydantic-v1 generator, which has been merged: OpenAPITools/openapi-generator#20614

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/sdk/python include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[Bug]: Python client "LogCommits" doesn't quote query parameters
3 participants