Proposes running big GitHub Actions only when PRs are ready for review. #32018
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
|
Size Change: 0 B Total Size: 2.87 MB ℹ️ View Unchanged
|
vcanales
changed the title
gziolo
reviewed
May 21, 2021
There was a problem hiding this comment.
That's a very nice idea. I agree that we should explore better handling for PRs in the draft mode. I think we can be more aggressive in preventing more workflows from running like:
- Compressed Size
- Build Gutenberg Plugin Zip
Is it possible to use labels as triggers so repository members could apply them manually to start additional workflows without converting PR from draft? I guess it's something less important that we can investigate later.
| @@ -2,6 +2,9 @@ name: React Native E2E Tests (Android) | |||
|
|
|||
| on: | |||
| pull_request: | |||
| types: [review_requested, ready_for_review] | |||
| paths: | |||
There was a problem hiding this comment.
I don't think that we should use paths here and for iOS, because React Native editor shares code with Gutenberg web.
ocean90
reviewed
May 21, 2021
.github/workflows/end2end-test.yml
Outdated
| @@ -2,6 +2,7 @@ name: End-to-End Tests | |||
|
|
|||
| on: | |||
| pull_request: | |||
| types: [review_requested, ready_for_review] | |||
There was a problem hiding this comment.
With that the test wouldn't run for further pushes on the PR, for example after a review. You'd have to request a review again which may be a workaround but might get a bit spammy.
Maybe add also synchronize but check for github.event.pull_request.draft like done here? This would not directly skip the workflow but still would be faster than running the full workflow.
There was a problem hiding this comment.
I just tried setting this PR back to draft, pushed something new, and it doesn't seem to stop checks from re-running. A Request for Changes here would be appreciated in order to check that case!
Maybe add also
synchronizebut check forgithub.event.pull_request.draftlike done here? This would not directly skip the workflow but still would be faster than running the full workflow.
Checking for draft, in my opinion, is not far enough to save us run time, as most PRs are not Drafts (139 out of 687 as of this comment.) I'll add synchronize to keep testing!
vcanales
force-pushed
the
test/workflow-changes
branch
from
4c0e856 to
07b5d16
Compare
desrosj
reviewed
Jul 2, 2021
| @@ -6,6 +6,7 @@ name: Pull request automation | |||
|
|
|||
| jobs: | |||
| pull-request-automation: | |||
| timeout-minutes: 10 | |||
There was a problem hiding this comment.
Does this ever reach this limit? I'm not sure that I have ever seen this workflow take more than 10 seconds.
There was a problem hiding this comment.
This would have been helpful during the last GitHub Actions outage; this process in particular was running for hours in some cases. Now, I'm not certain that GHA would actually timeout the process in the context of an outage, but I think it's worth a try.
There was a problem hiding this comment.
An example of this happening; this particular job ran for 1h 2m, on the day of a big GitHub Actions outage.
There was a problem hiding this comment.
Got it! Seems like a good idea. If GHA is having an outage, I wonder if that limit would even work? I guess it depends on the outage itself.
Another option could be to add the concurrency option to this workflow. That would just cancel all other preceding workflows for the same branch. I don't know how the code is structured within packages/project-management-automation though. So I can't say whether it's essential that it runs for every push event.
|
I'm not yet convinced that this is the best way to save time and reduce the chance for a backlog of jobs to occur. In general, I am pro running workflows for each commit/push so long as they are scoped properly, and earlier runs are cancelled when appropriate (fixed in #32016). Since #32016 was merged, we have not experienced a backlog that has resulted in any significant disruption. Some things I would like to try and compare results for before merging this:
|
vcanales
requested review from
adamsilverstein,
adamziel,
ajitbohra,
atimmer,
cameronvoell,
chrisvanpatten,
dmsnell,
draganescu,
ellatrix,
getdave,
guarani,
kevin940726 and
mkaz
as code owners
vcanales
requested review from
oandregal,
ntwb,
spacedmonkey,
swissspidy,
talldan,
tellthemachines and
TimothyBJacobs
as code owners
* Use a consistent cache key for NPM packages across all workflows. Also, include the NodeJS version in the cache key in case different package versions are required. * Update the `actions/cache` action to the latest version. * Remove the strategy matrix from jobs with a single NodeJS version. For some reason, GitHub Actions will attach matrix values to job names in parenthesis. Because required checks are configured by job name, these need to stay consistent.
vcanales
force-pushed
the
test/workflow-changes
branch
from
7873eef to
a53f2b2
Compare
ockham
reviewed
Jul 7, 2021
Comment on lines
-21
to
-44
| compute-stable-branches: | ||
| name: Compute current and next stable release branches | ||
| runs-on: ubuntu-latest | ||
| outputs: | ||
| current_stable_branch: ${{ steps.get_branches.outputs.current_stable_branch }} | ||
| next_stable_branch: ${{ steps.get_branches.outputs.next_stable_branch }} | ||
|
|
||
| steps: | ||
| - name: Get current and next stable release branches | ||
| id: get_branches | ||
| run: | | ||
| curl \ | ||
| -H "Accept: application/vnd.github.v3+json" \ | ||
| -o latest.json \ | ||
| "https://api.github.com/repos/${{ github.repository }}/releases/latest" | ||
| LATEST_STABLE_TAG=$(jq --raw-output '.tag_name' latest.json) | ||
| IFS='.' read LATEST_STABLE_MAJOR LATEST_STABLE_MINOR LATEST_STABLE_PATCH <<< "${LATEST_STABLE_TAG#v}" | ||
| echo "::set-output name=current_stable_branch::release/${LATEST_STABLE_MAJOR}.${LATEST_STABLE_MINOR}" | ||
| if [[ ${LATEST_STABLE_MINOR} == "9" ]]; then | ||
| echo "::set-output name=next_stable_branch::release/$((LATEST_STABLE_MAJOR + 1)).0" | ||
| else | ||
| echo "::set-output name=next_stable_branch::release/${LATEST_STABLE_MAJOR}.$((LATEST_STABLE_MINOR + 1))" | ||
| fi | ||
|
|
There was a problem hiding this comment.
Removing this would completely break our release tooling. (Same for a number of other changes below.) I'm guessing this wasn't deliberate but the result of a rebase that didn't go quite right? 😅
There was a problem hiding this comment.
100% — this branch was quite behind, I'll put everything back 😅
guarani
removed request for
spacedmonkey,
oandregal,
noisysocks,
talldan,
swissspidy,
ntwb,
guarani,
TimothyBJacobs,
noahtallen and
tellthemachines
|
Closing as stale, and fixed somewhere else. |
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.
Description
The recent GitHub Actions service degradation incident surfaced some quick wins that we can implement in order to save some computing time:
Static analysis, unit tests, etc. take less time and we might benefit from setting timeouts for those as well.
Other alternative
We could add more granularity by using if conditions in combination with Events.
How has this been tested?
Screenshots
Admin e2e tests and Performance Tests remain in "Expected" status while the PR is still a Draft.
Mobile e2e tests don't show up on the list.