Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merging is blocked #91

Closed
sprankhub opened this issue Jun 27, 2024 · 5 comments · Fixed by #95
Closed

Merging is blocked #91

sprankhub opened this issue Jun 27, 2024 · 5 comments · Fixed by #95

Comments

@sprankhub
Copy link
Contributor

Somehow, it seems to be impossible to merge PRs even if they have enough approvals. An example would be mage-os/mageos-async-events-sinks#5:

image

It looks like merging is currently not possible for anyone but @Vinai.

@sprankhub
Copy link
Contributor Author

It looks like these branch protection settings prevent us from merging:

image

But who should be able to merge?

@sprankhub
Copy link
Contributor Author

In the tech meeting, we decided that code owners should be able to merge. Things to check:

  1. Can code owners then also push new branches? This might be okay.
  2. Can code owners then also push to existing branches? This would not be okay.

@rhoerr
Copy link
Contributor

rhoerr commented Jul 13, 2024

I think this is a regression from the monorepo branch protection at ea3a4d0#diff-dc46acf24afd63ef8c556b77c126ccc6e578bc87e3aa09a931f33d9bf2532fbb

Previously there was no restrict_pushes rule. Now it sets push_allowances to mage-os-ci if it's a monorepo, or an empty array (no teams/users, meaning nobody gets push permissions?) if not. Vinai must be the only one able to merge by nature of being the organization owner/admin.

So I suppose the question is whether we want to go back to the prior behavior (which probably did allow codeowners to push to branches), or refactor for more fine grained control.

If the prior behavior is okay, I think this would maybe handle it?. Untested speculation.

  restrict_pushes {
    push_allowances = try(each.value.is_part_of_monorepo, false)
      ? [data.github_user.mage-os-ci.node_id]
      : github_repository.repositories[each.key].teams
  }

Point of concern: Does this block mage-os-ci from pushing on non-monorepo repositories?
If we need to allow mage-os-ci as well, then we need to change the non-monorepo path to reference each team and user by node_id, per the example comment on https://registry.terraform.io/providers/integrations/github/latest/docs/resources/branch_protection .

@rhoerr
Copy link
Contributor

rhoerr commented Jul 17, 2024

PR opened based on that thinking: #95

@sprankhub
Copy link
Contributor Author

sprankhub commented Sep 16, 2024

In the tech meeting, we decided that code owners should be able to merge. Things to check:

  1. Can code owners then also push new branches? This might be okay.
  2. Can code owners then also push to existing branches? This would not be okay.

I tested this now. When merging the PR, code owners can indeed push new branches. However, they cannot push to existing branches:

$ git push
Enumerating objects: 5, done.
Counting objects: 100% (5/5), done.
Delta compression using up to 16 threads
Compressing objects: 100% (3/3), done.
Writing objects: 100% (3/3), 943 bytes | 943.00 KiB/s, done.
Total 3 (delta 2), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (2/2), completed with 2 local objects.
remote: error: GH006: Protected branch update failed for refs/heads/sprankhub-test.
remote: 
remote: - Changes must be made through a pull request.
To github.com:mage-os/mageos-async-events.git
 ! [remote rejected] sprankhub-test -> sprankhub-test (protected branch hook declined)
error: failed to push some refs to 'github.com:mage-os/mageos-async-events.git'

Also, merging the PR will indeed lead to the fact that code owners can merge approved PRs.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants