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

Allow triggering try workfows using labels #30383

Merged
merged 1 commit into from
Oct 23, 2023
Merged

Conversation

mrobinson
Copy link
Member

This change adds and alternate method for triggering try changes.
Instead of comments, changes are triggered via applying labels to pull
requests. The action will remove the label from the request and start
the requested jobs.

This will require creating at least a few labels:

  • T-full
  • T-linux-wpt-2013
  • T-linux-wpt-2020
  • T-macos
  • T-windows

More labels can be added as we support more configurations.

The good thing about this change is that try jobs against the actual
branch in the pull request instead of the master branch. This means
that changes to CI can be tested (unlike for comment processing).

One bit caveat with this change is that when adding multiple labels, a
CI job is triggered for each. Only one real build will run for each
label, but whether or more try jobs is triggered is a race condition.
The first CI job to successfully remove the label will actually trigger
the job. If the same job removes two compatible labels, then they can
share a build (for instance two types of WPT linux jobs). If not there
will be two. Note that this is at least as efficient as the current
behavior.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes do not require tests because they add a new CI configuration.

@sagudev
Copy link
Member

sagudev commented Sep 18, 2023

That gives me idea for #30144 we could detect A-content/webgpu label and always run _webgpu suite and normal suite on PR with this label.

@mrobinson mrobinson changed the title Allow trigger try workfows using labels Allow triggering try workfows using labels Sep 20, 2023
@mrobinson mrobinson added T-linux-wpt-2013 Do a try run of the WPT (legacy layout) T-linux-wpt-2020 Do a try run of the WPT labels Sep 20, 2023
@mrobinson mrobinson added T-linux-wpt-2013 Do a try run of the WPT (legacy layout) and removed T-linux-wpt-2013 Do a try run of the WPT (legacy layout) T-linux-wpt-2020 Do a try run of the WPT labels Sep 20, 2023
@mrobinson mrobinson added T-linux-wpt-2013 Do a try run of the WPT (legacy layout) and removed T-linux-wpt-2013 Do a try run of the WPT (legacy layout) labels Sep 20, 2023
@mrobinson mrobinson added the T-linux-wpt-2020 Do a try run of the WPT label Sep 20, 2023
@mrobinson
Copy link
Member Author

It seems there are some permissions issues that prevent actions for PRs from other forks from modifying the labels applied to a PR. Adding the permissions key to the action doesn't seem to be helping. I'll need to investigate this further.

@sagudev
Copy link
Member

sagudev commented Sep 21, 2023

IIRC apart from adding permission key to workflow file you also need to change repo settings so that actions get r+w permission. I noticed this when working on servo/servo-build-deps#3

@mukilan
Copy link
Member

mukilan commented Sep 21, 2023

I think in order to modify labels of the PR from forks, we'll need to use the workflow trigger 'pull_request_target' instead of 'pull_request'

From the docs:

This event allows your workflow to do things like label or comment on pull requests from forks. Avoid using this event if you need to build or run code from the pull request.

I guess this also means we won't be able to use this to test CI changes in a PR.

@mrobinson
Copy link
Member Author

I guess this also means we won't be able to use this to test CI changes in a PR.

Unfortunately that's what I feared. Even if we cannot test CI changes I guess this offers some other benefits over the comment path.

@mrobinson
Copy link
Member Author

Hrm. I think the right thing to do here is to create a fine-grained access token that only allows changing the labels on pull requests. The alternative is to build and run code from PRs with full permissions.

@mrobinson mrobinson force-pushed the try-label branch 2 times, most recently from 821ba5a to b4e11ae Compare October 3, 2023 14:09
@mrobinson mrobinson added T-linux-wpt-2013 Do a try run of the WPT (legacy layout) and removed T-linux-wpt-2013 Do a try run of the WPT (legacy layout) T-linux-wpt-2020 Do a try run of the WPT labels Oct 3, 2023
@mrobinson mrobinson force-pushed the try-label branch 2 times, most recently from 42f5ee9 to b97e8eb Compare October 20, 2023 06:14
@mrobinson mrobinson removed the T-linux-wpt-2013 Do a try run of the WPT (legacy layout) label Oct 20, 2023
@mrobinson
Copy link
Member Author

This is ready to review. I'm happy to walk through this with someone. I am testing this on my own git repository because with the pull_request_target action the workflow files are taken from the main branch.

@mrobinson mrobinson marked this pull request as ready for review October 23, 2023 10:51
This change adds and alternate method for triggering try changes.
Instead of comments, changes are triggered via applying labels to pull
requests. The action will remove the label from the request and start
the requested jobs.

This will require creating at least a few labels:

 - T-full
 - T-linux-wpt-2013
 - T-linux-wpt-2020
 - T-macos
 - T-windows

More labels can be added as we support more configurations.

The good thing about this change is that try jobs against the actual
branch in the pull request instead of the master branch. This means
that changes to CI can be tested (unlike for comment processing).

One bit caveat with this change is that when adding multiple labels, a
CI job is triggered for each. Only one real build will run for each
label, but whether or more try jobs is triggered is a race condition.
The first CI job to successfully remove the label will actually trigger
the job. If the same job removes two compatible labels, then they can
share a build (for instance two types of WPT linux jobs). If not there
will be two. Note that this is at least as efficient as the current
behavior.
@mrobinson mrobinson added this pull request to the merge queue Oct 23, 2023
Merged via the queue into servo:master with commit fdcbe61 Oct 23, 2023
10 checks passed
@mrobinson mrobinson deleted the try-label branch October 23, 2023 13:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants