fix: include configured false booleans on organization_settings create

[somaz94]

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]

👋 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.}

[deiga]

Nice work!

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

[deiga]

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

[somaz94]

Done in 8cead3a — this file is deleted. The unit tests now live at the bottom of the existing github/resource_github_organization_settings_test.go.

[deiga]

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

[somaz94]

Done in 8cead3a — the five separate funcs are now a single table-driven Test_buildOrganizationSettings, following the repo's existing Test_getBaseURL / Test_configureProviderMeta convention. Testcase names use underscores (e.g. create_includes_explicitly_false_booleans, enterprise_create_includes_internal_repositories_boolean).

[somaz94]

@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 naming — Test_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.

[deiga]

Thanks for the work!

[deiga]

suggestion: There is little benefit on using shouldInclude for boolean fields, if it will just return true. What id you instead remove the call to shouldInclude from the boolean fields?

[somaz94]

Done in cbe668a8, with one wrinkle worth flagging.

Removing the call outright would have changed the update path: shouldInclude returns true for booleans only on create — on update it still falls through to d.HasChange(fieldName), which is what keeps unchanged fields out of the payload and avoids the API validation errors the function comment mentions.

So I split it in two instead. The schema lookup and the TypeBool special case are gone, and the boolean fields no longer call shouldInclude:

// Non-boolean fields are included only when explicitly configured on create,
// and only when changed on update.
shouldInclude := func(fieldName string) bool {
	if isUpdate {
		return d.HasChange(fieldName)
	}
	_, ok := d.GetOk(fieldName)
	return ok
}

// Boolean attributes always carry a definite value from their schema
// default, and d.GetOk cannot tell an explicit false from an unset field,
// so on create they are included unconditionally.
includeBool := func(fieldName string) bool {
	return !isUpdate || d.HasChange(fieldName)
}

Happy to collapse it further if you'd rather see something else.

[deiga]

@stevehipwell do I remember correctly that we want to remove all unnecessary partial updates?
In that case the whole shouldInclude function wpuld be unnecessary

[somaz94]

@deiga — one data point that might make this cheaper to decide than it looks: shouldInclude is used in exactly one place in the repo. It's defined in this file and called 27 times, all inside the same function. So removing it entirely would be a single-resource change, not a cross-provider refactor.

I also went looking for the broader rule you're recalling and couldn't find it written down anywhere — no issue, PR or CONTRIBUTING note about removing partial updates. The closest thing is #3429, which is the bug this PR fixes.

stevehipwell hasn't been active since Jul 27, so rather than hold this on his answer: if you'd like the full-payload version, say the word and I'll push it in this PR. If you'd rather keep the current includeBool split and treat removing shouldInclude as its own change, that works too. Happy either way — I just don't want to guess and build the wrong one.

Separately, and regardless of the above: CI and CodeQL are sitting at action_required on ecd0631f and have never run on this branch, so the green checkmarks are only the pull_request_target workflows. The Go tests haven't actually executed here yet. If you approve the run we'd both be looking at real results.

Happy to rebase too — it's one commit behind.

[somaz94]

@deiga @stevehipwell — happy to go either way on this, just let me know which you prefer so I do not build the wrong thing.

Option A (this PR as it stands): keep shouldInclude, but stop it from dropping explicitly-configured false booleans on create. Narrow, low-risk, fixes #3429.

Option B (what @deiga raised): remove shouldInclude entirely and always send the full set of fields on create. That is a superset of A — it fixes the same false-boolean bug as a side effect — but it is a broader behavioural change and touches every field, not just the booleans.

I am glad to do B in this PR if that is the agreed direction. The only thing blocking me is knowing which one you want.

For what it is worth, the three earlier review threads are already addressed (8cead3a, cbe668a8, ecd0631f) — they are just still showing unresolved.

[lloydmcl]

I hit this on a first apply against a new organisation, so a data point in favour of fixing it, plus one thing worth weighing before shouldInclude comes out entirely.

The partial payload on create is load-bearing. It arrived in #2807 to fix #2305, where the resource returned a 422 on every apply for EMU organisations because unconfigured fields were being sent. Dropping shouldInclude would send the full field set on create again, which is the shape that caused #2305 in the first place.

There is also a live conflict with #3360. That PR fixes #2689, a 422 when an enterprise policy locks forking, by removing Default: false from members_can_fork_private_repositories so the attribute is only sent when it is explicitly configured. includeBool sends it on every create regardless, so whichever of the two lands second undoes the other. The rationale that booleans always carry a definite value from their schema default also stops holding for that attribute once #3360 removes its default.

Option 1 in #3493 sidesteps both. Testing the raw configuration for a null attribute distinguishes a configured false from an unset one without widening the create payload at all, so it fixes this bug and stays compatible with #3360.

[somaz94]

Thanks for digging into this — the #3360 point is a fair catch and I'd missed it. includeBool does send members_can_fork_private_repositories on every create, which is the shape that produces the 422 in #2689, and my comment's premise about booleans always carrying a definite schema default stops holding for that attribute once #3360 makes it Computed.

On which way to fix it, I don't think I should pick on my own. @deiga suggested the opposite direction on 28 Jul — dropping the shouldInclude call from the boolean fields — and then asked @stevehipwell on 29 Jul whether the project wants to remove unnecessary partial updates entirely, which would remove shouldInclude altogether. That question hasn't been answered, and I think it's what this PR is actually waiting on.

Either answer is small work from here. If partial updates stay, I'll switch the create check to a raw-config null test (the sample patch in #3493), which fixes the explicit false without widening the create payload and stays compatible with #3360. If they go, this branch is already most of the way there and I'll drop shouldInclude from the non-boolean fields too.

@stevehipwell @deiga — could I get a steer? Happy to re-cut either way.