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

OpenAPI generator upgrade (7.0.0) + Python lakefs-sdk #6562

Merged
merged 14 commits into from
Oct 4, 2023

Conversation

Jonathan-Rosenberg
Copy link
Contributor

Closes #6425

Change Description

This PR doesn't actually close the issue, becuase I couldn't find true forward compatibility issues with the current SDK, but it's preferred to upgrade the OpenAPI generator as it's pretty old (Oct 24, 2021)...
This will not break the current behavior and examples from the docs because it's not published as the lakefs-client project but as a new lakefs-sdk project.

Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

Thanks!

  • I would like to share a common prefix on all Makefile targets of SDKs / generated API
  • I prefer to name the existing client targets "legacy"
  • I think we need a check that the Python SDK is checked-in, like we already have for the "legacy" Python SDK
  • Once ll this is done, please also check in the generated Python client.

@@ -28,14 +28,22 @@ jobs:
- name: Python build and make package
run: make package-python PACKAGE_VERSION=${{ steps.version.outputs.tag }}

- name: Python publish package
- name: Python client publish package
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
- name: Python client publish package
- name: Python SDK publish package

?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SDK legacy

Makefile Outdated
Comment on lines 19 to 21
OPENAPI_GENERATOR_VERSION_ADVANCED=v7.0.0
OPENAPI_GENERATOR_IMAGE_ADVANCED=openapitools/openapi-generator-cli:$(OPENAPI_GENERATOR_VERSION_ADVANCED)
OPENAPI_GENERATOR_ADVANCED=$(DOCKER) run --user $(UID_GID) --rm -v $(shell pwd):/mnt $(OPENAPI_GENERATOR_IMAGE_ADVANCED)
Copy link
Contributor

Choose a reason for hiding this comment

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

The more I think about these names the less I am a fan of them. Perhaps call them "OPENAPI_GENERATOR_SERVER" and "OPENAPI_GENERATOR_CLIENT_SDK"? (Extremely unimportant, any which way you decide)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

the only difference between them is the openAPI version they use. This is the reason that I've added the ADVANCED part to the newer version.
They both generate the client's SDK

Makefile Outdated

client-python: api/swagger.yml ## Generate SDK for Python client
python-client: api/swagger.yml ## Generate SDK for Python client - openapi generator version 5.3.0
Copy link
Contributor

Choose a reason for hiding this comment

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

Prefer to keep a common prefix for all SDKs, like we have common prefixes for other things here.

Suggested change
python-client: api/swagger.yml ## Generate SDK for Python client - openapi generator version 5.3.0
sdk-python-legacy: api/swagger.yml ## Generate SDK for Python client - openapi generator version 5.3.0

Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

Comments about the template files, too!

Gemfile.lock
Gemfile
_config.yml
templates/
Copy link
Contributor

Choose a reason for hiding this comment

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

This file looks wrong. Can we get a better file, maybe a "Python gitignore" will do better?


@property
def actions(self):
warnings.warn("Deprecated property. Use actions_api instead.", DeprecationWarning)
Copy link
Contributor

Choose a reason for hiding this comment

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

It's a new SDK. Let's not do this!

"aiohttp >= 3.0.0",
{{/asyncio}}
{{#tornado}}
"tornado>=4.2,<5",
Copy link
Contributor

Choose a reason for hiding this comment

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

We need this?

author="{{infoName}}{{^infoName}}OpenAPI Generator community{{/infoName}}",
author_email="{{infoEmail}}{{^infoEmail}}[email protected]{{/infoEmail}}",
url="{{packageUrl}}",
keywords=["OpenAPI", "OpenAPI-Generator", "{{{appName}}}"],
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
keywords=["OpenAPI", "OpenAPI-Generator", "{{{appName}}}"],
keywords=["OpenAPI", "auto-generated", "{{{appName}}}"],

It's not an OpenAPI generator. We can add something longer that says it was auto-generated in the description, though.

@Jonathan-Rosenberg Jonathan-Rosenberg marked this pull request as ready for review October 2, 2023 12:40
@Jonathan-Rosenberg Jonathan-Rosenberg added the include-changelog PR description should be included in next release changelog label Oct 4, 2023
Copy link
Contributor

@arielshaqed arielshaqed left a comment

Choose a reason for hiding this comment

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

Do we need swagger2.yml? (Because it might be folded in the default GitHub view...)

Other than that -- small comments only. Very very neat, thanks!

description: Tag/version to publish
required: true
project_name:
description: The Pypi (test) package name
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
description: The Pypi (test) package name
description: The PyPI (test) package name

Comment on lines +83 to +100
# - name: Build release as latest
# working-directory: clients/python
# run: bundle exec jekyll build -d _site -b /

# - name: Build release ${{ steps.docver.outputs.tag }}
# working-directory: clients/python
# run: bundle exec jekyll build -d _site/${{ steps.docver.outputs.tag }} -b /${{ steps.docver.outputs.tag }}

# - name: Publish to docs repository
# uses: dmnemec/[email protected]
# env:
# API_TOKEN_GITHUB: ${{ secrets.PERSONAL_TOKEN }}
# with:
# source_file: clients/python/_site/.
# destination_repo: treeverse/docs-lakefs-sdk-python
# destination_folder: /
# user_email: '[email protected]'
# user_name: 'python-docs-action'
Copy link
Contributor

Choose a reason for hiding this comment

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

?

Copy link
Contributor Author

@Jonathan-Rosenberg Jonathan-Rosenberg Oct 4, 2023

Choose a reason for hiding this comment

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

Disabled the publish of the new SDK until after I test it over TestPypi

api/swagger2.yml Outdated
@@ -0,0 +1,4631 @@
openapi: "3.0.0"
Copy link
Contributor

Choose a reason for hiding this comment

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

Do we need this file?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch.
Was checked in by mistake

# Conflicts:
#	clients/python-legacy/lakefs_client/api/retention_api.py
#	clients/python/.openapi-generator/FILES
#	clients/python/docs/RetentionApi.md
#	clients/python/test/test_retention_api.py
@Jonathan-Rosenberg Jonathan-Rosenberg merged commit 621b8c9 into master Oct 4, 2023
32 checks passed
@Jonathan-Rosenberg Jonathan-Rosenberg deleted the feature/upgrade-opengenerator-version branch October 4, 2023 13:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
include-changelog PR description should be included in next release changelog
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[1.0] Forwards-compatible Python clients
2 participants