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

Move change detection to separate workflow in CI #122336

Merged

Conversation

webknjaz
Copy link
Contributor

This is a continuation of the CI refactoring PR series I've been doing.

@webknjaz webknjaz force-pushed the maintenance/gha-change-detection-reusable branch from 7bd7ef5 to c994611 Compare July 26, 2024 23:54
@webknjaz webknjaz force-pushed the maintenance/gha-change-detection-reusable branch from c994611 to 2e5131d Compare July 27, 2024 00:10
@webknjaz webknjaz marked this pull request as ready for review July 27, 2024 00:13
@hugovk
Copy link
Member

hugovk commented Jul 29, 2024

It was already in its own job:

image

This PR:

image

Would it be more accurate to say this moves the job to its own workflow + file?

Benefits:

  • Makes the top-level build.yml shorter.
  • Helps make each bit more modular, separating config.
  • Are there other benefits of this change?

Costs:

  • This PR does add a net 46 lines of config, mainly passing the outputs from the new job out through the new workflow, though it's not too complex.
  • We get another "dummy" reusable workflow showing in the left menu, with no workflows shown when clicking (although the pinning does help find the important ones, thanks for letting me know about pinning):

image

@webknjaz
Copy link
Contributor Author

webknjaz commented Jul 29, 2024

Would it be more accurate to say this moves the job to its own workflow + file?

Yes, you're right. I must've phrased it better.

Benefits:

  • Makes the top-level build.yml shorter.

  • Helps make each bit more modular, separating config.

  • Are there other benefits of this change?

I think these benefits have a significant impact on their own. This also contributes to the separation of the structure / triggers vs. implementation details. In the future, it makes it cleaner to add more jobs (or separate what's in there already) if the need be. I feel like it currently computes several things that aren't necessarily related enough to be in the same job.
My idea is that the top-level workflow structure should be in the main configuration file, and the components would be separate, only interfacing through their inputs/outputs.
It's basically an abstraction layer separation exercise.
Having such predictable structure will also enable us to introduce linting that would keep it consistent and easy to reason about.

Costs:

  • This PR does add a net 46 lines of config, mainly passing the outputs from the new job out through the new workflow, though it's not too complex.

  • We get another "dummy" reusable workflow showing in the left menu, with no workflows shown when clicking (although the pinning does help find the important ones, thanks for letting me know about pinning):

Yes, you're right. I suppose, we could add names to those reusable workflows with something along the lines of “Don't click”. Combined with pinning the entry-point workflows, that would make it look nicer. Personally, I'm not too worried about these, but I see how some of the points may annoy people.

Additionally, it may make sense to hit GitHub support with a request to hide workflows that only have a workflow_call trigger, since it's known that those are never going to have any runs listed.

P.S. Regarding having a nested entry in the sidebar — I don't expect that many people would need to click on that, so it'll remain collapsed and green most of the time, not really affecting anyone on the scale.

@webknjaz
Copy link
Contributor Author

@picnixz this also needs backport labels FYI

@hugovk hugovk added needs backport to 3.12 bug and security fixes needs backport to 3.13 bugs and security fixes labels Jul 29, 2024
@hugovk hugovk changed the title Move change detection to separate job in CI Move change detection to separate workflow in CI Jul 31, 2024
@hugovk
Copy link
Member

hugovk commented Jul 31, 2024

Yes, I agree on balance this makes it things a bit clearer. Thanks!

@hugovk hugovk merged commit e60ee11 into python:main Jul 31, 2024
40 checks passed
@miss-islington-app
Copy link

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

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 31, 2024
(cherry picked from commit e60ee11)

Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk@sydorenko.org.ua>
@miss-islington-app
Copy link

Sorry, @webknjaz and @hugovk, I could not cleanly backport this to 3.12 due to a conflict.
Please backport using cherry_picker on command line.

cherry_picker e60ee11cb51b87deeb22ad125717bd0d0dc10fa8 3.12

@bedevere-app
Copy link

bedevere-app bot commented Jul 31, 2024

GH-122510 is a backport of this pull request to the 3.13 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.13 bugs and security fixes label Jul 31, 2024
hugovk pushed a commit that referenced this pull request Jul 31, 2024
…122510)

Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk@sydorenko.org.ua>
webknjaz added a commit to webknjaz/cpython that referenced this pull request Jul 31, 2024
).

(cherry picked from commit e60ee11)

Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <wk@sydorenko.org.ua>
@bedevere-app
Copy link

bedevere-app bot commented Jul 31, 2024

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

blhsing pushed a commit to blhsing/cpython that referenced this pull request Aug 22, 2024
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.

3 participants