Skip to content

fix: include configured false booleans on organization_settings create#3570

Draft
somaz94 wants to merge 2 commits into
integrations:mainfrom
somaz94:fix/org-settings-create-includes-false-bool
Draft

fix: include configured false booleans on organization_settings create#3570
somaz94 wants to merge 2 commits into
integrations:mainfrom
somaz94:fix/org-settings-create-includes-false-bool

Conversation

@somaz94

@somaz94 somaz94 commented Jul 23, 2026

Copy link
Copy Markdown

Resolves #3493


Before the change?

On create, github_organization_settings dropped any boolean attribute
explicitly set to false. buildOrganizationSettings gated every field
through shouldInclude, which on the create path used d.GetOk(name).
GetOk returns ok=false for a TypeBool whose value is false, so it
cannot tell an explicit false from an unset field. The attribute was omitted
from the create PATCH and GitHub applied its own default. A second apply
fixed it because the update path uses d.HasChange.

After the change?

On create, shouldInclude now always includes schema.TypeBool attributes.
Every bool in this resource declares a schema Default, so it always has a
definite value (there is no meaningful "unset"), and d.Get(name).(bool)
(already used by the bool setters) sends the configured value correctly,
including false. Non-boolean fields are unchanged (still included only when
configured), and the update path is untouched.

Note: since these bools declare a schema default, an unconfigured bool now also
sends its default explicitly on create. That value equals the schema default,
so the resulting state is identical, just made explicit.

Pull request checklist

  • Schema migrations have been created if needed → N/A (no schema change).
  • Tests for the changes have been added (for bug fixes / features) → new
    github/resource_github_organization_settings_unit_test.go with 5 unit
    tests (create includes false/true bool, unset string still omitted,
    enterprise-only bool gating, update path still gates on HasChange). Runs
    under make test with no org credentials. The false-bool test fails on
    current main and passes with this fix.
  • Docs have been reviewed and added / updated if needed → N/A (no
    user-facing schema/docs change; the buildOrganizationSettings doc
    comment was updated to match the new behavior).

Does this introduce a breaking change?

  • Yes
  • No

AI assistance disclosure: this PR was largely written with an AI coding
assistant (Claude Code, Opus 4.8). I reviewed and validated it locally:
gofmt, go build ./github/, golangci-lint run ./github/..., and
go test ./github/ -run TestBuildOrganizationSettings all pass.

@github-actions

Copy link
Copy Markdown

👋 Hi, and thank you for this contribution!

This repo is maintained by GitHub and community members on a best-effort basis. We'll get to this as soon as we can.

You can help us prioritize by joining the discussion on open issues and PRs, sharing details on the changes you need, and reviewing other contributions.


🤖 This is an automated message.

@github-actions github-actions Bot added the Type: Bug Something isn't working as documented label Jul 23, 2026

@deiga deiga left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Nice work!

Could you refactor the unit tests to use a single testing function with a table for test cases?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please don't create a separate unit test file. Add the unit tests to the existing test file instead ☺️

// attribute explicitly configured as false is included in the create payload.
// Regression test for the create path dropping false booleans (only fixed by a
// second apply through the update/HasChange path).
func TestBuildOrganizationSettingsCreateIncludesFalseBool(t *testing.T) {

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please use an underscore to separate function under test and the testcase in the name

@somaz94
somaz94 force-pushed the fix/org-settings-create-includes-false-bool branch from 20e8bf1 to 8cead3a Compare July 24, 2026 02:48
@somaz94

somaz94 commented Jul 24, 2026

Copy link
Copy Markdown
Author

@deiga Thanks for the review — all three points addressed in 8cead3a:

  • No separate file — deleted resource_github_organization_settings_unit_test.go; the tests now live at the bottom of the existing resource_github_organization_settings_test.go.
  • Single table-driven function — the five separate test functions are now one Test_buildOrganizationSettings with a 6-case table, following the same inline-anonymous-slice idiom as Test_configureProviderMeta in provider_test.go. Splitting the old enterprise test into two rows (enterprise_… / non_enterprise_…) made that case clearer too.
  • Underscore namingTest_buildOrganizationSettings matches the repo's Test_<functionUnderTest> convention, and the case names use underscores (create_includes_explicitly_false_booleans, update_omits_unchanged_booleans, …).

I also verified the table still has teeth: temporarily reverting the shouldInclude bool branch makes exactly create_includes_explicitly_false_booleans and enterprise_create_includes_internal_repositories_boolean fail, and restoring it turns them green.

Rebased on latest main as well, so the branch is no longer behind. go vet / gofmt / the unit tests are all clean.

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]: github_organization_settings silently ignores boolean attributes set to false on create (GetOk zero-value bug in shouldInclude)

2 participants