Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
1 task done
stempler opened this issue Jan 13, 2025 · 7 comments · May be fixed by #2538
Open
1 task done

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

stempler opened this issue Jan 13, 2025 · 7 comments · May be fixed by #2538
Labels
Status: Triage This is being looked at and prioritized Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR

Comments

@stempler
Copy link

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

  • I agree to follow this project's Code of Conduct
@stempler stempler added Status: Triage This is being looked at and prioritized Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR labels Jan 13, 2025
@stempler stempler linked a pull request Jan 14, 2025 that will close this issue
4 tasks
@stevehipwell
Copy link
Contributor

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

@Danielku15
Copy link
Contributor

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

Image

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

Image

@stevehipwell
Copy link
Contributor

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

@Danielku15
Copy link
Contributor

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 #2317

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

@stevehipwell
Copy link
Contributor

@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
Copy link
Contributor

@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
Copy link
Contributor

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 google/go-github#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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Status: Triage This is being looked at and prioritized Type: Maintenance Any dependency, housekeeping, and clean up Issue or PR
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants