Skip to content

Conversation

@pierrejeambrun
Copy link
Member

@pierrejeambrun pierrejeambrun commented Jul 19, 2022

Following #25123, allowing us to auto-generate typescript types for the api, here is a pre-commit hook to ensure that these types stay in sync with the OpenAPI spec.

Modifying the openapi/v1.yaml will trigger the type generation. (then could eventually break ESlint hook when attempting again to commit, in case of incompatible changes)

@boring-cyborg boring-cyborg bot added area:API Airflow's REST/HTTP API area:dev-tools area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues labels Jul 19, 2022
@pierrejeambrun pierrejeambrun force-pushed the generate-api-ts branch 3 times, most recently from 8936ad3 to 83574fa Compare July 19, 2022 22:19
Copy link
Contributor

@bbovenzi bbovenzi left a comment

Choose a reason for hiding this comment

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

looking great!

@bbovenzi
Copy link
Contributor

I merged my PR. Want to rebase and update the PR title and description?

@pierrejeambrun pierrejeambrun changed the title Generate typescript types from rest API docs Add generate-api-types pre-commit hook Jul 20, 2022
@pierrejeambrun pierrejeambrun changed the title Add generate-api-types pre-commit hook Add "generate-api-types" pre-commit hook Jul 20, 2022
@pierrejeambrun
Copy link
Member Author

pierrejeambrun commented Jul 20, 2022

The PR has been updated, we should be good

@potiuk
Copy link
Member

potiuk commented Jul 20, 2022

One small comment - it woudl be - I think - much better to merge this one with compile-www-assets - then same node env will be used and single yarn install command would be needed

@potiuk
Copy link
Member

potiuk commented Jul 20, 2022

Those are triggered by different source files - but I think overall it would be more efficient.

@pierrejeambrun
Copy link
Member Author

pierrejeambrun commented Jul 20, 2022

The compile-www-assets has a manual confined stage, should I extend this with commit ?

@potiuk
Copy link
Member

potiuk commented Jul 20, 2022

Right.... Forgot about it 🤦 But the "lint" one does not :). This what Is actually better "typescript compile lint" sems like good idea

@potiuk
Copy link
Member

potiuk commented Jul 20, 2022

On the other hand. isolation is better in this case rather than optimisation.

@pierrejeambrun
Copy link
Member Author

pierrejeambrun commented Jul 20, 2022

Following your suggestions, here is another option that merges the generate-api-types hook into lint-javascript (+ renaming reflecting this change).

Let me know what version is preferable :)

@pierrejeambrun pierrejeambrun changed the title Add "generate-api-types" pre-commit hook Add ts types generation to static checks Jul 20, 2022
Copy link
Member

Choose a reason for hiding this comment

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

Let's change the description too.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes I didn't change it cause we are limited in length :(, trying to find something better

Copy link
Member

@potiuk potiuk Jul 20, 2022

Choose a reason for hiding this comment

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

Generate and lint UI Javascript files ?

Copy link
Member Author

@pierrejeambrun pierrejeambrun Jul 20, 2022

Choose a reason for hiding this comment

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

I went for TS types generation and ESLint against current UI files (that is bellow < 70char) I wanted to remove the reference to Javascript as it is not really accurate anymore

@potiuk
Copy link
Member

potiuk commented Jul 20, 2022

I think this one is better. In VAST majority of cases running 'generate-api-types' will produce the same file.

Unfortunately - unlike in Python, pre-commit does not reuse node environments, so the less of them, the better for time/cache restore and creation etc:

⌁ [jarek:~/.cache/pre-commit] % echo */*/lib/node_modules | xargs du -h -s
 21M	repocw333njc/node_env-18.6.0/lib/node_modules
 48M	repoltxx29sq/node_env-18.6.0/lib/node_modules
 19M	repowd9np_qb/node_env-18.6.0/lib/node_modules
 20M	repoy6z0rp4o/node_env-16.15.1/lib/node_modules

@pierrejeambrun
Copy link
Member Author

Ok great thanks for the details :)

@potiuk
Copy link
Member

potiuk commented Jul 20, 2022

BTW. Is it possible @pierrejeambrun - you also remove all the node versions 18.6.0 in all node pre-commits and only update the version in the "top-level" default_language_version? I just realized that we have it by looking at those cached envs.

@pierrejeambrun
Copy link
Member Author

Done, using the default node version and updated it to 18.6.0

@potiuk potiuk closed this Jul 20, 2022
@potiuk potiuk reopened this Jul 20, 2022
@potiuk
Copy link
Member

potiuk commented Jul 20, 2022

Closed/reopened to rebuild as there was temp dockerhub failure.

@pierrejeambrun
Copy link
Member Author

pierrejeambrun commented Jul 20, 2022

Closed/reopened to rebuild as there was temp dockerhub failure.

Thanks,

I didn't know that trick, I'm used to amending and repushing 😂

@potiuk
Copy link
Member

potiuk commented Jul 20, 2022

Closed/reopened to rebuild as there was temp dockerhub failure.

Thanks,

I didn't know that trick, I'm used to amending and repushing 😂

That's what I do with my changes too :)

@potiuk potiuk merged commit 8c87246 into apache:main Jul 20, 2022
@pierrejeambrun pierrejeambrun deleted the generate-api-ts branch July 20, 2022 22:34
@ephraimbuddy ephraimbuddy added the type:misc/internal Changelog: Misc changes that should appear in change log label Aug 14, 2022
@ephraimbuddy ephraimbuddy added changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..) and removed type:misc/internal Changelog: Misc changes that should appear in change log labels Sep 14, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:API Airflow's REST/HTTP API area:dev-tools area:UI Related to UI/UX. For Frontend Developers. area:webserver Webserver related Issues changelog:skip Changes that should be skipped from the changelog (CI, tests, etc..)

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants