Uh oh!
There was an error while loading.Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork32k
Convertdoc.yml workflow to be reusable#103914
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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.
Already on GitHub?Sign in to your account
Convertdoc.yml workflow to be reusable#103914
Uh oh!
There was an error while loading.Please reload this page.
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@ambv I've added a few annotations below, hopefully they're helpful
| @@ -26,7 +26,7 @@ permissions: | |||
| contents: read | |||
| concurrency: | |||
| group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }} | |||
| group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}-reusable | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This collides with the “parent” workflow if it's exactly the same, which is why it was required to introduce some difference.
| Misc/** | ||
| .github/workflows/reusable-docs.yml | ||
| - name: Check for docs changes | ||
| if: >- |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
If this step is skipped, the job output above will fall back tofalse.
| @@ -35,6 +35,7 @@ jobs: | |||
| runs-on: ubuntu-latest | |||
| timeout-minutes: 10 | |||
| outputs: | |||
| run-docs: ${{ steps.docs-changes.outputs.run-docs || false }} | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The part after|| is only used as a fallback when the first part of the expression is empty (like when that step didn't even get executed).
| filter: | | ||
| Doc/** | ||
| Misc/** | ||
| .github/workflows/reusable-docs.yml |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This reimplements what was implemented throughpaths:.
| check-docs: | ||
| name: 📝 | ||
| needs: check_source | ||
| if: fromJSON(needs.check_source.outputs.run-docs) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
The job above sets eithertrue orfalse as strings, parsing that as JSON allows not having equality checks.
Uh oh!
There was an error while loading.Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I made the workflow file name prefixed withreusable- to start a convention that would help distinguish standalone workflows from the “inclusion snippets”.
| on: | ||
| workflow_call: | ||
| workflow_dispatch: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I kept this to make it possible to still trigger just this workflow separately, via a manual request.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
@hugovk this is the only bit I wasn't sure about — is it useful to keep this trigger at all. If not, deleting it could improve maintainability..
.github/workflows/reusable-docs.yml Outdated
| #push: | ||
| # branches: | ||
| # - 'main' | ||
| # - '3.11' | ||
| # - '3.10' | ||
| # - '3.9' | ||
| # - '3.8' | ||
| # - '3.7' | ||
| # paths: | ||
| # - 'Doc/**' | ||
| pull_request: | ||
| branches: | ||
| - 'main' | ||
| - '3.11' | ||
| - '3.10' | ||
| - '3.9' | ||
| - '3.8' | ||
| - '3.7' | ||
| paths: | ||
| - 'Doc/**' | ||
| - 'Misc/**' | ||
| - '.github/workflows/doc.yml' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
I reimplemented the filtering on the calling side. Dropping thepull_request trigger means that it won't run standalone on PRs, but it will run via inclusion into the main build workflow.
| on: | ||
| workflow_call: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others.Learn more.
This bit is what allows to “include” this workflow into another.
cbf7985 to3f8d6d2CompareUh oh!
There was an error while loading.Please reload this page.
Uh oh!
There was an error while loading.Please reload this page.
bedevere-bot commentedApr 30, 2023
A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated. Once you have made the requested changes, please leave a comment on this pull request containing the phrase |
ghost commentedMay 2, 2023 • edited by ghost
Loading Uh oh!
There was an error while loading.Please reload this page.
edited by ghost
Uh oh!
There was an error while loading.Please reload this page.
All commit authors signed the Contributor License Agreement. |
bedevere-bot commentedMay 2, 2023
Thanks for making the requested changes! @hugovk: please review the changes made to this pull request. |
How far back do we want to backport this (and its parent#97533), if at all? |
@hugovk are there huge differences? I think it's okay to backport all the way back to 3.7, if the difference isn't big. Even if it is, I'm willing to explore solving all the cherry-picking conflicts, if necessary. |
As for the "parent PR" I envision that@ambv would want it fully backported because he wanted to make auto-merge painless... |
The general policy is to only backport non-security things for bugfix branches (i.e. 3.11), but CI changes can usually go further back because we need to test older security branches (i.e. 3.7-3.10) when we do get security fixes, and keeping them in sync makes backporting easier. https://devguide.python.org/versions/ So we're probably fine backporting to 3.7, and we can always check with the release managers. |
Yep, that was my thinking too. |
I've implemented a similar change in pypa/build recently, it seems to work well so far:pypa/build@04df94c. |
1c7f95d to0a04207CompareThis is necessary because paths with whitespaces tend to crash saidaction[[1]][[2]][[3]]. Also, we don't need to use JSON as it's harderto parse while the value isn't used except for the emptiness check.The change fixes [[4]][1]:https://github.com/Ana06/get-changed-files#get-all-changed-files-as-space-delimited[2]:python#103914 (comment)[3]:python#103914 (comment)[4]:python#103914 (comment)
This is necessary because paths with whitespaces tend to crash saidaction[[1]][[2]][[3]]. Also, we don't need to use JSON as it's harderto parse while the value isn't used except for the emptiness check.The change fixes [[4]][1]:https://github.com/Ana06/get-changed-files#get-all-changed-files-as-space-delimited[2]:python#103914 (comment)[3]:python#103914 (comment)[4]:python#103914 (comment)
Thanks@webknjaz for the PR, and@pradyunsg for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12. |
Thanks@webknjaz for the PR, and@pradyunsg for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11. |
Sorry,@webknjaz and@pradyunsg, I could not cleanly backport this to |
Sorry@webknjaz and@pradyunsg, I had trouble checking out the |
Thanks@webknjaz for the PR, and@pradyunsg for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10. |
Sorry,@webknjaz and@pradyunsg, I could not cleanly backport this to |
Thanks@webknjaz for the PR, and@pradyunsg for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8. |
Sorry@webknjaz and@pradyunsg, I had trouble checking out the |
Thanks@webknjaz for the PR, and@pradyunsg for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9. |
Sorry,@webknjaz and@pradyunsg, I could not cleanly backport this to |
Co-authored-by: Adam Turner <9087854+AA-Turner@users.noreply.github.com>Co-authored-by: Hugo van Kemenade <hugovk@users.noreply.github.com>(cherry picked from commit88d14da)Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
bedevere-bot commentedJul 22, 2023
GH-107042 is a backport of this pull request to the3.12 branch. |
bedevere-bot commentedJul 22, 2023
GH-107043 is a backport of this pull request to the3.11 branch. |
This patch is meant to simplify the maintenance of multiple complex workflow definitions that are tied together into a single workflow later on.
It is separated out of#97533 for atomicity.