Skip to content

Resurrect python openapi client generator #19155

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.

Sign up for GitHub

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

Jump to bottom
Merged
uranusjr merged 4 commits into from
Nov 11, 2021
Merged

Resurrect python openapi client generator #19155

uranusjr merged 4 commits into from
Nov 11, 2021

Conversation

@hterik
Copy link
Contributor

@hterik hterik commented Oct 22, 2021

These changes enable the airflow-client-python to be generated and used successfully.

  • Update openapi generator to 5.2.1, this fixes the client to not send readonly-fields which was the reason for most of the client to not work at all.
    Fixes airflow/airflow-client-python#4
    Fixes airflow/airflow-client-python#21
  • Ability to run generator on Ubuntu, previous sed-flags assumed OSX only.
  • Update version number to match latest airflow release 2.2.0
  • Improve console output on successful generation.

With these changes, I've tried generating the client and can now successfully call the post_dag_run from it, which was not possible before.
Generated airflow-client-python/airflow_client/test unit tests are passing.

@boring-cyborg boring-cyborg bot added the area:API Airflow's REST/HTTP API label Oct 22, 2021
This was referenced Oct 22, 2021
Closed
Closed
Closed
zhongjiajie
zhongjiajie reviewed Oct 22, 2021
View reviewed changes
Copy link
Member

@zhongjiajie zhongjiajie left a comment

Choose a reason for hiding this comment

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

Add somethink NIT here

msumit
msumit reviewed Oct 22, 2021
View reviewed changes
@github-actions github-actions bot added the okay to merge It's ok to merge this PR as it does not require more tests label Oct 25, 2021
@github-actions
Copy link

github-actions bot commented Oct 25, 2021

The PR is likely ready to be merged. No tests are needed as no important environment files, nor python files were modified by it. However, committers might decide that full test matrix is needed and add the 'full tests needed' label. Then you should rebase it to the latest main or amend the last commit of the PR, and push it with --force-with-lease.

@hterik hterik force-pushed the ressurect_python_openapi_client branch from 18ed558 to 99c13d7 Compare October 25, 2021 06:26
@hterik
Copy link
Contributor Author

hterik commented Nov 2, 2021

I've removed the double-run of pre-commit.
Anything else needed for proceeding with this PR?

@uranusjr
Copy link
Member

uranusjr commented Nov 2, 2021

Instead of || true, we should check $? and only show the message on failures.

@msumit msumit mentioned this pull request Nov 2, 2021
Closed
msumit
msumit reviewed Nov 2, 2021
View reviewed changes
@msumit
Copy link
Contributor

msumit commented Nov 2, 2021

Instead of || true, we should check $? and only show the message on failures.

Given that we are deleting lots of folders before generating code & then running pre-commit, so it's always going to return with a non-zero exit code. Thus no point in adding a if check, just to display the message, which anyway always going to be displayed.

@uranusjr
Copy link
Member

uranusjr commented Nov 2, 2021

Hmm OK then

@christhekeele christhekeele mentioned this pull request Nov 9, 2021
Closed
hterik added 4 commits November 9, 2021 06:59
@hterik
Make python openapi generator run on non OSX systems.
03cf370 
On OSX, sed -i always requires the suffix argument, so it
is normally passed as empty string when not desired.

On other linux, for example Ubuntu, it should not be passed at
all when it is empty. This gave errors about file not found when
running the generator.
@hterik
Update openapi generator to 5.3.0
7b4f421 
This new version is required to exclude readonly fields from being
sent in the request.

Fixes airflow/airflow-client-python#4
Fixes airflow/airflow-client-python#21
@hterik
Update version number to 2.2.1 for python api gen.
655ab69
@hterik
Improve success message when running api generator
e1dfb5c 
pre-commit would print failures, which were ok to ignore
due to the "|| true", caused by hook adding licence headers.
To new eyes, this was not clear if the generation was successful
or not if just looking at console output. Added a more descriptive
message to explain why ignoring this failure is ok.

Finally add a success message at the end and enable bash exit-on-error
mode to be absolutely sure the script ran to completion.
@hterik hterik force-pushed the ressurect_python_openapi_client branch from 99c13d7 to e1dfb5c Compare November 9, 2021 06:06
@uranusjr uranusjr merged commit 8d63bdf into apache:main Nov 11, 2021
@boring-cyborg
Copy link

boring-cyborg bot commented Nov 11, 2021

Awesome work, congrats on your first merged pull request!

@hterik hterik deleted the ressurect_python_openapi_client branch November 11, 2021 10:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@msumit msumit msumit approved these changes

@zhongjiajie zhongjiajie zhongjiajie left review comments

@uranusjr uranusjr uranusjr approved these changes

Assignees

No one assigned

Labels

area:API Airflow's REST/HTTP API okay to merge It's ok to merge this PR as it does not require more tests

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@hterik @uranusjr @msumit @zhongjiajie