fix: include configured false booleans on organization_settings create by somaz94 · Pull Request #3570 · integrations/terraform-provider-github

somaz94 · 2026-07-23T05:18:47Z

Resolves #3493

Before the change?

On create, a boolean explicitly set to false was dropped from the payload. Inclusion was decided with d.GetOk, which reports false the same as unset, so GitHub kept its own value and only a second apply (through the HasChange update path) converged.

After the change?

Create sends an attribute when it is written in the configuration (read from the raw config, so an explicit false counts) or when its schema default is non-zero, which the create payload already contained before. Unset attributes that default to false/"" still stay out of the request, so the narrow payload from #2807 (#2305) is preserved. Update is unchanged: only changed attributes are sent.

Following review, create and update are separate CRUD functions with the logic inline, neither calls Read, logging uses tflog, and the raw-config check lives in util.go as isConfigured.

Pull request checklist

Schema migrations have been created if needed → N/A (no schema change).
Tests for the changes have been added → acceptance case sends booleans configured as false on create (explicit false on five default-true booleans, then a plan-only step that must be empty). TestAccGithubOrganizationSettings is still skipped by the existing t.Skip("TODO: Make this test cleanup correctly"), so it does not run in CI, and I have not run it against a live org.
Docs have been reviewed and added / updated if needed → N/A (no schema or docs change).

Does this introduce a breaking change?

Yes
No

AI assistance disclosure: this PR was largely written with an AI coding assistant (Claude Code). I reviewed it and validated it locally with gofmt, go build ./..., go vet ./github/, go test ./github/, and golangci-lint with .golangci.new.yml (0 new issues). The acceptance test has not been run against a live organization.

github-actions · 2026-07-23T05:18:55Z

👋 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 · 2026-07-23T08:58:16Z

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

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 · 2026-07-23T08:59:02Z

+// 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) {


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

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 · 2026-07-24T02:49:14Z

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

somaz94 · 2026-07-30T03:51:45Z

@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 · 2026-08-30T04:33:21Z

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 · 2026-08-31T02:06:46Z

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.

stevehipwell

For me before any other changes are made the helper needs removing and the code needs to be put back into create/update. Then the API surface needs to be correctly designed to support the limitations of Terraform (SDK v2), as once a field is set there is no way to check if was done so by the user or a read.

FYI for boolean schema values Get should be used as getOk always returns ok as true.

somaz94 · 2026-09-08T01:53:36Z

Thanks @stevehipwell — that's the steer I was after. One thing I want to pin down before re-cutting: with shouldInclude gone, should create send the full field set? That's the shape #2807 narrowed to fix #2305, and @lloydmcl flagged it would also collide with #3360 on members_can_fork_private_repositories. If you'd rather keep the payload narrow I'll use the raw-config null check from #3493 instead, and either way I'll split resourceGithubOrganizationSettingsCreateOrUpdate so create and update stop sharing a helper that re-detects the operation from d.Id().

stevehipwell · 2026-09-08T08:47:40Z

@somaz94 if a field is optional and can be overridden at a higher layer we shouldn't set/read it if the user hasn't explicitly set it.

…n test

… settings

somaz94 · 2026-09-10T02:33:23Z

Done. The helper is gone and create and update are separate handlers again.

Inclusion is now decided from the raw config rather than GetOk, so an attribute is only sent when the user actually wrote it. Unconfigured fields stay out of the create payload the way #2305 needs, and a boolean explicitly set to false now reaches the API instead of being silently dropped (#3493).

That also means members_can_fork_private_repositories is only sent when it's configured, so it no longer collides with #3360.

Boolean values are read with d.Get.

I split it into a refactor commit and the fix so the two read separately.

Copilot

🟡 Changes recommended

Create now omits schema-defaulted settings, risking first-apply drift, and lacks enabled acceptance coverage.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

These provider review instructions are being used. This PR fixes false boolean handling when creating organization settings and modernizes CRUD handlers.

Changes:

Detects explicitly configured values through raw configuration.
Separates create/update payload builders.
Adds unit coverage for payload construction.

File summaries

File	Description
`github/resource_github_organization_settings.go`	Refactors CRUD and payload construction.
`github/resource_github_organization_settings_test.go`	Adds create/update payload tests.

Review details

Files reviewed: 2/2 changed files
Comments generated: 2
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

+	}
+}
+
+func Test_organizationSettingsForCreate(t *testing.T) {


…e payload

somaz94 · 2026-09-10T06:40:06Z

Copilot's first point is right, fixed in bb3b9c1. Create now also sends an attribute when its schema default is a non-zero value, which is what the GetOk path already did before this PR, so the create payload is unchanged apart from explicitly configured false values now reaching it. Attributes defaulting to false or an empty string are still only sent when configured, so nothing new is sent to EMU orgs. The unit case for an unset default-true attribute now expects it to be sent, and the test helper fills schema defaults into the attributes the way the planned state does.

stevehipwell

@somaz94 there are still too many abstractions in this logic, please refactor back to the CRUD functions and remove all but the obvious abstractions. For example if the logic for getting the payload for create and update are different then there shouldn't be a common function, it should be inline.

And also RE tests, please add acceptance tests and not unit tests.

stevehipwell · 2026-09-10T09:13:40Z

+// stringAttr returns the string value of the named attribute. The resource
+// schema guarantees the type, so a failed assertion can only mean the schema
+// and this code disagree, in which case the zero value is the safe result.
+func stringAttr(d *schema.ResourceData, name string) string {
+	v, _ := d.Get(name).(string)
+
+	return v
+}
+
+// boolAttr returns the boolean value of the named attribute, with the same
+// type guarantee as stringAttr.
+func boolAttr(d *schema.ResourceData, name string) bool {
+	v, _ := d.Get(name).(bool)
+
+	return v
+}


These are unnecessary and make the code harder to read, please remove them and keep the logic inline.

stevehipwell · 2026-09-10T09:15:11Z

+// isConfigured reports whether the named attribute is explicitly set in the
+// configuration, as opposed to merely carrying its schema default.
+//
+// d.GetOk cannot answer this for booleans: it reports a value configured as
+// false exactly like an unset one, which is why an explicit false used to be
+// dropped from the create payload (#3493). The raw configuration keeps the
+// distinction, so a null there means the user did not write the attribute.
+//
+// The raw configuration is absent on some code paths, such as import. In that
+// case there is nothing to read the user's intent from, so this falls back to
+// the previous d.GetOk behaviour rather than inventing one.
+func isConfigured(d *schema.ResourceData, name string) bool {
+	if d.GetRawConfig().IsNull() {
+		_, ok := d.GetOk(name)
+
+		return ok
+	}
+
+	v, diags := d.GetRawConfigAt(cty.GetAttrPath(name))
+	if diags.HasError() {
+		_, ok := d.GetOk(name)
+
+		return ok
+	}
+
+	return !v.IsNull()
+}


If this is a reusable helper then it should be in one of the utils files.

deiga · 2026-09-10T10:10:38Z

-	// Debug: Log the settings being sent to the API with detailed field information
+// logOrganizationSettings records the payload about to be sent to the API.
+func logOrganizationSettings(org string, isEnterprise bool, settings *github.Organization) {
+	log.Printf("[DEBUG] Built settings for org %s (enterprise: %v)", org, isEnterprise)


Please use tflog instead of log

deiga · 2026-09-10T10:11:27Z

-	return resourceGithubOrganizationSettingsRead(d, meta)
+	d.SetId(strconv.FormatInt(orgSettings.GetID(), 10))
+
+	return resourceGithubOrganizationSettingsRead(ctx, d, owner)


Never call a CRUD function

…he CRUD functions

somaz94 · 2026-09-11T02:26:59Z

Done: the logic is back inline in create/update, the helpers are gone apart from isConfigured, which moved to util.go since it's generic, logging uses tflog, and create/update no longer call Read. The unit tests are replaced by an acceptance test (explicit false on default-true booleans, then an empty plan).

One question: the whole TestAccGithubOrganizationSettings suite is still behind the existing t.Skip("TODO: Make this test cleanup correctly"), so the new case won't run in CI. Should I move it into its own enabled test, or leave the suite as is?

stevehipwell · 2026-09-11T15:19:20Z

@somaz94 ideally TestAccGithubOrganizationSettings needs fixing so it cleans up correctly, all resources tests should be grouped under this function.

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

deiga requested changes Jul 23, 2026

View reviewed changes

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

somaz94 force-pushed the fix/org-settings-create-includes-false-bool branch from 8cead3a to 0f86150 Compare July 28, 2026 06:17

somaz94 marked this pull request as ready for review July 28, 2026 06:17

deiga requested changes Jul 28, 2026

View reviewed changes

Comment thread github/resource_github_organization_settings.go

Comment thread github/resource_github_organization_settings_test.go Outdated

deiga added the Needs Discussion This issue/PR needs maintainers to discuss and decide on a course of action. label Aug 31, 2026

stevehipwell requested changes Sep 7, 2026

View reviewed changes

somaz94 added 6 commits September 10, 2026 11:26

fix: include configured false booleans on organization_settings create

46f30cf

test: consolidate organization_settings unit tests into a table-drive…

9f1591a

…n test

refactor: drop shouldInclude call from boolean fields in organization…

74911e9

… settings

test: compare built organization settings payload with cmp.Diff

856c3ef

refactor: split organization settings create and update handlers

7cb2e6c

fix: decide organization_settings create payload from the raw config

fee6498

somaz94 force-pushed the fix/org-settings-create-includes-false-bool branch from ecd0631 to fee6498 Compare September 10, 2026 02:28

deiga requested a balanced review from Copilot September 10, 2026 06:01

Copilot started reviewing on behalf of deiga September 10, 2026 06:01 View session

Copilot AI reviewed Sep 10, 2026

View reviewed changes

Comment thread github/resource_github_organization_settings.go Outdated

Comment thread github/resource_github_organization_settings_test.go Outdated

}

}

func Test_organizationSettingsForCreate(t *testing.T) {

fix: keep non-zero schema defaults in the organization_settings creat…

bb3b9c1

…e payload

stevehipwell requested changes Sep 10, 2026

View reviewed changes

deiga requested changes Sep 10, 2026

View reviewed changes

somaz94 added 2 commits September 11, 2026 11:25

test: replace organization_settings unit tests with an acceptance test

ffd139b

refactor: inline organization_settings create and update logic into t…

d8d8e24

…he CRUD functions

Uh oh!

Conversation

somaz94 commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Before the change?

After the change?

Pull request checklist

Does this introduce a breaking change?

Uh oh!

github-actions Bot commented Jul 23, 2026

Uh oh!

deiga left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

somaz94 commented Jul 24, 2026

Uh oh!

deiga left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

somaz94 commented Jul 30, 2026

Uh oh!

lloydmcl commented Aug 30, 2026

Uh oh!

somaz94 commented Aug 31, 2026

Uh oh!

stevehipwell left a comment

Choose a reason for hiding this comment

Uh oh!

somaz94 commented Sep 8, 2026

Uh oh!

stevehipwell commented Sep 8, 2026

Uh oh!

somaz94 commented Sep 10, 2026

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

somaz94 commented Sep 10, 2026

Uh oh!

stevehipwell left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

somaz94 commented Sep 11, 2026

Uh oh!

stevehipwell commented Sep 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

somaz94 commented Jul 23, 2026 •

edited

Loading

stevehipwell left a comment •

edited

Loading