Create a wrapper script to generate python client; regenerate the python client #1347
Conversation
|
@snazy can you take a look at this draft? My impression is that I should be using the headers from |
eric-maynard
force-pushed
the
2025-04-09-python
branch
from
592d2af to
56477d4
Compare
eric-maynard
force-pushed
the
2025-04-09-python
branch
from
87accb7 to
68ac664
Compare
eric-maynard
force-pushed
the
2025-04-09-python
branch
from
592e296 to
53e8503
Compare
eric-maynard
requested review from
adutra,
ashvina,
dennishuo,
dimas-b,
jackye1995,
jbonofre,
vvcephei,
collado-mike,
snazy,
RussellSpitzer,
takidau,
MonkeyCanCode,
flyrain and
ebyhr
as code owners
| @@ -260,6 +260,7 @@ paths: | |||
| parameters: | |||
| - $ref: '#/components/parameters/page-token' | |||
| - $ref: '#/components/parameters/page-size' | |||
| - $ref: '#/components/parameters/prefix' | |||
There was a problem hiding this comment.
This change is needed due to an unfortunate bug in the python generator wherein parameters are overriden, not extended. Without this, the method list_namespaces doesn't have a prefix argument.
There was a problem hiding this comment.
Shall we make a note somewhere (e.g. spec/README.md) that these changes are needed? Since iceberg-rest-catalog-open-api.yaml should be a copy-paste of upstream one, we may easily lose track of these additional changes the next time we copy-paste from the upstream.
There was a problem hiding this comment.
Ideally, the CI will fail when a bad copy/paste happens. We need to wire up the CI to the new task but I would like to do that in a followup. For now, let me add this in the README!
There was a problem hiding this comment.
Nit: it'd be nice to add the comment directly here like Non functional change required by Python generator. I believe this is the place where people may get confused every time pulling iceberg spec upstream.
| "./polaris/management/__pycache__/" | ||
| "./polaris/management/models/__pycache__/" | ||
| "./polaris/management/api/__pycache__/" | ||
| ) |
There was a problem hiding this comment.
[non-blocker] Shall we add ./.DS_STORE to exclude_paths for mac users? This makes running regenerate.sh fails in my mac locally with
Re-applying license headers
No header compatible with file ./.DS_Store
There was a problem hiding this comment.
| ) | |
| "./.DS_Store" | |
| "./poetry.lock" | |
| ) |
Also "poetry.lock" should be excluded
There was a problem hiding this comment.
Synced with Eric offline, I believe we can solve this in a follow-up which automate the regeneration during test
github-project-automation
bot
moved this from PRs In Progress
to Ready to merge
in Basic Kanban Board
github-project-automation
bot
moved this from Ready to merge
to Done
in Basic Kanban Board
I ran these commands from main: ``` redocly bundle spec/polaris-catalog-service.yaml -o spec/generated/bundled-polaris-catalog-service.yaml ./gradlew regeneratePythonClient ``` I didn't realize before that some Python types are generated form the bundled spec, so some of the fixes from #1347 didn't get properly applied before.
It looks like this method lost the `prefix` and `namespace` parameters in #1347. They're re-introduced to the spec here, and then I've run: ``` redocly bundle spec/polaris-catalog-service.yaml -o spec/generated/bundled-polaris-catalog-service.yaml ./gradlew regeneratePythonClient ``` Then, some manual reverts: ``` alias gitrevert='git checkout upstream/main --' gitrevert client/python/.github/workflows/python.yml gitrevert client/python/.gitlab-ci.yml gitrevert client/python/pyproject.toml ``` I still hope to automate this process as part of CI soon; see #1675
As noted in #755 and elsewhere, the generated types in
client/pythonare currently out of date. This introduces a script to regenerate them and a gradle task to run that script.I've also run the script, which necessitated several things to get tests passing: