Skip to content

Pre commit new UI#14836

Merged
ryanahamilton merged 6 commits into
apache:masterfrom
astronomer:pre-commit-new-ui
Mar 16, 2021
Merged

Pre commit new UI#14836
ryanahamilton merged 6 commits into
apache:masterfrom
astronomer:pre-commit-new-ui

Conversation

@bbovenzi

@bbovenzi bbovenzi commented Mar 16, 2021

Copy link
Copy Markdown
Contributor

Add pre-commit hook to lint and test the new react UI.

Closes #14726


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@boring-cyborg boring-cyborg Bot added the area:UI Related to UI/UX. For Frontend Developers. label Mar 16, 2021
@bbovenzi bbovenzi marked this pull request as ready for review March 16, 2021 16:52
@ryanahamilton ryanahamilton added AIP-38 Modern Web Application area:dev-tools labels Mar 16, 2021
Comment thread .pre-commit-config.yaml Outdated
- id: ui-lint-test
name: Lint and test React UI
language: system
entry: ./scripts/ci/pre_commit/pre_commit_ui_lint_test.sh

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I believe using system hook and script is not needed (but I am not sure if it works with yarn)

pre-commit has a built-in language: node and it understand package.json. Not sure about yarn.lock though . But if we can make it works - it will even automatically rebuild the node environment for you and allow to run yarn commands.

You can some examples here:

https://github.com/Exocortex/pre-commit/blob/master/.pre-commit-hooks.yaml

Also I think you should select the files that the lint should run on - .js files basically. Pre-commit has the nice feature that it only runs the checks on files that are changed in your commit but also splits the checks to N parallell checks - as many as many processors you have. So for sure you need "files:" field to tell pre-commit which files it should run on and your command (either script or yarn command) should take those files as parameters.

For example when you only change a.js, b.js, c.js but not d.js and you have 2 CPUs precommit should run two commands in parallel:

yarn lint a.js b.js
yarn lint c.js

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Good call. Updated. I also made lint and test separate hooks.

@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@github-actions

Copy link
Copy Markdown
Contributor

The Workflow run is cancelling this PR. It has some failed jobs matching ^Pylint$,^Static checks,^Build docs$,^Spell check docs$,^Backport packages$,^Provider packages,^Checks: Helm tests$,^Test OpenAPI*.

@potiuk potiuk left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Sooooo much better now!

@ryanahamilton ryanahamilton merged commit e395fcd into apache:master Mar 16, 2021
@ryanahamilton ryanahamilton deleted the pre-commit-new-ui branch March 16, 2021 23:06
ryanahamilton added a commit to astronomer/airflow that referenced this pull request Mar 24, 2021
ryanahamilton added a commit that referenced this pull request Mar 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

AIP-38 Modern Web Application area:dev-tools area:UI Related to UI/UX. For Frontend Developers.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add precommit linting and testing to the new /ui

4 participants