allow empty push_restrictions

[lifeofguenter]

It currently does not look like this config is possible with the github_branch_protection resource:

[bouk]

The problem is that restricts pushes is only set if the number of users that have access > 1: https://github.com/terraform-providers/terraform-provider-github/blob/e9dc4e7177785b1a5c349cb15527b263dba389ef/github/util_v4_branch_protection.go#L159

[mrodm]

I have the same issue using version 4.2.0 of the provider.
If I want that check enabled, the workaround that I just found was adding a user to the list.

[github-actions]

👋 Hey Friends, this issue has been automatically marked as stale because it has no recent activity. It will be closed if no further activity occurs. Please add the Status: Pinned label if you feel that this issue needs to remain open/active. Thank you for your contributions and help in keeping things tidy!

[fatbasstard]

Hi all!

I'm hitting this same issue now. I want the option to be enabled, just as in the picture above, without specifying names. In my case I only want "maintainers" to be able to push...

This is currently not possible

[georgekaz]

I'm bumping against this issue too now and I thought I might work towards a PR on it but thought it was worth discussion. If you look at the image from the UI:

It kind of feels like it should be structured more like the following, where having the restrict_pushes block is what enables it.

restrict_pushes {
    blocks_creations  = false
    push_restrictions = [
      "/some-user"
    ]
  }

The Graph API has each of these as their own property; restrictsPushes (bool), blocksCreations (bool) and pushAllowances (list). The code currently sets restrictsPushes to true if pushAllowances list count > 1 but it does not have a method to individually set this. In the REST API version (_v3), having empty lists was the way to enable it, so for consistency with that you'd just have this and have restrictsPushes = true if push_restrictions exists:

push_restrictions = []
blocks_creations = true

I'll try and raise a patch PR and maybe someone else can think about the future of this.

[georgekaz]

I've been trying to create a PR to resolve this and I just can't seem to manage it without making breaking changes. I'm not really sure what the protocol on creating breaking changes is. If I'm allowed to offer breaking changes, I have a working solution.

As it currently stands, if push_restrictions is defined or not, the state json stores "push_restrictions": []. This means that if you define it as an empty list or you don't define it at all, the state can't tell it apart. Therefore defining it empty cannot be used to enable push restrictions.

Option 1
You define another property in state such as restrict_pushes = true in order to track it. However, by adding this (with a sensible default false), existing deployments that do provide a list of push_restrictions actors will stop working unless the new property is defined. Currently this isn't needed because of line 318 here:

terraform-provider-github/github/util_v4_branch_protection.go

Lines 316 to 319 in 46650cb

	if len(pushActorIDs) > 0 {
	data.PushActorIDs = pushActorIDs
	data.RestrictsPushes = true
	}

, but this must be removed if it's controlled by the new property or it will create constant state flux.

restrict_pushes = true
push_restrictions = [
"/someone"
]
blocks_creations = true

Option 2
Restructure the whole thing in this format and move the old properties inside of this block.

  restrict_pushes {
    blocks_creations = true
    push_restrictions = [
      "/some_user"
    ]
  }

The inclusion of the block is what enables the restrictions but with no actors specified, with the inner properties configuring the additional features. Removing restrict_pushes turns off the feature. e.g. enable the feature with the following to allow empty push_restrictions

  restrict_pushes {
  }

My preference is for solution 2 because I don't like that properties rely on other properties but aren't noticeably connected. (While doing this I've discovered that if you set blocks_creations = true but don't set the push_restrictions list, blocks_creations is never enabled because RestrictsPushes isn't true, and so the state is in constant flux.)

i.e. this will not work because the user has failed to set restrict_pushes = true

push_restrictions = [
"/someone"
]

That's why connecting them in a block would be better.

I'd love some feedback on this please. Thanks

[kfcampbell]

@georgekaz I like your proposed solution and have responded on the PR; the main issue now is that major version changes in Terraform are somewhat onerous and require code to handle state migrations to avoid breaking users.

[georgekaz]

Changes to support this (#2045) have been merged into the v6 release so I think this issue can be closed.