CI: Use Mergify for automated PR merging with safety requirements

[okurz]

Motivation:
Improve development workflow by automating the merging process once all CI
checks pass. This ensures that PRs are not merged manually with missing or
failed CI checks, which can happen if they are not triggered correctly.
Mergify is more diligent in verifying that all required checks are present and
successful.

Design Choices:

• Require at least 2 approvals for safety, mitigating concerns about bad
  actors or account takeovers.
• Only merge if "integration" and "unit" tests pass on the "master" branch.
• Avoid merging if "not-ready" or "acceptance-tests-needed" labels are present.
• Automated conflict notification to prompt authors for fixes.

User Benefits:

• Faster merging of approved and tested contributions.
• Improved reliability by strictly enforcing CI check requirements.
• Consistent automation across the os-autoinst/openQA organization.

[foursixnine]

Change the approval requirement to members of @os-autoinst/asset-sync-and-integration

[okurz]

Change the approval requirement to members of @os-autoinst/asset-sync-and-integration

I don't think that's possible according to https://docs.mergify.com/workflow/actions/merge/ . Also I don't think that we need to overcomplicate things. Still only approvals from priviledged members of the organisation count, not like anybody

[foursixnine]

Change the approval requirement to members of @os-autoinst/asset-sync-and-integration

I don't think that's possible according to https://docs.mergify.com/workflow/actions/merge/ . Also I don't think that we need to overcomplicate things. Still only approvals from priviledged members of the organisation count, not like anybody

Not convinced. Ever heard of bad actors and account takeover? - If others (non members of @os-autoinst/tools-team) agree, and number of approvals is increased to 2 and I can reconsider.

I am not a fan of automated merges on things that go live - also no description is provided nor reasoning as to why is it important for this repository.

[okurz]

Change the approval requirement to members of @os-autoinst/asset-sync-and-integration

I don't think that's possible according to https://docs.mergify.com/workflow/actions/merge/ . Also I don't think that we need to overcomplicate things. Still only approvals from priviledged members of the organisation count, not like anybody

Not convinced. Ever heard of bad actors and account takeover? - If others (non members of @os-autoinst/tools-team) agree, and number of approvals is increased to 2 and I can reconsider.

Sure, let's play it safe(r). Increased to >=2 now.

I am not a fan of automated merges on things that go live - also no description is provided nor reasoning as to why is it important for this repository.

Well, first it might seem to be more convenient than it is important but we have good experiences in os-autoinst, openQA and others and the approach is helping to improve the quality and prevent unforeseen problems because unlike human reviewers mergify is very diligent to ensure that all necessary CI checks are not failed but also present as happened multiple times that CI checks were not triggered at all.

[okurz]

Change the approval requirement to members of @os-autoinst/asset-sync-and-integration

I don't think that's possible according to https://docs.mergify.com/workflow/actions/merge/ . Also I don't think that we need to overcomplicate things. Still only approvals from priviledged members of the organisation count, not like anybody

Not convinced. Ever heard of bad actors and account takeover? - If others (non members of @os-autoinst/tools-team) agree, and number of approvals is increased to 2 and I can reconsider.

I am not a fan of automated merges on things that go live - also no description is provided nor reasoning as to why is it important for this repository.

@foursixnine I updated the commit message and PR description to include motivation, reasoning, design choices and benefits. Please take a look again

[foursixnine]

I still don't think this is needed for this repo, but since we can still merge manually, I have no troubles with the current proposal

[Vogtinator]

This is highly insecure as long as the GitHub option to automatically remove stale reviews is disabled. See https://github.blog/changelog/2023-06-06-security-enhancements-to-required-approvals-on-pull-requests/

[okurz]

This is highly insecure as long as the GitHub option to automatically remove stale reviews is disabled. See https://github.blog/changelog/2023-06-06-security-enhancements-to-required-approvals-on-pull-requests/

please elaborate why you see it like this. Keep in mind that CI jobs for newcomers need approval anyway

[Vogtinator]

please elaborate why you see it like this

1. User creates PR with valid (mergable) changes
2. CI passes, reviewers send approvals, mergify would merge
3. User pushes new (possibly malicious) changes on top
4. mergify merges new changes without requiring a review of the new diff

With the option I mentioned, step 3 would invalidate the reviewer approvals and require re-review. (Does not apply to plain rebases which don't change the PR diff)

[okurz]

please elaborate why you see it like this

1. User creates PR with valid (mergable) changes

2. CI passes, reviewers send approvals, mergify would merge

3. User pushes new (possibly malicious) changes on top

4. mergify merges new changes without requiring a review of the new diff

With the option I mentioned, step 3 would invalidate the reviewer approvals and require re-review. (Does not apply to plain rebases which don't change the PR diff)

I would distinguish between trusted users for which CI jobs are automatically triggered and non-trusted users for which CI jobs need approval.

[Vogtinator]

I would distinguish between trusted users for which CI jobs are automatically triggered and non-trusted users for which CI jobs need approval.

Does that make a difference for the scenario I posted?

[perlpunk]

With the option I mentioned, step 3 would invalidate the reviewer approvals and require re-review. (Does not apply to plain rebases which don't change the PR diff)

If a simple rebase does not invalidate existing reviews, that sounds like a good compromise to me

[okurz]

I would distinguish between trusted users for which CI jobs are automatically triggered and non-trusted users for which CI jobs need approval.

Does that make a difference for the scenario I posted?

I think so. This is how I understand how scenarios look for different users:

Untrusted user

1. Untrusted new contributor creates a pull request
2. repo owners must approve CI runs
3. repo owners give 2 approvals
4. untrusted new, malicious contributor quickly pushes a malicious update
5. CI jobs won't run, mergify won't merge

Trusted user

1. Trusted contributor creates a pull request
2. repo owners give 2 approvals
3. Compromised or malicious contributer quickly pushes a malicious update
4. CI jobs will run, pass, mergify will merge

So is the "Trusted user" scenario the one which you had in mind?

[Vogtinator]

Both. In the first scenario with the untrusted user it's a bit harder, as the "Approve CI run" button still needs to be hit.

It's still a major issue that that button is essentially equivalent to a review approval, as that's not obvious to reviewers, especially if they think it was just a rebase on the PR base branch.

[Vogtinator]

If a simple rebase does not invalidate existing reviews, that sounds like a good compromise to me

Yup, the only issue is that this option is (AFAIK) only available if the "Require approvals for merge" checkbox is clicked which can be annoying in some circumstances.

[perlpunk]

Both. In the first scenario with the untrusted user it's a bit harder, as the "Approve CI run" button still needs to be hit.

It's still a major issue that that button is essentially equivalent to a review approval, as that's not obvious to reviewers, especially if they think it was just a rebase on the PR base branch.

True. Also, honestely I don't consider a contributor trusted automatically after they had just one pull request merged.
See the xz utils backdoor.

[Vogtinator]

Any news here? FWICT it's still enabled.

[okurz]

Any news here? FWICT it's still enabled.

there are no new news. I don't see why this project needs to change anything as we have basically the same rules in other projects for many years. If you have a reasonable proposal considering pros and cons please open an according pull request.

[Vogtinator]

I don't see why this project needs to change anything as we have basically the same rules in other projects for many years.

It's a recent change for this (larger) project, and just because it's also done somewhere else does not mean it's good, just that it's widespread.

If you have a reasonable proposal considering pros and cons please open an according pull request.

That would be a revert of this, with the reasoning being:

This is highly insecure as long as the GitHub option to automatically remove stale reviews is disabled. See https://github.blog/changelog/2023-06-06-security-enhancements-to-required-approvals-on-pull-requests/