fix: handle sha_pinning_required=false#3224
fix: handle sha_pinning_required=false#3224sheeeng wants to merge 1 commit intointegrations:mainfrom
sha_pinning_required=false#3224Conversation
|
👋 Hi! Thank you for this contribution! Just to let you know, our GitHub SDK team does a round of issue and PR reviews twice a week, every Monday and Friday! We have a process in place for prioritizing and responding to your input. Because you are a part of this community please feel free to comment, add to, or pick up any issues/PRs that are labeled with |
0dee61f to
3067dd7
Compare
sha_pinning_required=falsesha_pinning_required = false
sha_pinning_required = falsesha_pinning_required=false
github/resource_github_actions_organization_permissions_test.go
Outdated
Show resolved
Hide resolved
github/resource_github_actions_organization_permissions_test.go
Outdated
Show resolved
Hide resolved
3067dd7 to
fd7ac91
Compare
deiga
left a comment
There was a problem hiding this comment.
LGTM! @stevehipwell WDYT?
stevehipwell
left a comment
There was a problem hiding this comment.
@sheeeng I thought the whole point here was that SHA pinning was dependent on organization configuration? This change would not fix the issue, but would break this resource.
I'm not sure how I missed this in review, but I'll repeat my suggestion to look at how allow_forking works in the repository resource. To fix this you should split the create and update functions in the resource to simplify the logic. Then in create you can use d.GetOkExists() to see if it should be sent. In update you need to check d.Changed() and if it has changed you need to use d.Get() and then send that value.
fef996b to
5b44ee6
Compare
deiga
left a comment
There was a problem hiding this comment.
Partial review. Apply comments suggestions to all applicable sections
d371f61 to
c3fc78b
Compare
c3fc78b to
fe4dd69
Compare
deiga
left a comment
There was a problem hiding this comment.
The code looks like you're using an AI to generate it and it's lost all notion of what is supposed to happen.
Please go through the code yourself and understand for yourself what it should be doing.
If the next time I come to review this and you're still showing disregard for the comments that have been left earlier, then I'll close the PR.
It's not worth anybody's time for us to play Broken Telephone from us to your AI.
7ca2562 to
bb27f7a
Compare
|
I did a rebase from default branch. The pull request is updated with a force-push. |
66ddbf2 to
6ffd608
Compare
Split CreateOrUpdate into separate Create and Update functions for both `github_actions_organization_permissions` and `github_actions_repository_permissions` resources. In Create, use `GetOkExists` instead of `GetOk` so that an explicit `sha_pinning_required = false` is sent to the API rather than silently dropped. In Update, use `HasChange` + `Get` which avoids the zero-value problem entirely. Fix integrations#3223. Signed-off-by: Leonard Sheng Sheng Lee <305414+sheeeng@users.noreply.github.com>
6ffd608 to
9c72a33
Compare
Split
CreateOrUpdateinto separateCreateandUpdatefunctions for bothgithub_actions_organization_permissionsandgithub_actions_repository_permissionsresources.In
Create, replaced.GetOk()withd.GetOkExists()forsha_pinning_required. The field is Optional and Computed, soGetOkreturns(false, false)when a user explicitly writessha_pinning_required = false, which is indistinguishable from omitting the field.GetOkExistsreturns(false, true), correctly detecting the explicit value.In
Update, used.HasChange()+d.Get(), which avoids the zero-value problem entirely by checking the state diff.Add two-step acceptance tests (true → false) to verify the fix.
Fix #3223.
Before the change?
sha_pinning_required = falseapplied successfully in Terraform, but not actually changed in GitHub resources itself.After the change?
sha_pinning_required = falseapplied successfully in Terraform, and actually changed in GitHub resources itself.Pull request checklist
Does this introduce a breaking change?
Please see our docs on breaking changes to help!