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

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

allow empty push_restrictions #594

Description

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions