workflows/labels: fix merge conflict label#419836
workflows/labels: fix merge conflict label#419836wolfgangwalther merged 1 commit intoNixOS:masterfrom
Conversation
The previous implementation had two problems: - When switching from /search to /pulls, we disabled the additional GET on each single pull request - which causes no test merge commit creation for all PRs. This means, merge conflicts will not actually be detected. - By using `item` in the pull-request triggered case, this goes back to `context.payload.pull_request`, which is the state *at the beginning* of the workflow run. But this renders our "let's wait 3 minutes before checking merge_commit_sha" logic void. While we wait for 3 minutes, we still use the *old* value afterwards... Just making the extra request every time simplifies the logic and solves both problems.
nixpkgs-ci
bot
added
2.status: merge conflict
| const pull_request = item.head ? item : (await github.rest.pulls.get({ | ||
| const pull_request = (await github.rest.pulls.get({ |
There was a problem hiding this comment.
This also fires off an extra request while iterating through the allPulls items.
- Does the
github.rest.pulls.listrequest trigger creation of the merge commit, or do we still need agithub.rest.pulls.getto do that? - Is there a way to distinguish
allPullsitems from apull_requestcontext item?
If the answer to 1 is "listing PRs doesn't trigger a merge commit" then this is perfect.
If the answer is "listing PRs does trigger a merge commit, but there's no way to distinguish between allPulls items and an pull_request context item", then we should consider how we can refactor in a follow up PR.
Either way, this fixes the issue, so whether or not it is perfect in the allPulls case shouldn't block merging, assuming we have the headroom in our ratelimit (which we do).
There was a problem hiding this comment.
Does the
github.rest.pulls.listrequest trigger creation of the merge commit, or do we still need agithub.rest.pulls.getto do that?
No, pulls.list does not create it. We need the extra get here.
There was a problem hiding this comment.
assuming we have the headroom in our ratelimit (which we do).
Yes, we do. I just checked, and we currently use ~ 6k/hour for labeling. So still another 6k available. Works out nicely with a lot of room for more.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
wolfgangwalther
added
the
8.has: port to stable
2 participants
The previous implementation had two problems:
itemin the pull-request triggered case, this goes back tocontext.payload.pull_request, which is the state at the beginning of the workflow run. But this renders our "let's wait 3 minutes before checking merge_commit_sha" logic void. While we wait for 3 minutes, we still use the old value afterwards...Just making the extra request every time simplifies the logic and solves both problems.
Fixes #419654 (comment)
Things done
Add a 👍 reaction to pull requests you find important.