RFC: Untrusted pull request roles based on author's association #175
Conversation
ahal
force-pushed
the
github_author_association
branch
from
6405241 to
1bcefd5
Compare
There was a problem hiding this comment.
I like this proposal very much! I know the mobile teams would be pleased to let untrusted contributors run a subset of the jobs anyway.
The tiny nit I'd like to talk about is the name mixed. If someone new to TC reads a .taskcluster.yml file, they have no way to guess what this policy does without reading the docs. How about something like public_restricted or non_collarborators_restricted?
Good call, I like |
ahal
force-pushed
the
github_author_association
branch
from
1bcefd5 to
ea9c4ea
Compare
| be assumed. So far this behavior is identical to the `collaborators` policy. | ||
|
|
||
| However if the author is not a collaborator, a new role called `repo:github.com/${ | ||
| payload.organization }/${ payload.repository }:pull-request-untrusted` will be assumed instead. |
There was a problem hiding this comment.
I'm also unsure about the name pull-request-untrusted and would like to invite some bikeshedding here too. Other options: pull-request-public, pull-request-restricted, ??
I think I like "untrusted" because it's a good reminder to ci-config maintainers about which scopes should be granted to it. On the other hand, maybe "untrusted" will be confusing due to us using that term in CoT + levels already.
There was a problem hiding this comment.
At first I thought that it would be good to align pull request policy name public_RESTRICTED and the role ...:pull-request-UNTRUSTED, but I agree that for someone managing those roles, it would be easier to understand that those scopes should be checked carefully.
I guess we can leave it as is 👍
matt-boris
requested review from
a team,
lotas,
petemoore and
matt-boris
and removed request for
a team
|
Here's an example implementation of this RFC: taskcluster/taskcluster#5569 (Got a little ahead of myself, but happy to change it if things come up here). |
| To allow projects to tell whether a pull request was created by a collaborator or not, a new | ||
| `is_collaborator` field will be included in the JSON-e context used to evaluate the | ||
| `.taskcluster.yml` file. This field will be present when `tasks_for` is `github-pull-request` | ||
| (regardless of `pullRequest` policy), otherwise it will be undefined. |
There was a problem hiding this comment.
Actually I think I'd like to re-work this part bit. While going through the implementation I realized this is adding some unnecessary complexity and ambiguity.
Rather than an is_collaborator context value which only has meaning in some contexts, I'd like to make a new tasks_for value of github-pull-request-untrusted. I think this will result in both a cleaner implementation, as well as cleaner code in consumer's taskgraph transforms.
ahal
dismissed stale reviews from matt-boris, lotas, and JohanLorenzo
via
f92ab31
|
This will be landed on Friday this week during the Taskcluster Community meeting, unless there are any matters which come up that block it. Therefore please submit any final comments or raise objections before then. |
Also makes the title a bit more clear.
|
Latest push doesn't change the content, I had just put the wrong RFC number in it (didn't realize it was supposed to match the PR, nor that there was a script to generate it). |
|
I think it might good if new pushes didn't invalidate the reviews here :). The author can probably use their best judgement if a new change warrants a new review and re-request explicitly if it does. |
Rendered
Issue: #173