CI: Add a verify-success reusable workflow to use for required checks

[echoix]

Since the issue might be a little weird to understand, let's go through the problem by stating individual facts, then explaining why the PR is needed and the changes to make.

1. The current repo's settings defines rulesets (or the older branch protection rules that now work similarly) that selects some required checks before merging. That is a good thing.
2. The current repo has multiple checks. That is also good.
3. Some background terminology:
   • A workflow is defined in a .yml file.
   • A workflow defines the triggers and filters that it is executed on.
   • A workflow contains one or more jobs.
   • A job can include a matrix strategy to create jobs for multiple combinations of variables using a single job definition.
   • A job contains steps to run, or can call a reusable workflow defined elsewhere, like another file or repo.
   • When a reusable workflow is used, the job has no other steps.
   • A job can depend on other jobs in the same workflow, using the needs property. This is what makes some pretty diagrams in the workflow summary when more than one rectangle is present. Usually, all jobs in needs have to be successfully complete before the dependant job starts, unless a conditional expression makes it continue.
   • The result of a job can either be success, failure, cancelled, or skipped.
   • A job can use conditions (by using if at the job-level) to control if it should be executed, or it should be skipped.
   • Skipped jobs due to a conditional will report its status as a success. (i.e. A job runs for the main repo, another for pull requests that come from a fork. A failure wouldn't be appropriate)
4. Some checks use a matrix strategy to run a (or multiple) job, specifying the variables there instead of all around the job. This is also not a problem.
5. Each job (normal or created from a matrix strategy) will create a status check. For matrix strategy jobs, they are named using the variables of the matrix strategy.
6. Required status checks are defined for a context that use the name of the status check.

Now the problematic part starts:

1. Since the required status checks depend on the name of the status check,
2. and the name of the status check depend on the values of the matrix strategy variables,
3. then it is not possible to change a variable in a job's matrix strategy to another value if it is a required check. It is only possible to add a new value to an existing variable inside the matrix strategy, but that won't make this new strategy required too. It wouldn't be possible to add another variable to the matrix strategy, as it changes the name of the status check.

Examples of the problematic behaviour:

• Adding python 3.12 to the pytest versions tested in CI: Add Python 3.12 to pytest's matrix #3314 was non-breaking. It is not a required check at the moment. It would not be possible, at the moment, to create a PR that changes either Python 3.8 or Python 3.10 from that workflow, for example when we change our supported python versions. Furthermore, it wouldn't even be possible to set 3.10.0 since the name would be different.
• Trying to sync the updated linter versions with an update of the pre-commit config in pre-commit: Update pre-commit config and tools versions #3318 waited indefinitely for a status check to be reported, but would never be satisfied since that combination wouldn't exist anymore. So the PR would remain unmergeable, deadlocked.

So the solution is to have the status checks defined on a job that can only complete successfully if the checks we want to be required are also successful. That is what I bring in this PR.

To make it possible to use the same logic multiple times, I created a reusable workflow called verify-success.yml, and call it from our existing workflows. In order to have the most flexibility to really create the outcomes that we want, I added inputs that allow to adjust for what types of job results that check should fail. That reusable workflow doesn't run my itself since it only has the workflow_call trigger.

In the future, I imagine that I might bring a PR that combines multiple workflow files (that have the same triggers) together to reuse some commonly built artifacts, or optimizations like that. But if some of the jobs shouldn't be required, but others do, the way I created the reusable workflow allows to group the non-required jobs or even cancelled jobs to make the result of that verify-success job successful, and that could be used a dependant job for another call of verify-success, along with other required jobs. That's why you already see that I set the job_id to ubuntu in the ubuntu.yml workflow.

I tested out this with multiple smaller jobs with all statuses, and all 16 combinations of my input flags (boolean). It works correctly.

Some more info:
https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/collaborating-on-repositories-with-code-quality-features/troubleshooting-required-status-checks
https://docs.github.com/en/actions/using-workflows/required-workflows
https://docs.github.com/en/actions/using-workflows/workflow-syntax-for-github-actions#jobsjob_idneeds
https://docs.github.com/en/actions/learn-github-actions/contexts#needs-context
https://docs.github.com/en/actions/using-workflows/reusing-workflows#overview

Now, for the settings changes that an admin has to do to allow this PR to merge:
I made them all from rulesets. They are layered, and when more than one rule applies, the strictest setting is used.
I made screenshots of the configuration I think we need.

1. In the repo settings, open the Rules > Rulesets tab. Create a new ruleset.

   Image

   

   

2. Add bypass to admin, it is sometimes needed. I selected for pull requests for now

   Image

   

   

3. Choose the branches to target. The default branch, plus our releasebranch_*

   Image

   

   

   

   Once saved, we see that it will apply correctly:
   

4. For the next section of settings, we select the branch protection settings

   Image

   I started by selecting at least to disallow deletions.
   

   Then, I selected the most basic settings concerning pull requests. Some of them have bigger quirks than others, and need some time getting used to. If we were to be more active on reviews, "Require approval of the most recent reviewable push" could be interesting in order to not allow someone to merge their own pull requests. I don't think we are ready for this yet.

   

5. Then the core part, specific for this PR, choosing the settings for "Require status checks to pass".

   Image

   If the rules were configured as rulesets, make sure to remove the existing entries for Ubuntu (2 lines), Pytest (2 lines), and Python Code Quality (2 lines). If there is nothing there, we need to remove

   Then, select all the following entries, to match the picture:
   
   The newest checks that come from this PR might only show once they have run once for this PR (if they don't show yet, wait for the first round of CI checks since this PR was opened to complete first)

6. Lastly, I selected to block force pushes to these 7 (for now) protected branches, enabled, and saved.

   Image

   

   I saved the settings.
   Then back to the top, make sure that the ruleset is enabled:
   

   Then save again.

7. If there wasn't any required checks in the rulesets, then the configuration was through branch protection rules. They are found on the left, two tabs above rulesets. Make sure to disable any conflicting configuration from there. The functionality of the older branch protection rules was ported up to be essentially the same (user-interface - wise).

I exported my config, I'm joining it here. It might reference my repo though:
Main and release branch rules.json

[wenzeslaus]

Thanks for discussing the rules. Your ruleset JSON imports cleanly, so we can use that. We are using Branch protection rules now.

The required checks make sense only for branches where the checks are present. For release branches so far, we are not requiring any checks. I think we need 8.4 or higher or something like that for the new ruleset.

I definitively find it strange that I need to change the rules every time a version updates. It is doable, but not smooth. I would rather see a GitHub solution than more custom CI code, but faster updates of more versions require more of these manual non-code updates and that's not a good outlook, so we need a solution sooner rather than later.

While the same system of true matrix-based multi-jobs, the use of the matrix to set the versions for one job started as a workaround of not having a GitHub feature for setting variables which are not environment variables, so maybe there is a better way now which would (only!) partially reduce need for this PR.

[echoix]

Thanks for discussing the rules. Your ruleset JSON imports cleanly, so we can use that. We are using Branch protection rules now.

I can't be sure that using both Branch protection rules and rulesets will do what we want. Adding rulesets does not remove the (as I understand) current branch protection rules that require a check that won't exist if a version is updated. If you sent me (privately if needed) screenshots of the existing config in branch protection rules, I could study a suggestion, on what to remove from it (and make sure an equivalent is set on the rulesets).
If the changes aren't ready to be applied yet, you might want to make sure that the imported rules (from the JSON) has its "Enforcement status" set to "disabled" to not block the whole repo.

The required checks make sense only for branches where the checks are present. For release branches so far, we are not requiring any checks. I think we need 8.4 or higher or something like that for the new ruleset.

In that case, we should split the ruleset to only apply on what we want. Like what is common to all (the permissions), and what is specific (main and later, vs older release branches)

I definitively find it strange that I need to change the rules every time a version updates. It is doable, but not smooth. I would rather see a GitHub solution than more custom CI code, but faster updates of more versions require more of these manual non-code updates and that's not a good outlook, so we need a solution sooner rather than later.

This is what it is fixing. By configuring the repo to require a check (or a check name) that doesn't change in time. I saw other repos doing the same-ish idea, some had issues at first with handling of skipped jobs that didn't block them from merging when they wished it would, that's why I really took my time to handle the possibilities. I avoid the flaws discussed here: https://github.com/orgs/community/discussions/26822

While the same system of true matrix-based multi-jobs, the use of the matrix to set the versions for one job started as a workaround of not having a GitHub feature for setting variables which are not environment variables, so maybe there is a better way now which would (only!) partially reduce need for this PR.
The usage of matrix is not the problem here, I think. It is how the requirements are set. I'm not even sure that older release branches could have a valid PR if the workflow in that branch has an older check name.

I'm thinking about this. Why don't we only apply the advanced required checks for the default branch (main), and handle completely differently the releasebranch_* branches. Something like it MUST have 1 approval, and it would be a manual review (without enforcing anything, since it might have to change in the future, or the fix is expressly to fix something new that breaks with older code, so enforcing "new" checks is incompatible). It could even be limited to people with bypass permissions for these branches.
How many people have bypass permissions (admin-level) permissions? (To know if it would be viable to restrict merging backports only to them, or else no "enforcing" of the status checks)

[echoix]

On a side note, concerning repo settings, it would have been useful for #531 if automerge wasn't disabled.

This would have changed this:

Into this (for checks running):

Enabling auto merge enables the possibility of auto merging, not that it automerges anything by default.
If the checks are finished running, there are no changes:

The setting to change is in the General tab:

and near the bottom of the pull requests group box.

I double checked, and there isn't any merging done if there is no required checks or no required reviewers. It stays a simple PR. The wording of the interface is scarier than it is.

I use it daily on other repos. Once I make my tour and everything is ok, and only the CI needs to finish after my review, I enable it and it will do its thing without having to go back to it.

[echoix]

I'm polishing up a new round of rulesets. I experimented a bit more in the last days with other ways of splitting them, some configurations I didn't like, but I think I have something I like better now. I'll come back with it when they are ready.

[echoix]

For the updated rulesets, I suggest the following:

1. Default branch with general settings:

   • File: Default branch general settings.json

   • Doesn't contain the required checks, they are separated to be contained in one with their separated permissions.

   • Requires a pull request before merging (so no accidental git push to the bad remote)

   • Doesn't also require, for now, that the pull requests should have a review before merging, for members to get used to it. For roles "maintain" and "admin", they will have a bold red-text checkbox allowing them to bypass the protection, and the merge button will become red clearly indicating they are bypassing the protections. It's a good thing to be able to unblock the repo.

   • Screenshot

     
     

2. Required status checks for the default branch:

   • File: Required checks for default branch.json

   • No real changes compared to before

   • Screenshot

     
     

3. Release branches:

   • File: releasebranch restrictions.json

   • Creation of release branches is restricted to people with maintain or admin roles (not write). Release branches trigger publishing of docker images, so pushing to these by accident should be avoided.

   • It also has bypass for maintain and admin roles.

   • It does not have required checks, since the checks change over time and the Github settings might be out of date. Instead, it requires at least 1 PR approval. For now a user with "maintain" and "admin" could approve their own PR, but as above, it makes it obvious that they know what they are doing. Backports are a little bit rarer.

   • Screenshot

     
     

4. All other branches (not default, not release branches), which would be there only when mistakes are made, or by tools like renovate, dependabot, or our integrations:

   • File: All other branches PR reviews.json

   • Written with these exclusions so quirks about rules layering wouldn't matter, being easier to change less important rules. (It doesn't match the branches that apply to the other rules)

   • Has "maintain" and "admin" have bypass permissions (always), but "write" has bypass permissions for PRs.

   • The idea here is that is another unrelated branch was there, it would be possible to clean up without knocking for help.

   • It requires a PR before merging, so no accidental pushes to a unrelated branch in the main repo.

   • Requires 1 PR approval, but has "Require approval of the most recent reviewable push" (so someone else than the person who pushed must review), and conversations must be resolved.

   • It's a little bit stricter, but still allows write permissions to bypass to solve their mistake, if it happens. The red warnings are enough to make sure you think.

   • Again, since this rule doesn't affect main or release branch, it's kinda of a sandbox, but with an exit door. Member with write permissions should already be trusted, if you don't trust them enough to merge into main or releasebranch_*, they should not have that role and make PRs that would be merged normally like others. Collaborators aren't really limited by not having write permissions to a repo.

   • I explicitly a) didn't enable the restriction to delete these other branches (to allow fixing a mistake), or b) restrict the creation of these other branches to allow current and future tools and integrations to create branches without bumping against these rules. It's quite limiting to have a branch creation restriction, and with that config, doesn't really matter yet to enforce.

   • Screenshot

     
     

     For the bypassing protections, it looks like this:
     
     And this (once checked)
     
     

Lastly, it is worth to mention that using rulesets, failures because of permissions are a little less opaque. For example,
while with branch protections you see that there is some branch protection rules, like

Screenshots

In the current repo, logged in as me,

View rules is available,

but can't show anything to understand:

In the current repo, in private navigation (logged out) (note that some beta-ish repository views might not be enabled when logged out), we get:

and clicking in this brings:

While with rulesets, we have more insights, even publicly:

Screenshots

In my repo, logged in as me, we have both this

and this

bringing to this

where we can see the rules that apply and their details, this extra transparency allowing to create an issue to discuss config changes.

Logged out, we see

That brings to the same

with the same amount of details

[wenzeslaus]

Auto-merge is enabled. I went through the doc and fits with your explanation, but it is similarly confusing/scary as the UI, like they never considered that people want to hear that they will be confirming the action first. It is basically a new button for each PR to "merge later when ready", not a policy to "immediately merge everything across the whole repo".

[echoix]

Auto-merge is enabled. I went through the doc and fits with your explanation, but it is similarly confusing/scary as the UI, like they never considered that people want to hear that they will be confirming the action first. It is basically a new button for each PR to "merge later when ready", not a policy to "immediately merge everything across the whole repo".

Yah, I know, I was scared too when playing in my settings, but since I already knew what the feature was (from using it), I only had to find how it was called. And it's that. It's not so bad.

[wenzeslaus]

The plan is to change the rules and then merge this, correct?

[echoix]

Yes, if the rules seem okay to you, and they are equivalent enough to replace the branch protection rules (at least for the main branch, and at least for the required checks).

The more I think about it, the more I think the members that merge PRs are getting used to add reviews, and thus checking the require PR reviews (to at least 1) might not be a so bad idea. At first I wanted to be extra conservative to not add anything more, but since automerge is possible, it plays a bit nicer with it: you can set automerge on waiting for an "approval" and expressly don't add yours to wait for someone to add it.

[wenzeslaus]

I applied the two rulesets for the default branch. I added linear history to 1 (otherwise that would be ensured only by using PRs and the settings for the PRs). I also experimentally added the 1 required approval. The bypass is enabled as suggested. I removed everything from 2 except the checks. I deleted the old branch protection rule for main. Result:

https://github.com/OSGeo/grass/rules?ref=refs%2Fheads%2Fmain

I didn't create the release and other branches rulesets. I'll leave feedback for these later. (After reviewing this PR again so we can merge it, ha ha.)

[echoix]

For the require linear history, I didn’t opt to include it in my recommendation since we sometimes might need to have a PR not squashed merged to help with file renames. But since we require PRs, the default is squash merge, and the ones who can merge should know what they are doing, it wasn’t a concern for me. You can look at the descriptions of the potential problems or things to look out (for the available rules) in this doc https://docs.github.com/en/repositories/configuring-branches-and-merges-in-your-repository/managing-rulesets/available-rules-for-rulesets#require-linear-history

[echoix]

@wenzeslaus Lastly, since the rulesets are now active, we need to merge this PR that includes the now required status checks tonight, so that all the other PRs aren’t blocked by this. Either this, or to not enforce the rule if you still want to wait.

[wenzeslaus]

Good job writing that reusable workflow, but I'm anyway secretly hoping GitHub will a native solution at some point.

[echoix]

I’ll let you try using the auto merge for this one ;). Enable it, then go and approve the PR when ready.

[wenzeslaus]

Too late! :-))) But I already tried elsewhere (just to enable and disable).

[echoix]

Good job writing that reusable workflow, but I'm anyway secretly hoping GitHub will a native solution at some point.

It seems like there was an intention for this on October, but when will it really be available? We never know. We'll adapt. It's our tooling, it's not set in stone.

[wenzeslaus]

About the rulesets for release branches, release branches are now often updated locally, i.e., backports are happening without PRs. They can and do happen with PRs sometimes, but not always. Release procedure also does direct commits and pushes to release branches. There was a talk about using a bot for backports, so maybe that would help here. Current release branch rules are "Require linear history" and "Do not allow bypassing the above settings". I don't have a suggestion right now on how to proceed.

[echoix]

I don't see any rulesets for the release branches. Are they really set, or it is branch protection rules (that I can't really help or analyze)

[wenzeslaus]

For clarity, I migrated the current branch protection rules for release branches to a new ruleset. It is the same as before including no bypass.

[wenzeslaus]

When reviewing #3333, I decided to also require conversations to be closed before merging. I was giving an approving review, but there was still an open comment. Auto-merge would make sense only after the comment is resolved. It seems that if you want a review, you want the conversations to be closed as well.

[echoix]

When reviewing #3333, I decided to also require conversations to be closed before merging. I was giving an approving review, but there was still an open comment. Auto-merge would make sense only after the comment is resolved. It seems that if you want a review, you want the conversations to be closed as well.

Totally agree, and with auto-merge/require resolved conversations, the UI makes it clear why it isn't ready to merge.