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

Skip integration tests for new UI only PR. Run docs build only when needed. #43512

Merged
merged 8 commits into from
Oct 30, 2024

Conversation

tirkarthi
Copy link
Contributor

This is a follow-up on #43503 . Following are the changes.

  1. Made only_new_ui_files as an attribute that is true when there are any source files and they contain only new UI changes. Sometimes the PR has no source code changes so instead of diff on 2 empty sets this ensures there are source files and changes in new UI folder with no remaining files left. I am thinking of skipping db tests that now run with "always" that takes 2 mins down from 20 mins earlier but is not needed in new UI only PRs.
  2. docs-build flag is not used in the CI configuration causing docs build to always run that is not needed in new UI only PRs that don't change any user facing docs. Sometimes the build runs for an hour or more with intermittent failures and retry. This ensures to build docs only when needed. If there are scenarios where the flag is False with docs required then this could be improved.
  3. Integration tests are still executed since there is a default list of integrations. Using the only_new_ui_files return an empty list so that these tests are skipped for PR with only new UI changes. Each test takes 2 minutes.

@tirkarthi
Copy link
Contributor Author

tirkarthi commented Oct 30, 2024

docs-build = true yet the docs build step in CI seems to be skipped.

https://github.com/apache/airflow/actions/runs/11594095043/job/32279562944?pr=43512#step:8:731

@potiuk
Copy link
Member

potiuk commented Oct 30, 2024

docs-build = true yet the docs build step in CI seems to be skipped.

https://github.com/apache/airflow/actions/runs/11594095043/job/32279562944?pr=43512#step:8:731

You also need to add "docs-build" as input of the composite static-checks-mypy-docs.yml workflow and pass it in ci.yml.

Unfortunately GitHub Actions convers unknown variables, inputs etc into empty string 😱 and in this case you have:

if inputs.docs-build == 'true'

turns into

if '' == 'true'

I opened (it is still opened) a github support request for that around 3 years ago. But I am afraid the ship has sailed - there are far too many workflows that would be broken if they introduce an error on undefined variables in their expression language :(

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

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

Nice. Other than the input that needs to be added to the composite workfw - it is a very nice optimisation!

@tirkarthi tirkarthi force-pushed the ci-only-new-ui-tests branch from a2ec4f9 to 54f4784 Compare October 30, 2024 15:08
@tirkarthi tirkarthi closed this Oct 30, 2024
@tirkarthi tirkarthi reopened this Oct 30, 2024
@tirkarthi
Copy link
Contributor Author

tirkarthi commented Oct 30, 2024

As per my understanding it seems ci.yml has docs-build . I just added it to static-checks-mypy-docs.yml and I am getting email notifications that "PR run failed at startup:" and "no jobs were run". Maybe I missed something else.

https://github.com/tirkarthi/airflow/blob/54f47844e0da031130d97eb234d607554c86cb5f/.github/workflows/ci.yml#L102

https://github.com/tirkarthi/airflow/blob/54f47844e0da031130d97eb234d607554c86cb5f/.github/workflows/static-checks-mypy-docs.yml#L95-L98

@tirkarthi
Copy link
Contributor Author

Thanks @potiuk for the details. These look like long standing issues and similar ones from 2021 reports below. It looks even required: true doesn't work as expected.

actions/runner#924
actions/runner#1070

@tirkarthi
Copy link
Contributor Author

Ok, I had to pass docs-build in static-checks-mypy-docs section in ci.yml from build-info. I thought outputs from ci.yml is automatically available in other builds. My bad. It seems to be building now. Thanks.

@potiuk potiuk merged commit b94b1a1 into apache:main Oct 30, 2024
82 checks passed
ellisms pushed a commit to ellisms/airflow that referenced this pull request Nov 13, 2024
…eeded. (apache#43512)

* Run only ui tests for PR with new UI only changes.

* Revert test change to UI folder.

* Skip integration tests for new UI changes only PR.

* Build docs only when needed. Move new UI files only test to a separate attribute for more enhancements.

* Fix attribute.

* Add docs-build to pass it to build.

* Add docs-build in ci.yml to pass it.

* Fix outputs.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants