1539523 - [GitHub] Allow using the .taskcluster.yml file from the provisional merge commit rather than the PR HEAD

Status: NEW

(Taskcluster :: Services, enhancement)

Description

[James Graham [:jgraham]]

TaskCluster-GitHub reads the configuration from the .taskcluster.yml file on the HEAD of the PR branch. This is good and sensible except in one circumstance; if your tests are running on the provisional merge commit with the base branch (i.e. master usually) and the taskcluster config on master itself has changed to the extent that the .taskcluster.yml on the PR HEAD is no longer valid.

The choice of running the tests on the provisional merge commit rather than on the PR HEAD is rather common (it's the default behaviour on at least Travis), so it seems reasonable to allow for this use case. Updating the taskcluster configuration is also a fairly normal operation, and it's certainly not unusual to have brnaches that are based on an old master.

In terms of implementation, the simplest thing to use would be a property in the .yml file indicating which version should be used. That would likely involve re-fetching in the case that the merged version is selected, although I note that if one made this part of the policy key then no extra work would be required compared to today.

Comment 1

[Aki Sasaki (not active)]

Hm, now that I reread, I think you're asking for the opposite behavior. Sorry :)

original comment:

Agreed that this would be useful. Depending on how this is done, and for which repos, this could introduce security concerns. For example:

• a github repo has sensitive processes (e.g. access to taskcluster secrets) that only happen on merge or release tag
• the github repo also allows for taskcluster automation on PR, for everyone, not just contributors
• a random github user opens a PR that changes .taskcluster.yml so they can access the taskcluster secret and print it out into the log

We should probably only allow this type of behavior in certain circumstances, and warn about the possibility of leaking sensitive processes or secrets.

Comment 2

[James Graham [:jgraham]]

To be clear the behaviour I want is "(optionally) read the .taskcluster.yml data for PRs from refs/pull/${number}/merge rather than refs/pull/${number}/head". It's unclear to me that there are additional security concerns beyond those implied by supporting jobs on PRs for untrusted users in the first place.

Comment 3

[Simon Sapin (:SimonSapin)]

Servo is running into this as well: https://github.com/servo/taskcluster-config/pull/2#issuecomment-552069572

I was gonna file a bug asking to simply change the behavior, but an opt-in flag would work for us as well. Owlish, what do you think?

Comment 4

[Simon Sapin (:SimonSapin)]

… and again: https://github.com/servo/servo/issues/24846

Relevant code starts here: https://github.com/taskcluster/taskcluster/blob/b955ec0666b8855f85473fd790d9b944ce7c7a10/services/github/src/handlers.js#L551

  let sha = message.payload.details['event.head.sha'];

Owlish, how would you feel about a PR that separates this into two variables head_sha and merge_sha (for PR events), and passes the latter to this.getYml?

Comment 5

[[:owlish] 🦉 PST]

(In reply to Simon Sapin (:SimonSapin) from comment #4)

Owlish, how would you feel about a PR that separates this into two variables head_sha and merge_sha (for PR events), and passes the latter to this.getYml?

Did I understand you correctly you would like to make the PR? That would be fantastic!

Comment 6

[Simon Sapin (:SimonSapin)]

I did offer to make a PR, and was asking before I did if changing the default/only behavior sounds acceptable or desirable (since a previous comment mentioned an opt-in switch). However this was a while ago and it’s all very much "out of cache" in my head at this point. I don’t know when I would get around to it so don’t wait on me if anyone feels like working on this.