-
Notifications
You must be signed in to change notification settings - Fork 904
fix: Fix org roles implementation #2968
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
base: main
Are you sure you want to change the base?
Conversation
4dd960f to
43c7339
Compare
deiga
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Would you be able to add a test case which reproduces the error from the reported issue?
| Optional: true, | ||
| Computed: true, | ||
| ValidateDiagFunc: validateValueFunc([]string{"read", "triage", "write", "maintain", "admin"}), | ||
| Required: true, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
question: GH API doesn't mark this as required, if you found that it IS required, could you add a comment here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue on this covers the error. Note that the GitHub REST API docs aren't great, especially in cases like this where the spec has changed.
This change is intentional, if none had been an option when I created the apartment for this resource this would have been the design I'd have chosen. Basically optional+ computed is best avoided if possible (which it often isn't). But if it works in some cases without this being set then this would be a major version change so I can change the implementation. I'm hoping the automation testing PRs are merged soon so this can be tested.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
OK, I've changed the implementation to have a default as this will be non-breaking. Based on the previous logic and the reported error it looks like the implementation broke when the API swapped out "" for "none".
I've added extra test coverage and I've tested this manually using the API/UI, but as this is an enterprise feature I can't directly run the unit tests.
| Computed: true, | ||
| ValidateDiagFunc: validateValueFunc([]string{"read", "triage", "write", "maintain", "admin"}), | ||
| Required: true, | ||
| ValidateDiagFunc: validateValueFunc([]string{"none", "read", "triage", "write", "maintain", "admin"}), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
issue: There doesn't seem to be an option for none in https://docs.github.com/en/enterprise-cloud@latest/rest/orgs/organization-roles?apiVersion=2022-11-28#create-a-custom-organization-role
But is this an oversight on GH API docs?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, read the description text above.
@deiga there is already a test for the org role error, that worked prior to the API changing. The other error is a distributed system error that can't easily be tested, as it'd be proving a negative; but we do have tests. We've seen the same problem in other areas in the API and need to get rid of the re-read pattern. Interestingly the resource failing here is having to use a list for get pattern as the get API was a later addition and needs v68. Also interesting is that at least one report is from GHES. |
43c7339 to
c8e5b2b
Compare
Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>
c8e5b2b to
61057bb
Compare
Resolves #2805
Resolves #2960
This PR improves on the code in #2487.
Before the change?
github_organization_roledidn't correctly support org only permissionsAfter the change?
github_organization_rolecan support org only permissions or no permissionsPull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!