ICFY: migrate icfy-stats task from master branch to trunk #47620
Conversation
When calculating stats for a branch build and submitting them to ICFY via a webhook, one of the things that the task computes is the "ancestor" commit SHA, i.e., the point in the `trunk` branch from where the PR branch is created. This will now have to look at `trunk` instead of `master`. If we don't do this, bundle size diffs will be calculated against that last `master` commit instead of the `trunk` one. As `master` will be frozen and will stop moving forward, the diffs would be increasingly incorrect. As a drive-by fix, I'm changing the URL of the webhook endpoint. The `api.` prefix and the 5000 port have been optional for a long time now.
matticbot
added
the
[Status] Needs Review
|
Of course the |
| @@ -42,12 +42,12 @@ jobs: | |||
| env: | |||
| ICFY_SECRET: ${{ secrets.ICFY_SECRET }} | |||
| run: | | |||
| ANCESTOR_SHA1=$(git merge-base HEAD origin/master) | |||
| ANCESTOR_SHA1=$(git merge-base HEAD origin/trunk) | |||
There was a problem hiding this comment.
I have not tested it, but StackOverflow says you can use ${{ github.event.pull_request.base.ref }} to get the target of the PR. This will make ICFY works for branches targeting branches other than master/trunk
There was a problem hiding this comment.
@scinos It's a long time since I last looked at this in detail, but I think that github.event.pull_request.base.ref describes the tip of the base branch where we want to merge the PR -- master or another PR branch for stacked PRs. The tip is useful, for example, for detecting merge conflicts.
About the merge base, the git merge-base man page says:
Note that there can be more than one merge base for a pair of commits.
And shows this criss-cross merge as an example:
---1---o---A
\ /
X
/ \
---2---o---o---B
where 1 and 2 are equally good merge bases for A and B.
This is very unlikely to happen in our repo, as we don't use merges, and also the two histories need to be independent (we have a few of these after importing packages like wpcom from other repos), without a common ancestor.
But it's probably a reason why merge-base is not exposed by the GitHub API.
matticbot
removed
the
[Status] Needs Review
When calculating stats for a branch build and submitting them to ICFY via a webhook, one of the things that the task computes is the "ancestor" commit SHA, i.e., the point in the
trunkbranch from where the PR branch was created.This will now have to look at
trunkinstead ofmaster. If we don't do this, bundle size diffs will be calculated against the lastmastercommit instead of thetrunkone. Asmasterwill be frozen and will stop moving forward, the diffs would be increasingly incorrect.As a drive-by fix, I'm changing the URL of the webhook endpoint. The
api.prefix and the 5000 port have been optional for a long time now.Looking at #45234, this change is not there.