Repository navigation
fix(decision): use the merge base as base_rev for GitHub pushes and pull requests (bug 2072920) - #1100
fix(decision): use the merge base as base_rev for GitHub pushes and pull requests (bug 2072920)#1100ahochheiden wants to merge 1 commit into
Conversation
d1a27ff to
2c05b64
Compare
…ull requests (bug 2072920) GitHub reports the wrong base for a new branch (the null revision), a force push (the orphaned old tip) and a pull request (the current tip of its target), so `files_changed` lists files the push never touched. The Decision Task now uses the merge base of the head and the target branch, which for a push is the default branch. A force push to the default branch uses the merge base of its old and new tips. A fast forward push, or one whose merge base cannot be found, keeps its reported base. GitHub's compare API provides the merge base, authenticated with `GITHUB_TOKEN` when it is set. When the API fails, and for other hosts, git finds it instead. Refs taskcluster#808
2c05b64 to
6e3c436
Compare
This begs the question: if we can find what we need with git, what benefit does the github API provide? |
Speed. The compare API took 1-2 seconds in my testing, and git alone 8-26 seconds, depending on how far back the branch forked (which means 26 seconds is not an upper bound). Since this is in the decision task, I think speed matters even more than usual. |
| raise _GitHubCompareError(f"{repository} is not on GitHub") | ||
| headers = {"Accept": "application/vnd.github+json"} | ||
| if token := os.environ.get("GITHUB_TOKEN"): | ||
| headers["Authorization"] = f"Bearer {token}" |
There was a problem hiding this comment.
I can say from experience that this will hit rate limits. I know this falls back to using the vcs, but if we don't solve it up-front, we'll just need to make a second release + vendor patch. So might as well solve it now.
Luckily, there's an auth.githubRepoToken endpoint you can call where Taskcluster will return a Github app installation token. Here's an example you can copy from (the taskcluster package is already a dependency):
mozilla-releng/scriptworker@9cf3117#diff-5ecedb756939ecdd7b53ac91bbee8a94a965afa1cb63c55895a57537f771f696R80-R84
The app name should be configurable in taskcluster/config.yml, but in practice for fxci, it's going to be "read"
There was a problem hiding this comment.
Alternatively, we could make run-task be the thing that calls this API and sets GITHUB_TOKEN accordingly.. In which case this PR wouldn't need any changes. I think we might need to implement this ability into run-task anyway in order to clone private repos..
Downsides to using run-task are that we wouldn't have the taskcluster package (would need to make request with urllib) and there wouldn't be a convenient place to configure the Github app name (would need to be passed in via env or cli flag).
But if it's going to be needed anyway.. then we can avoid doing it here.
| if not base_ref: | ||
| return None | ||
| branch_ref = f"refs/heads/{base_ref.removeprefix('refs/heads/')}" | ||
| return self._try_derive_base_rev( |
There was a problem hiding this comment.
Since this just ends up calling the same function as derive_base_rev but with different args, let's merge the two together to reduce the API surface. Callers can control the behaviour they want by passing in the appropriate args
| base_repository, head_rev, branch_ref=branch_ref | ||
| ) | ||
|
|
||
| def _try_derive_base_rev( |
There was a problem hiding this comment.
With the above comment fixed, there would be no need for this intermediate function and the try/except can be moved to derive_base_rev.
| repository = remote | ||
| timeout = self._timeout() | ||
| try: | ||
| self.run("fetch", *options, repository, refspec, timeout=timeout) |
There was a problem hiding this comment.
Hmm, if we're going to be doing fetches.. this makes me lean towards implementing this into run-task, as currently run-task's purview is making sure that the local clone is in a ready state for the task.
There was a problem hiding this comment.
But before jumping too far ahead, why do we need to fetch the base rev? Would it be possible to use the Github API to fetch the actual files changed so there's no need for the base? (Possibly needing GraphQL)
Or is there no API for that?
There was a problem hiding this comment.
Looks like they're included in the response of the compare API, but capped at 300 files, which we'd definitely exceed in some cases.
|
Here's my stab at implementing this in (see the first PR in the stack for the Github auth implementation) Couple deviations from this one:
|
GitHub reports the wrong base for a new branch (the null revision), a force push (the orphaned old tip) and a pull request (the current tip of its target), so
files_changedlists files the push never touched.The Decision Task now uses the merge base of the head and the target branch, which for a push is the default branch. A force push to the default branch uses the merge base of its old and new tips. A fast forward push, or one whose merge base cannot be found, keeps its reported base.
GitHub's compare API provides the merge base, authenticated with
GITHUB_TOKENwhen it is set. When the API fails, and for other hosts, git finds it instead.Firefox needs a GITHUB_TOKEN in its Decision Tasks for authenticated compare requests, which needs a new scope in fxci-config.
Refs #808