CI: Merge open blocker PRs in all CI workflows + other improvements #36338
- Loading branch information
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,28 @@ | ||
| #!/bin/sh | ||
| # Merge open PRs from sagemath/sage labeled "blocker". | ||
| GH="gh -R sagemath/sage" | ||
| PRs="$($GH pr list --label "p: blocker / 1" --json number --jq '.[].number')" | ||
|
Contributor
There was a problem hiding this comment. You probably want to make sure that they also have the "postive review" label.
Contributor
Author
There was a problem hiding this comment. Definitely not. Sometimes it takes a while to find a competent reviewer.
Collaborator
There was a problem hiding this comment. See #36338 (comment) for @mkoeppe's opinion on this. Without requiring "positive review" label, we can check the PR before we set it positive review.
Contributor
There was a problem hiding this comment. It will influence every other PR, so I think there should be some safety measure.
What do you mean? Doesn't the CI of that PR already checks the PR? Why do you need other PRs to check it?
Contributor
Author
There was a problem hiding this comment.
We already have that, namely the Triage group + common sense.
Contributor
Author
There was a problem hiding this comment.
Well, it's not fire, but it's a matter of "availability of the CI services", which, if poor, leads to developer frustration (@fchapoton @tornaria) and can dramatically reduce development velocity because developers may disengage or wait until the CI is fixed again. Let's consider the period from the release of a beta version to the release of the next beta version, currently about a week. For, say, 95% of availability of the CI, we want a CI fix to take effect within a few hours of a broken release.
This may be true for trivial fixes such as the linter failures, which can be fixed and reviewed by many people. But for fixes to CI and build system, it hinges on the availability of a very small number of people, and time-to-positive-review can be several days -- which can easily make the availability of the CI services less than 50%.
Contributor
Author
There was a problem hiding this comment. @tobiasdiez wrote:
I know that @tobiasdiez knows this already, but I'll point out again: Our project does not hold majority votes In PR discussions.
This is false. This PR does resolve the issue of the failing CIs. Merging this PR is enough to fix all CI problems in the current cycle: it auto-applies the blocker PRs #36327 and #36276.
This is false, and moreover an inappropriate attack on author and reviewers.
Collaborator
There was a problem hiding this comment. OK. Here is my suggestion. As we can observe by looking at the labelling war below, there are conflicting opinions on which way this PR should proceed. If the issue is large and of broad interest in our community, then this situation would be resolved by a voting in sage-devel. But the current issue is a small one, and it would be ridiculous and shameful to spend a long time for endless discussions or a large-scale voting. Hence let me suggest a small voting here, as defined below: The voting is about (a) "only blocker label" vs (b) "blocker label + positive review". The voting occurs only in this thread. The voting starts by the votes of @tobiasdiez and @mkoeppe, by which they are supposed to have vowed on accepting the voting result. The voting ends automatically when there was no more vote for the last 24 hours. When the voting ends, the author of the PR proceeds in the way of winning candidate. Please only vote with an optional simple comment in the rest of this thread.
Contributor
Author
There was a problem hiding this comment. -1 on holding majority votes in PRs.
Contributor
There was a problem hiding this comment.
Sadly not false... |
||
| if [ -z "$PRs" ]; then | ||
| echo 'Nothing to do: Found no open PRs with "blocker" status.' | ||
| else | ||
| echo "Found PRs: " $PRs | ||
| export GIT_AUTHOR_NAME="ci-sage workflow" | ||
| export GIT_AUTHOR_EMAIL="[email protected]" | ||
| export GIT_COMMITTER_NAME="$GIT_AUTHOR_NAME" | ||
| export GIT_COMMITTER_EMAIL="$GIT_AUTHOR_EMAIL" | ||
| git commit -q -m "Uncommitted changes" --no-allow-empty -a | ||
| git tag -f test_head | ||
| for a in $PRs; do | ||
| echo "::group::Merging PR #$a" | ||
| $GH pr checkout $a | ||
| git checkout -q test_head | ||
| if git merge --no-edit -q FETCH_HEAD; then | ||
| echo "::endgroup::" | ||
| echo "Merged #$a" | ||
| else | ||
| echo "::endgroup::" | ||
| echo "Failure merging #$a, resetting" | ||
| git reset --hard | ||
| fi | ||
| done | ||
| fi | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Interesting approach. There is a couple of thoughts I have about this:
Would there still be a sanity check that just tests this PR without merging in other things?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There isn't, and I don't think it's needed. Since the transition to GitHub, using priority labels has become pretty rare, and no one has used the blocker tag without a good reason.
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but for these the merge would be a no-op, and I think I'm handling this correctly in the scripts.
EDIT: See the run for the current workflow.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Currently, the release manager scripts ignore all dependencies and all priorities anyway...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds plausible.