[MAINT]: actor_id for OrganizationAdmin seems to have changed from 1 to 0

[stempler]

Describe the need

Since a couple of days I'm getting changes shown on terraform plan related to rulesets that use the actor_type OrganizationAdmin in the bypass actors.

For example:

...

      ~ bypass_actors {
          ~ actor_id    = 0 -> 1
            # (2 unchanged attributes hidden)
        }

        # (7 unchanged blocks hidden)
    }

Plan: 0 to add, 110 to change, 0 to destroy.

It seems the actor_id used internally in Github for OrganizationAdmin changed from 1 to 0.

Changing my configurations from 1 to 0 again leads to "no changes" detected as expected. Applying with the 1 still present did not lead to any errors, so I would assume this is not a breaking change related to creating/updating this kind of resources.

I guess for this project the main impact would be that the documentation would need to be updated to refer to 0 instead of 1.

SDK Version

No response

API Version

No response

Relevant log output

No response

Code of Conduct

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

[stevehipwell]

I added a comment to https://github.com/github/rest-api-description/issues/4406 as this could be a regression as the GitHub API docs still explicitly say to use 1.

[Danielku15]

We're on GHES 3.15.1 within my work place. After creating a ruleset manually with OrganizationAdmin added it has "actor_id": 1.

I just did the same on github.com in one of my repos, and there it has "actor_id": null.

[stevehipwell]

FYI @Danielku15 null will be represented as 0 in a Go int.

[Danielku15]

Yes, I've noticed that in some places the null values are represented as 0 in the models for simplicity. But if not handled correctly on the api calls, it can lead to bugs like https://github.com/integrations/terraform-provider-github/issues/2317

Null and 0 can have different semantics on the github api leading to validation errors when sending the wrong data.

[stevehipwell]

@Danielku15 in this context if null is being treated as anything other than 0 then it's a bug as the GH Go SDK treats it as an int which can't be represented as null.

[Danielku15]

@stevehipwell True, in this context things should be fine. It appears also for me #2317 disappeared on our GHES after upgrade to 3.15. In my previous tests and troubleshooting it appeared that a 0 value on integration_id somehow sneaked through some checks and was ultimately sent to the API.

GH Go SDK uses *int64 to differenciate between null and 0 values, hence setting 0 without a check when mapping from Terraform structures to API structures can lead to problems.

[stevehipwell]

GH Go SDK uses *int64 to differenciate between null and 0 values, hence setting 0 without a check when mapping from Terraform structures to API structures can lead to problems.

@Danielku15 you're right, I've been working in both the SDK and TF provider so my memory was slightly off. I'm planning on re-implementing the ruleset code once https://github.com/google/go-github/pull/3430 has been merged & released. One of the things I'm planning on doing is supporting not requiring the actor ID for well known actors.

[fatbasstard]

Seeing the same here:

~ bypass_actors {
  ~ actor_id :0 -> 5
  ~ actor_type : "OrganizationAdmin" -> "RepositoryRole"
  bypass_mode : "always"
  }
~ bypass_actors {
  ~ actor_id : 5 -> 1
  ~ actor_type : "RepositoryRole" -> "OrganizationAdmin"
  bypass_mode : "always"
  }

It's even a bit more annoying since we also set the RepositoryRole to Admin (source) which seems to flip over the configuration even worse.

[sthompson0]

This makes noisy output in terraform plans that confuses end users. Any chance this will be fixed in the near future? https://github.com/google/go-github/pull/3430 has been merged. Has it been released yet?

[sthompson0]

@stevehipwell Is setting the actor_id to 0 an appropriate local fix, or will this end up causing a bug later if this is ever addressed?

[stevehipwell]

@sthompson0 this is what we've been doing since January, AFAIK there isn't another solution to this.

[deiga]

My current suggestion is to "swallow" the 0 value inside the provider and just keep acting like it's 1. See: https://github.com/integrations/terraform-provider-github/pull/2976/commits/9d22505853a851df0bb542e4b5e616602a3a6304

[stevehipwell]

@deiga we'd need to make this support both 0 & 1 as I'd expect most people to have switched over to using 0 (we have).

[deiga]

@deiga we'd need to make this support both 0 & 1 as I'd expect most people to have switched over to using 0 (we have).

@stevehipwell That's a good point! I'll check how to do that reasonably

[stevehipwell]

@deiga the simplest pattern would be to never swap 1 & 0 in state (e.g. if the current value is <= 1, either from user input or API response, then only update it if the new value is > 1). A custom diff suppression could be used but would be more complex and may have unintended consequences in the future.