search

mage-os / terraform

Terraform files for managing the organization repository permissions.
4 stars 8 forks source link

Merging is blocked #91

Closed sprankhub closed 1 month ago

sprankhub commented 4 months ago

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

image

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

sprankhub commented 4 months ago

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

image

But who should be able to merge?

sprankhub commented 4 months ago

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 commented 4 months ago

I think this is a regression from the monorepo branch protection at https://github.com/mage-os/terraform/commit/ea3a4d098310f258b7e3a5c341e049e9b47b9ddb#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 commented 4 months ago

PR opened based on that thinking: #95

sprankhub commented 1 month ago

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.