Skip to content

fix: handle sha_pinning_required=false#3224

Open
sheeeng wants to merge 1 commit intointegrations:mainfrom
sheeeng:fix/sha-pinning-required-false-ignored
Open

fix: handle sha_pinning_required=false#3224
sheeeng wants to merge 1 commit intointegrations:mainfrom
sheeeng:fix/sha-pinning-required-false-ignored

Conversation

@sheeeng
Copy link
Copy Markdown
Contributor

@sheeeng sheeeng commented Feb 25, 2026

Split CreateOrUpdate into separate Create and Update functions for both github_actions_organization_permissions and github_actions_repository_permissions resources.

In Create, replace d.GetOk() with d.GetOkExists() for sha_pinning_required. The field is Optional and Computed, so GetOk returns (false, false) when a user explicitly writes sha_pinning_required = false, which is indistinguishable from omitting the field. GetOkExists returns (false, true), correctly detecting the explicit value.

In Update, use d.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?

  • Setting sha_pinning_required = false applied successfully in Terraform, but not actually changed in GitHub resources itself.

After the change?

  • Setting sha_pinning_required = false applied successfully in Terraform, and actually changed in GitHub resources itself.

Pull request checklist

  • Schema migrations have been created if needed (example)
  • Tests for the changes have been added (for bug fixes / features)
  • Docs have been reviewed and added / updated if needed (for bug fixes / features)

Does this introduce a breaking change?

Please see our docs on breaking changes to help!

  • Yes
  • No

@github-actions
Copy link
Copy Markdown

👋 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 Status: Up for grabs. You & others like you are the reason all of this works! So thank you & happy coding! 🚀

@github-actions github-actions bot added the Type: Bug Something isn't working as documented label Feb 25, 2026
@sheeeng sheeeng force-pushed the fix/sha-pinning-required-false-ignored branch from 0dee61f to 3067dd7 Compare February 25, 2026 09:38
@sheeeng sheeeng changed the title fix: handle sha_pinning_required=false fix: handle sha_pinning_required = false Feb 25, 2026
@sheeeng sheeeng changed the title fix: handle sha_pinning_required = false fix: handle sha_pinning_required=false Feb 25, 2026
@sheeeng sheeeng force-pushed the fix/sha-pinning-required-false-ignored branch from 3067dd7 to fd7ac91 Compare February 26, 2026 16:08
@sheeeng sheeeng requested a review from deiga February 26, 2026 16:10
Copy link
Copy Markdown
Collaborator

@deiga deiga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! @stevehipwell WDYT?

Copy link
Copy Markdown
Collaborator

@stevehipwell stevehipwell left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@sheeeng sheeeng force-pushed the fix/sha-pinning-required-false-ignored branch 15 times, most recently from fef996b to 5b44ee6 Compare February 28, 2026 09:14
Copy link
Copy Markdown
Collaborator

@deiga deiga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review. Apply comments suggestions to all applicable sections

@sheeeng sheeeng force-pushed the fix/sha-pinning-required-false-ignored branch 2 times, most recently from d371f61 to c3fc78b Compare March 2, 2026 17:24
@sheeeng sheeeng force-pushed the fix/sha-pinning-required-false-ignored branch from c3fc78b to fe4dd69 Compare March 2, 2026 17:24
@sheeeng sheeeng requested a review from deiga March 2, 2026 17:27
Copy link
Copy Markdown
Collaborator

@deiga deiga left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@sheeeng sheeeng marked this pull request as draft March 3, 2026 00:22
@sheeeng sheeeng marked this pull request as ready for review March 3, 2026 15:15
@sheeeng sheeeng marked this pull request as draft April 10, 2026 10:55
@sheeeng sheeeng force-pushed the fix/sha-pinning-required-false-ignored branch from 7ca2562 to bb27f7a Compare April 10, 2026 12:11
@sheeeng sheeeng marked this pull request as ready for review April 10, 2026 12:14
@sheeeng
Copy link
Copy Markdown
Contributor Author

sheeeng commented Apr 10, 2026

I did a rebase from default branch. The pull request is updated with a force-push.

@sheeeng sheeeng force-pushed the fix/sha-pinning-required-false-ignored branch 2 times, most recently from 66ddbf2 to 6ffd608 Compare April 13, 2026 05:54
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>
@sheeeng sheeeng force-pushed the fix/sha-pinning-required-false-ignored branch from 6ffd608 to 9c72a33 Compare April 13, 2026 06:05
@deiga deiga added this to the v6.13.0 Release milestone Apr 14, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Type: Bug Something isn't working as documented

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[BUG]: sha_pinning_required = false is silently ignored due to d.GetOk() zero-value bug

3 participants