Skip earlier when run on forks #11212
Merged
Conversation
This file contains 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
jeffwidman
commented
Jan 3, 2025
|
It worked! 🎉 
|
jeffwidman
force-pushed
the
skip-earlier-when-run-on-forks
branch
2 times, most recently
from
0462149 to
2668296
Compare
|
Once this is merged, I'll need to double-check that when run against a fork via |
thavaahariharangit
approved these changes
Jan 3, 2025
I found the original code confusing because it _always_ checks if the PR is approved, even when run on forks. Yet the job still bails if run on a fork, _even if the PR is approved_. So I pulled the `if` conditional that bypasses forks up into the first job. If that results in skipping the `approval` job, then the subsequent job that `needs: approval` will also be skipped ([docs](https://docs.github.com/en/actions/writing-workflows/workflow-syntax-for-github-actions#jobsjob_idneeds)). So this should hopefully be both more readable within the code, more readable on PR's, and also more performant. 🥂 Originally, I wasn't 100% sure this would work... perhaps there was a reason why the fork-check conditional needed to be attached to the second job. However, when I checked `git blame` I see that originally this was a single job, and then split in two: * dependabot#8651 And looking at the commit history on that PR, I think these two commits indicate the author was focused on simplifying two steps into one, so didn't consider the possibility of skipping earlier: * dependabot@ede644b * dependabot@c7069e6
jeffwidman
force-pushed
the
skip-earlier-when-run-on-forks
branch
from
2668296 to
201c600
Compare
|
I discussed this with @landongrindheim and @pavera and this change seemed sensible to them as well, so 🚢 |
landongrindheim
approved these changes
Jan 3, 2025
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What are you trying to accomplish?
I found the original code confusing because it always checks if the PR is approved, even when run on forks. Yet the job still bails if run on a fork, even if the PR is approved.
So I pulled the
ifconditional that bypasses forks up into the first job. This works because if theapprovaljob gets skipped, then the subsequent job will also be skipped since itneeds: approval(docs).So this should hopefully be both more readable within the code, more readable in CI output on PR's, and also more performant. 🥂
Anything you want to highlight for special attention from reviewers?
Originally, I wasn't 100% sure this works... perhaps there was a reason why the fork-check conditional needed to be attached to the second job.
However, when I checked
git blameI see that originally this was a single job, and then split in two:And looking at the commit history on that PR, I think these two commits indicate the author was focused on simplifying two steps into one, so didn't consider the possibility of skipping earlier:
In order to test this, I've opened this PR from my personal fork, rather than the main Dependabot repo...