FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Convert `doc.yml` workflow to be reusable by webknjaz · Pull Request #103914 · python/cpython · GitHub

/ cpython Public

Convert doc.yml workflow to be reusable - #103914

Merged
pradyunsg merged 2 commits into
python:mainfrom
webknjaz:maintenance/gha-reusable-docs-workflow
May 28, 2023
Merged

Convert doc.yml workflow to be reusable#103914
pradyunsg merged 2 commits into
python:mainfrom
webknjaz:maintenance/gha-reusable-docs-workflow

Conversation

Copy link
Copy Markdown
Member

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.

webknjaz left a comment

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@ambv I've added a few annotations below, hopefully they're helpful


concurrency:
group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}
group: ${{ github.workflow }}-${{ github.head_ref || github.run_id }}-reusable

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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: >-

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

If this step is skipped, the job output above will fall back to false.

runs-on: ubuntu-latest
timeout-minutes: 10
outputs:
run-docs: ${{ steps.docs-changes.outputs.run-docs || false }}

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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).

Comment on lines +69 to +91
filter: |
Doc/**
Misc/**
.github/workflows/reusable-docs.yml

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This reimplements what was implemented through paths:.

check-docs:
name: 📝
needs: check_source
if: fromJSON(needs.check_source.outputs.run-docs)

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

The job above sets either true or false as strings, parsing that as JSON allows not having equality checks.

Comment thread .github/workflows/reusable-docs.yml Outdated

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I made the workflow file name prefixed with reusable- to start a convention that would help distinguish standalone workflows from the “inclusion snippets”.


on:
workflow_call:
workflow_dispatch:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I kept this to make it possible to still trigger just this workflow separately, via a manual request.

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

@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..

Comment on lines -5 to -28
#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'

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I reimplemented the filtering on the calling side. Dropping the pull_request trigger means that it won't run standalone on PRs, but it will run via inclusion into the main build workflow.

name: 📝

on:
workflow_call:

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

This bit is what allows to “include” this workflow into another.

webknjaz force-pushed the maintenance/gha-reusable-docs-workflow branch 3 times, most recently from cbf7985 to 3f8d6d2 Compare April 27, 2023 22:23
hugovk previously requested changes Apr 30, 2023
Comment thread .github/workflows/reusable-docs.yml Outdated
Comment thread .github/workflows/build.yml Outdated

Copy link
Copy Markdown

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 I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

ghost commented May 2, 2023
edited by ghost
Loading

Copy link
Copy Markdown

All commit authors signed the Contributor License Agreement.

webknjaz requested a review from hugovk May 2, 2023 15:47

webknjaz commented May 2, 2023

Copy link
Copy Markdown
Member Author

I have made the requested changes; please review again.

cc @ambv @hugovk

Copy link
Copy Markdown

Thanks for making the requested changes!

@hugovk: please review the changes made to this pull request.

hugovk commented May 10, 2023

Copy link
Copy Markdown
Member

How far back do we want to backport this (and its parent #97533), if at all?

Copy link
Copy Markdown
Member Author

@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.

Copy link
Copy Markdown
Member Author

As for the "parent PR" I envision that @ambv would want it fully backported because he wanted to make auto-merge painless...

hugovk commented May 11, 2023

Copy link
Copy Markdown
Member

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.

Copy link
Copy Markdown
Member Author

Yep, that was my thinking too.

Copy link
Copy Markdown
Member Author

I've implemented a similar change in pypa/build recently, it seems to work well so far: pypa/build@04df94c.

webknjaz force-pushed the maintenance/gha-reusable-docs-workflow branch 2 times, most recently from 1c7f95d to 0a04207 Compare May 24, 2023 16:50
hugovk dismissed their stale review May 24, 2023 18:03

Changes made

Copy link
Copy Markdown
Member Author

@hugovk this should fix the problem: #105151.

hugovk pushed a commit to hugovk/cpython that referenced this pull request Jun 1, 2023
This is necessary because paths with whitespaces tend to crash said
action[[1]][[2]][[3]]. Also, we don't need to use JSON as it's harder
to 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)
hugovk pushed a commit to hugovk/cpython that referenced this pull request Jun 1, 2023
This is necessary because paths with whitespaces tend to crash said
action[[1]][[2]][[3]]. Also, we don't need to use JSON as it's harder
to 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)
pablogsal added needs backport to 3.11 only security fixes needs backport to 3.12 only security fixes labels Jul 22, 2023

Copy link
Copy Markdown
Contributor

Thanks @webknjaz for the PR, and @pradyunsg for merging it 🌮🎉.. I'm working now to backport this PR to: 3.12.
🐍🍒⛏🤖

Copy link
Copy Markdown
Contributor

Thanks @webknjaz for the PR, and @pradyunsg for merging it 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

Copy link
Copy Markdown
Contributor

Sorry, @webknjaz and @pradyunsg, I could not cleanly backport this to 3.11 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 88d14da76f579fe014cbd7c15e42be4234135fe9 3.11

Copy link
Copy Markdown
Contributor

Sorry @webknjaz and @pradyunsg, I had trouble checking out the 3.12 backport branch.
Please retry by removing and re-adding the "needs backport to 3.12" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker 88d14da76f579fe014cbd7c15e42be4234135fe9 3.12

pablogsal added the needs backport to 3.10 only security fixes label Jul 22, 2023

Copy link
Copy Markdown
Contributor

Thanks @webknjaz for the PR, and @pradyunsg for merging it 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

Copy link
Copy Markdown
Contributor

Sorry, @webknjaz and @pradyunsg, I could not cleanly backport this to 3.10 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 88d14da76f579fe014cbd7c15e42be4234135fe9 3.10

Copy link
Copy Markdown
Contributor

Thanks @webknjaz for the PR, and @pradyunsg for merging it 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖 I'm not a witch! I'm not a witch!

Copy link
Copy Markdown
Contributor

Sorry @webknjaz and @pradyunsg, I had trouble checking out the 3.8 backport branch.
Please retry by removing and re-adding the "needs backport to 3.8" label.
Alternatively, you can backport using cherry_picker on the command line.
cherry_picker 88d14da76f579fe014cbd7c15e42be4234135fe9 3.8

Copy link
Copy Markdown
Contributor

Thanks @webknjaz for the PR, and @pradyunsg for merging it 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

Copy link
Copy Markdown
Contributor

Sorry, @webknjaz and @pradyunsg, I could not cleanly backport this to 3.9 due to a conflict.
Please backport using cherry_picker on command line.
cherry_picker 88d14da76f579fe014cbd7c15e42be4234135fe9 3.9

webknjaz added a commit to webknjaz/cpython that referenced this pull request Jul 22, 2023
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 commit 88d14da)

Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>

Copy link
Copy Markdown

GH-107042 is a backport of this pull request to the 3.12 branch.

Copy link
Copy Markdown

GH-107043 is a backport of this pull request to the 3.11 branch.

ambv pushed a commit that referenced this pull request Jul 23, 2023
…) (#107043)

Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
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 commit 88d14da)
(cherry picked from commit eaa6702)
ambv pushed a commit that referenced this pull request Jul 23, 2023
…) (#107042)

Co-authored-by: Sviatoslav Sydorenko <wk.cvs.github@sydorenko.org.ua>
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 commit 88d14da)
(cherry picked from commit eaa6702)
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
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.

8 participants


Back | FazBrowse Home | New Git URL