Organization Ruleset missing support for OrganizationAdmin bypass

[YElyousfi]

Expected Behavior

actor_id should not be required because it is not applicable to OrganizationAdmin role type

Actual Behavior

actor_id causes apply to fail if it is not set or set to null. Setting to 0 doesn't work either. Creating a ruleset in the UI with org admin bypass and fetching the payload shows that the actor id should be null.

Curious on what @o-sama thinks and if a change is needed.

Terraform Version

V1.5.5

Affected Resource(s)

• github_organization_ruleset

Terraform Configuration Files

No response

Steps to Reproduce

No response

Debug Output

No response

Panic Output

No response

Code of Conduct

• I agree to follow this project's Code of Conduct

[o-sama]

@YElyousfi Thanks for opening this! I'll take a look into it tomorrow.

My first thoughts though are that this isn't really a provider issue, it could be something on the go-github side that we can look into, or it may be on the API side. The reason I'm saying this is that the API docs explicitly list the field as required. It's hard for me to say more without looking into the code and testing it manually since I'm on my phone at the moment. For reference:

[o-sama]

Seems my hunch was partially correct, even if we were to set it to nil on the provider's side, we'd get the following error:

github_organization_ruleset.test: Modifying... [id=55542]
╷
│ Error: PUT https://api.github.com/orgs/assuranceiq/rulesets/55542: 422 Invalid request.
│ 
│ Invalid property /bypass_actors/1: object is missing required key: actor_id. []
│ 
│   with github_organization_ruleset.test,
│   on main.tf line 33, in resource "github_organization_ruleset" "test":
│   33: resource "github_organization_ruleset" "test" {
│ 

This is part of the request made on my test provider build where I set actor_id to nil if the type is OrganizationAdmin:

"bypass_actors": [
2023-09-18T15:06:59.339-0400 [DEBUG] provider.terraform-provider-github:   {
2023-09-18T15:06:59.339-0400 [DEBUG] provider.terraform-provider-github:    "actor_id": 5,
2023-09-18T15:06:59.339-0400 [DEBUG] provider.terraform-provider-github:    "actor_type": "RepositoryRole",
2023-09-18T15:06:59.339-0400 [DEBUG] provider.terraform-provider-github:    "bypass_mode": "always"
2023-09-18T15:06:59.339-0400 [DEBUG] provider.terraform-provider-github:   },
2023-09-18T15:06:59.339-0400 [DEBUG] provider.terraform-provider-github:   {
2023-09-18T15:06:59.339-0400 [DEBUG] provider.terraform-provider-github:    "actor_type": "OrganizationAdmin",
2023-09-18T15:06:59.339-0400 [DEBUG] provider.terraform-provider-github:    "bypass_mode": "always"
2023-09-18T15:06:59.339-0400 [DEBUG] provider.terraform-provider-github:   }
2023-09-18T15:06:59.339-0400 [DEBUG] provider.terraform-provider-github:  ],

There are a couple of things to note here:

• The API documentation states that actor_id is required, as tested above, it is clearly a required field which is strictly populated with null when the request is sent through the UI.
• The current approach go-github takes when serializing this field is json:"actor_id,omitempty". If this field is nil then it'll just be dropped when serializing the struct.
  • This playground might help explain what I mean

Based on those notes, as things currently stand, whichever approach we take with the provider would cause a problem.

The options here are either:

• GitHub API accepts a value, maybe 0 or -1, for actor_id when actor_type is set to OrganizationAdmin. Whether this happens or not, it'd be cool if the API docs have these quirks explained since this is a restriction that wasn't really mentioned anywhere.
• go-github doesn't use omitempty when serializing this field.

Either way, before this is fixed here, one of the two options must happen since each requires a minor change here for it to work.

[YElyousfi]

• GitHub API accepts a value, maybe 0 or -1, for actor_id when actor_type is set to OrganizationAdmin. Whether this happens or not, it'd be cool if the API docs have these quirks explained since this is a restriction that wasn't really mentioned anywhere.

So this was the case but the actor_id needs to be 1 for OrganizationAdmin

Not sure why it's not documented on the API side but it should be added to the provider docs on the field or even simply as an example of resource usage

[o-sama]

Interesting, I'll add that to the docs. Thanks @YElyousfi!

[lewismiddleton]

I think there are some similar magic numbers in the RepositoryRole actor_ids. Adding bypassers through the UI and then importing the terraform resource I found these corresponding IDs:

role_to_actor_id_map = {
  Maintain          = 2,
  Write             = 4,
  RepositoryAdmin   = 5,
  OrganisationAdmin = 1
}

It's also worth mentioning that if you try to create a bypass_actor with id = 3 this fails at apply time with a HTTP 500 error. Would be nice to catch this earlier during a plan.

Although I don't know if this project is comfortable enough assuming that these magic numbers will stay constant from Github. I imagine this would cause funky behaviour if Github shifted these under you. Probably one for the maintainers to make a call on. @kfcampbell