From 18ff1d08181858d4f23451614a63f28c67c994e0 Mon Sep 17 00:00:00 2001 From: somaz Date: Thu, 23 Jul 2026 14:14:12 +0900 Subject: [PATCH 01/11] fix: include configured false booleans on organization_settings create --- .../resource_github_organization_settings.go | 26 ++-- ..._github_organization_settings_unit_test.go | 125 ++++++++++++++++++ 2 files changed, 143 insertions(+), 8 deletions(-) create mode 100644 github/resource_github_organization_settings_unit_test.go diff --git a/github/resource_github_organization_settings.go b/github/resource_github_organization_settings.go index b8ba89060c..6bbe091ad8 100644 --- a/github/resource_github_organization_settings.go +++ b/github/resource_github_organization_settings.go @@ -173,8 +173,12 @@ func resourceGithubOrganizationSettings() *schema.Resource { } } -// buildOrganizationSettings creates a github.Organization struct with only the fields that are explicitly configured. -// For updates, it only includes fields that have actually changed to avoid API validation errors. +// buildOrganizationSettings creates a github.Organization struct. +// On create, boolean fields are always included (their schema default is a +// definite value and d.GetOk cannot distinguish an explicit false from unset), +// while non-boolean fields are included only when explicitly configured. +// On update, it only includes fields that have actually changed to avoid API +// validation errors. func buildOrganizationSettings(d *schema.ResourceData, isEnterprise bool) *github.Organization { settings := &github.Organization{} @@ -183,13 +187,19 @@ func buildOrganizationSettings(d *schema.ResourceData, isEnterprise bool) *githu // Helper function to check if field should be included shouldInclude := func(fieldName string) bool { - if !isUpdate { - // For creates, include if explicitly configured - _, ok := d.GetOk(fieldName) - return ok - } // For updates, only include if the field has changed - return d.HasChange(fieldName) + if isUpdate { + return d.HasChange(fieldName) + } + // For creates, boolean attributes always carry a definite value from + // their schema default, and d.GetOk cannot tell an explicit false from + // an unset field, so include them unconditionally. Other types are + // included only when explicitly configured. + if s, ok := resourceGithubOrganizationSettings().Schema[fieldName]; ok && s.Type == schema.TypeBool { + return true + } + _, ok := d.GetOk(fieldName) + return ok } // Required field - always include if configured (API requires it even if unchanged) diff --git a/github/resource_github_organization_settings_unit_test.go b/github/resource_github_organization_settings_unit_test.go new file mode 100644 index 0000000000..d6b3dacf5c --- /dev/null +++ b/github/resource_github_organization_settings_unit_test.go @@ -0,0 +1,125 @@ +package github + +import ( + "testing" + + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" +) + +// TestBuildOrganizationSettingsCreateIncludesFalseBool verifies that a boolean +// 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) { + t.Parallel() + + r := resourceGithubOrganizationSettings() + d := schema.TestResourceDataRaw(t, r.Schema, map[string]any{ + "billing_email": "org@example.com", + "has_organization_projects": false, + "web_commit_signoff_required": false, + }) + // TestResourceDataRaw leaves the ID empty, so this exercises the create path. + + settings := buildOrganizationSettings(d, false) + + if settings.HasOrganizationProjects == nil { + t.Fatal("has_organization_projects set to false was dropped on create; expected it to be included") + } + if got := settings.GetHasOrganizationProjects(); got != false { + t.Errorf("has_organization_projects = %v, want false", got) + } + + if settings.WebCommitSignoffRequired == nil { + t.Fatal("web_commit_signoff_required set to false was dropped on create; expected it to be included") + } + if got := settings.GetWebCommitSignoffRequired(); got != false { + t.Errorf("web_commit_signoff_required = %v, want false", got) + } +} + +// TestBuildOrganizationSettingsCreateIncludesTrueBool verifies a bool set to +// true is still included on create. +func TestBuildOrganizationSettingsCreateIncludesTrueBool(t *testing.T) { + t.Parallel() + + r := resourceGithubOrganizationSettings() + d := schema.TestResourceDataRaw(t, r.Schema, map[string]any{ + "billing_email": "org@example.com", + "web_commit_signoff_required": true, + }) + + settings := buildOrganizationSettings(d, false) + + if settings.WebCommitSignoffRequired == nil { + t.Fatal("web_commit_signoff_required set to true was dropped on create") + } + if got := settings.GetWebCommitSignoffRequired(); got != true { + t.Errorf("web_commit_signoff_required = %v, want true", got) + } +} + +// TestBuildOrganizationSettingsCreateStringUnsetOmitted verifies the fix does +// not change string handling: an unconfigured optional string stays omitted on +// create (only booleans are always included). +func TestBuildOrganizationSettingsCreateStringUnsetOmitted(t *testing.T) { + t.Parallel() + + r := resourceGithubOrganizationSettings() + d := schema.TestResourceDataRaw(t, r.Schema, map[string]any{ + "billing_email": "org@example.com", + }) + + settings := buildOrganizationSettings(d, false) + + if settings.Company != nil { + t.Errorf("company was not configured but got included as %q", settings.GetCompany()) + } + if settings.BillingEmail == nil || settings.GetBillingEmail() != "org@example.com" { + t.Errorf("billing_email = %v, want org@example.com", settings.BillingEmail) + } +} + +// TestBuildOrganizationSettingsUpdateGatesOnHasChange verifies the update path +// is unaffected by the create-path fix: with an ID set and no diff, unchanged +// boolean fields are still omitted (only changed fields are included on update). +func TestBuildOrganizationSettingsUpdateGatesOnHasChange(t *testing.T) { + t.Parallel() + + r := resourceGithubOrganizationSettings() + d := schema.TestResourceDataRaw(t, r.Schema, map[string]any{ + "billing_email": "org@example.com", + "has_organization_projects": false, + }) + d.SetId("example-org") // non-empty ID exercises the update path + + settings := buildOrganizationSettings(d, false) + + if settings.HasOrganizationProjects != nil { + t.Error("has_organization_projects with no change should be omitted on update") + } +} + +// TestBuildOrganizationSettingsCreateEnterpriseBool verifies the enterprise-only +// boolean is included on create only when isEnterprise is true. +func TestBuildOrganizationSettingsCreateEnterpriseBool(t *testing.T) { + t.Parallel() + + r := resourceGithubOrganizationSettings() + + dEnterprise := schema.TestResourceDataRaw(t, r.Schema, map[string]any{ + "billing_email": "org@example.com", + "members_can_create_internal_repositories": false, + }) + if got := buildOrganizationSettings(dEnterprise, true); got.MembersCanCreateInternalRepos == nil { + t.Fatal("members_can_create_internal_repositories set to false was dropped on enterprise create") + } + + dNonEnterprise := schema.TestResourceDataRaw(t, r.Schema, map[string]any{ + "billing_email": "org@example.com", + "members_can_create_internal_repositories": false, + }) + if got := buildOrganizationSettings(dNonEnterprise, false); got.MembersCanCreateInternalRepos != nil { + t.Error("members_can_create_internal_repositories should not be set when isEnterprise is false") + } +} From b315f067f68ee49a44ccd3a398f9a89f00a710d4 Mon Sep 17 00:00:00 2001 From: somaz Date: Fri, 24 Jul 2026 11:47:51 +0900 Subject: [PATCH 02/11] test: consolidate organization_settings unit tests into a table-driven test --- ...ource_github_organization_settings_test.go | 147 ++++++++++++++++++ ..._github_organization_settings_unit_test.go | 125 --------------- 2 files changed, 147 insertions(+), 125 deletions(-) delete mode 100644 github/resource_github_organization_settings_unit_test.go diff --git a/github/resource_github_organization_settings_test.go b/github/resource_github_organization_settings_test.go index f85b80bd6a..f3df69c252 100644 --- a/github/resource_github_organization_settings_test.go +++ b/github/resource_github_organization_settings_test.go @@ -4,6 +4,7 @@ import ( "fmt" "testing" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-testing/helper/resource" ) @@ -613,3 +614,149 @@ func TestAccGithubOrganizationSettings(t *testing.T) { }) }) } + +func Test_buildOrganizationSettings(t *testing.T) { + t.Parallel() + + for _, tt := range []struct { + name string + raw map[string]any + id string // a non-empty ID exercises the update path + isEnterprise bool + + wantBillingEmail *string + wantCompany *string + wantHasOrganizationProjects *bool + wantWebCommitSignoffRequired *bool + wantMembersCanCreateInternalRepos *bool + wantOmitted []string + }{ + { + // Regression test for the create path dropping booleans configured as + // false, which only corrected itself on a second apply through the + // update/HasChange path. + name: "create_includes_explicitly_false_booleans", + raw: map[string]any{ + "billing_email": "org@example.com", + "has_organization_projects": false, + "web_commit_signoff_required": false, + }, + wantBillingEmail: new("org@example.com"), + wantHasOrganizationProjects: new(false), + wantWebCommitSignoffRequired: new(false), + }, + { + name: "create_includes_true_booleans", + raw: map[string]any{ + "billing_email": "org@example.com", + "web_commit_signoff_required": true, + }, + wantBillingEmail: new("org@example.com"), + wantWebCommitSignoffRequired: new(true), + }, + { + // Only booleans are included unconditionally on create; an + // unconfigured optional string stays omitted. + name: "create_omits_unconfigured_optional_string", + raw: map[string]any{ + "billing_email": "org@example.com", + }, + wantBillingEmail: new("org@example.com"), + wantOmitted: []string{"company"}, + }, + { + name: "update_omits_unchanged_booleans", + raw: map[string]any{ + "billing_email": "org@example.com", + "has_organization_projects": false, + }, + id: "example-org", + wantOmitted: []string{"has_organization_projects"}, + }, + { + name: "enterprise_create_includes_internal_repositories_boolean", + raw: map[string]any{ + "billing_email": "org@example.com", + "members_can_create_internal_repositories": false, + }, + isEnterprise: true, + wantMembersCanCreateInternalRepos: new(false), + }, + { + name: "non_enterprise_create_omits_internal_repositories_boolean", + raw: map[string]any{ + "billing_email": "org@example.com", + "members_can_create_internal_repositories": false, + }, + wantOmitted: []string{"members_can_create_internal_repositories"}, + }, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + d := schema.TestResourceDataRaw(t, resourceGithubOrganizationSettings().Schema, tt.raw) + if tt.id != "" { + d.SetId(tt.id) + } + + got := buildOrganizationSettings(d, tt.isEnterprise) + + assertStringPtr(t, "billing_email", got.BillingEmail, tt.wantBillingEmail) + assertStringPtr(t, "company", got.Company, tt.wantCompany) + assertBoolPtr(t, "has_organization_projects", got.HasOrganizationProjects, tt.wantHasOrganizationProjects) + assertBoolPtr(t, "web_commit_signoff_required", got.WebCommitSignoffRequired, tt.wantWebCommitSignoffRequired) + assertBoolPtr(t, "members_can_create_internal_repositories", got.MembersCanCreateInternalRepos, tt.wantMembersCanCreateInternalRepos) + + omitted := map[string]bool{ + "billing_email": got.BillingEmail == nil, + "company": got.Company == nil, + "has_organization_projects": got.HasOrganizationProjects == nil, + "web_commit_signoff_required": got.WebCommitSignoffRequired == nil, + "members_can_create_internal_repositories": got.MembersCanCreateInternalRepos == nil, + } + for _, field := range tt.wantOmitted { + isOmitted, ok := omitted[field] + if !ok { + t.Fatalf("wantOmitted references unknown field %q", field) + } + if !isOmitted { + t.Errorf("%s should have been omitted from the payload", field) + } + } + }) + } +} + +// assertBoolPtr checks a boolean field of the built payload. A nil want means +// the field is not asserted by this test case; use wantOmitted to require that +// a field is absent. +func assertBoolPtr(t *testing.T, field string, got, want *bool) { + t.Helper() + + if want == nil { + return + } + if got == nil { + t.Errorf("%s was omitted from the payload, want %v", field, *want) + return + } + if *got != *want { + t.Errorf("%s = %v, want %v", field, *got, *want) + } +} + +// assertStringPtr is the string counterpart of assertBoolPtr. +func assertStringPtr(t *testing.T, field string, got, want *string) { + t.Helper() + + if want == nil { + return + } + if got == nil { + t.Errorf("%s was omitted from the payload, want %q", field, *want) + return + } + if *got != *want { + t.Errorf("%s = %q, want %q", field, *got, *want) + } +} diff --git a/github/resource_github_organization_settings_unit_test.go b/github/resource_github_organization_settings_unit_test.go deleted file mode 100644 index d6b3dacf5c..0000000000 --- a/github/resource_github_organization_settings_unit_test.go +++ /dev/null @@ -1,125 +0,0 @@ -package github - -import ( - "testing" - - "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" -) - -// TestBuildOrganizationSettingsCreateIncludesFalseBool verifies that a boolean -// 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) { - t.Parallel() - - r := resourceGithubOrganizationSettings() - d := schema.TestResourceDataRaw(t, r.Schema, map[string]any{ - "billing_email": "org@example.com", - "has_organization_projects": false, - "web_commit_signoff_required": false, - }) - // TestResourceDataRaw leaves the ID empty, so this exercises the create path. - - settings := buildOrganizationSettings(d, false) - - if settings.HasOrganizationProjects == nil { - t.Fatal("has_organization_projects set to false was dropped on create; expected it to be included") - } - if got := settings.GetHasOrganizationProjects(); got != false { - t.Errorf("has_organization_projects = %v, want false", got) - } - - if settings.WebCommitSignoffRequired == nil { - t.Fatal("web_commit_signoff_required set to false was dropped on create; expected it to be included") - } - if got := settings.GetWebCommitSignoffRequired(); got != false { - t.Errorf("web_commit_signoff_required = %v, want false", got) - } -} - -// TestBuildOrganizationSettingsCreateIncludesTrueBool verifies a bool set to -// true is still included on create. -func TestBuildOrganizationSettingsCreateIncludesTrueBool(t *testing.T) { - t.Parallel() - - r := resourceGithubOrganizationSettings() - d := schema.TestResourceDataRaw(t, r.Schema, map[string]any{ - "billing_email": "org@example.com", - "web_commit_signoff_required": true, - }) - - settings := buildOrganizationSettings(d, false) - - if settings.WebCommitSignoffRequired == nil { - t.Fatal("web_commit_signoff_required set to true was dropped on create") - } - if got := settings.GetWebCommitSignoffRequired(); got != true { - t.Errorf("web_commit_signoff_required = %v, want true", got) - } -} - -// TestBuildOrganizationSettingsCreateStringUnsetOmitted verifies the fix does -// not change string handling: an unconfigured optional string stays omitted on -// create (only booleans are always included). -func TestBuildOrganizationSettingsCreateStringUnsetOmitted(t *testing.T) { - t.Parallel() - - r := resourceGithubOrganizationSettings() - d := schema.TestResourceDataRaw(t, r.Schema, map[string]any{ - "billing_email": "org@example.com", - }) - - settings := buildOrganizationSettings(d, false) - - if settings.Company != nil { - t.Errorf("company was not configured but got included as %q", settings.GetCompany()) - } - if settings.BillingEmail == nil || settings.GetBillingEmail() != "org@example.com" { - t.Errorf("billing_email = %v, want org@example.com", settings.BillingEmail) - } -} - -// TestBuildOrganizationSettingsUpdateGatesOnHasChange verifies the update path -// is unaffected by the create-path fix: with an ID set and no diff, unchanged -// boolean fields are still omitted (only changed fields are included on update). -func TestBuildOrganizationSettingsUpdateGatesOnHasChange(t *testing.T) { - t.Parallel() - - r := resourceGithubOrganizationSettings() - d := schema.TestResourceDataRaw(t, r.Schema, map[string]any{ - "billing_email": "org@example.com", - "has_organization_projects": false, - }) - d.SetId("example-org") // non-empty ID exercises the update path - - settings := buildOrganizationSettings(d, false) - - if settings.HasOrganizationProjects != nil { - t.Error("has_organization_projects with no change should be omitted on update") - } -} - -// TestBuildOrganizationSettingsCreateEnterpriseBool verifies the enterprise-only -// boolean is included on create only when isEnterprise is true. -func TestBuildOrganizationSettingsCreateEnterpriseBool(t *testing.T) { - t.Parallel() - - r := resourceGithubOrganizationSettings() - - dEnterprise := schema.TestResourceDataRaw(t, r.Schema, map[string]any{ - "billing_email": "org@example.com", - "members_can_create_internal_repositories": false, - }) - if got := buildOrganizationSettings(dEnterprise, true); got.MembersCanCreateInternalRepos == nil { - t.Fatal("members_can_create_internal_repositories set to false was dropped on enterprise create") - } - - dNonEnterprise := schema.TestResourceDataRaw(t, r.Schema, map[string]any{ - "billing_email": "org@example.com", - "members_can_create_internal_repositories": false, - }) - if got := buildOrganizationSettings(dNonEnterprise, false); got.MembersCanCreateInternalRepos != nil { - t.Error("members_can_create_internal_repositories should not be set when isEnterprise is false") - } -} From 98c3c8dd5687cbd64e2bf53376bd6104d637b0c7 Mon Sep 17 00:00:00 2001 From: somaz Date: Wed, 29 Jul 2026 10:57:20 +0900 Subject: [PATCH 03/11] refactor: drop shouldInclude call from boolean fields in organization settings --- .../resource_github_organization_settings.go | 54 +++++++++---------- 1 file changed, 27 insertions(+), 27 deletions(-) diff --git a/github/resource_github_organization_settings.go b/github/resource_github_organization_settings.go index 6bbe091ad8..e5f1aa1841 100644 --- a/github/resource_github_organization_settings.go +++ b/github/resource_github_organization_settings.go @@ -185,23 +185,23 @@ func buildOrganizationSettings(d *schema.ResourceData, isEnterprise bool) *githu // Check if this is an update (has ID) or create (no ID) isUpdate := d.Id() != "" - // Helper function to check if field should be included + // Non-boolean fields are included only when explicitly configured on create, + // and only when changed on update. shouldInclude := func(fieldName string) bool { - // For updates, only include if the field has changed if isUpdate { return d.HasChange(fieldName) } - // For creates, boolean attributes always carry a definite value from - // their schema default, and d.GetOk cannot tell an explicit false from - // an unset field, so include them unconditionally. Other types are - // included only when explicitly configured. - if s, ok := resourceGithubOrganizationSettings().Schema[fieldName]; ok && s.Type == schema.TypeBool { - return true - } _, 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) + } + // Required field - always include if configured (API requires it even if unchanged) if billingEmail, ok := d.GetOk("billing_email"); ok { settings.BillingEmail = new(billingEmail.(string)) @@ -245,12 +245,12 @@ func buildOrganizationSettings(d *schema.ResourceData, isEnterprise bool) *githu } // Boolean fields - only set if should be included - // Use d.Get() instead of d.GetOk() when shouldInclude() returns true, + // Use d.Get() instead of d.GetOk() when includeBool() returns true, // because we already know the field should be included, and d.Get() correctly handles false values - if shouldInclude("has_organization_projects") { + if includeBool("has_organization_projects") { settings.HasOrganizationProjects = new(d.Get("has_organization_projects").(bool)) } - if shouldInclude("has_repository_projects") { + if includeBool("has_repository_projects") { settings.HasRepositoryProjects = new(d.Get("has_repository_projects").(bool)) } if shouldInclude("default_repository_permission") { @@ -258,52 +258,52 @@ func buildOrganizationSettings(d *schema.ResourceData, isEnterprise bool) *githu settings.DefaultRepoPermission = new(defaultRepoPermission.(string)) } } - if shouldInclude("members_can_create_repositories") { + if includeBool("members_can_create_repositories") { settings.MembersCanCreateRepos = new(d.Get("members_can_create_repositories").(bool)) } - if shouldInclude("members_can_create_private_repositories") { + if includeBool("members_can_create_private_repositories") { settings.MembersCanCreatePrivateRepos = new(d.Get("members_can_create_private_repositories").(bool)) } - if shouldInclude("members_can_create_public_repositories") { + if includeBool("members_can_create_public_repositories") { settings.MembersCanCreatePublicRepos = new(d.Get("members_can_create_public_repositories").(bool)) } - if shouldInclude("members_can_create_pages") { + if includeBool("members_can_create_pages") { settings.MembersCanCreatePages = new(d.Get("members_can_create_pages").(bool)) } - if shouldInclude("members_can_create_public_pages") { + if includeBool("members_can_create_public_pages") { settings.MembersCanCreatePublicPages = new(d.Get("members_can_create_public_pages").(bool)) } - if shouldInclude("members_can_create_private_pages") { + if includeBool("members_can_create_private_pages") { settings.MembersCanCreatePrivatePages = new(d.Get("members_can_create_private_pages").(bool)) } - if shouldInclude("members_can_fork_private_repositories") { + if includeBool("members_can_fork_private_repositories") { settings.MembersCanForkPrivateRepos = new(d.Get("members_can_fork_private_repositories").(bool)) } - if shouldInclude("web_commit_signoff_required") { + if includeBool("web_commit_signoff_required") { settings.WebCommitSignoffRequired = new(d.Get("web_commit_signoff_required").(bool)) } - if shouldInclude("advanced_security_enabled_for_new_repositories") { + if includeBool("advanced_security_enabled_for_new_repositories") { settings.AdvancedSecurityEnabledForNewRepos = new(d.Get("advanced_security_enabled_for_new_repositories").(bool)) } - if shouldInclude("dependabot_alerts_enabled_for_new_repositories") { + if includeBool("dependabot_alerts_enabled_for_new_repositories") { settings.DependabotAlertsEnabledForNewRepos = new(d.Get("dependabot_alerts_enabled_for_new_repositories").(bool)) } - if shouldInclude("dependabot_security_updates_enabled_for_new_repositories") { + if includeBool("dependabot_security_updates_enabled_for_new_repositories") { settings.DependabotSecurityUpdatesEnabledForNewRepos = new(d.Get("dependabot_security_updates_enabled_for_new_repositories").(bool)) } - if shouldInclude("dependency_graph_enabled_for_new_repositories") { + if includeBool("dependency_graph_enabled_for_new_repositories") { settings.DependencyGraphEnabledForNewRepos = new(d.Get("dependency_graph_enabled_for_new_repositories").(bool)) } - if shouldInclude("secret_scanning_enabled_for_new_repositories") { + if includeBool("secret_scanning_enabled_for_new_repositories") { settings.SecretScanningEnabledForNewRepos = new(d.Get("secret_scanning_enabled_for_new_repositories").(bool)) } - if shouldInclude("secret_scanning_push_protection_enabled_for_new_repositories") { + if includeBool("secret_scanning_push_protection_enabled_for_new_repositories") { settings.SecretScanningPushProtectionEnabledForNewRepos = new(d.Get("secret_scanning_push_protection_enabled_for_new_repositories").(bool)) } // Enterprise-specific field if isEnterprise { - if shouldInclude("members_can_create_internal_repositories") { + if includeBool("members_can_create_internal_repositories") { settings.MembersCanCreateInternalRepos = new(d.Get("members_can_create_internal_repositories").(bool)) } } From 3c70a23ea59f5f91232745bcf70281b42b3edf6b Mon Sep 17 00:00:00 2001 From: somaz Date: Wed, 29 Jul 2026 10:57:20 +0900 Subject: [PATCH 04/11] test: compare built organization settings payload with cmp.Diff --- ...ource_github_organization_settings_test.go | 206 +++++++++++------- 1 file changed, 130 insertions(+), 76 deletions(-) diff --git a/github/resource_github_organization_settings_test.go b/github/resource_github_organization_settings_test.go index f3df69c252..201d0f9a25 100644 --- a/github/resource_github_organization_settings_test.go +++ b/github/resource_github_organization_settings_test.go @@ -4,6 +4,8 @@ import ( "fmt" "testing" + "github.com/google/go-cmp/cmp" + "github.com/google/go-github/v89/github" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-testing/helper/resource" ) @@ -623,27 +625,39 @@ func Test_buildOrganizationSettings(t *testing.T) { raw map[string]any id string // a non-empty ID exercises the update path isEnterprise bool - - wantBillingEmail *string - wantCompany *string - wantHasOrganizationProjects *bool - wantWebCommitSignoffRequired *bool - wantMembersCanCreateInternalRepos *bool - wantOmitted []string + want *github.Organization }{ { // Regression test for the create path dropping booleans configured as // false, which only corrected itself on a second apply through the - // update/HasChange path. + // update/HasChange path. Every boolean carries a definite value on + // create, so the full payload is asserted here. name: "create_includes_explicitly_false_booleans", raw: map[string]any{ "billing_email": "org@example.com", "has_organization_projects": false, "web_commit_signoff_required": false, }, - wantBillingEmail: new("org@example.com"), - wantHasOrganizationProjects: new(false), - wantWebCommitSignoffRequired: new(false), + want: &github.Organization{ + BillingEmail: new("org@example.com"), + HasOrganizationProjects: new(false), + HasRepositoryProjects: new(true), + DefaultRepoPermission: new("read"), + MembersCanCreateRepos: new(true), + MembersCanCreatePrivateRepos: new(true), + MembersCanCreatePublicRepos: new(true), + MembersCanCreatePages: new(true), + MembersCanCreatePublicPages: new(true), + MembersCanCreatePrivatePages: new(true), + MembersCanForkPrivateRepos: new(false), + WebCommitSignoffRequired: new(false), + AdvancedSecurityEnabledForNewRepos: new(false), + DependabotAlertsEnabledForNewRepos: new(false), + DependabotSecurityUpdatesEnabledForNewRepos: new(false), + DependencyGraphEnabledForNewRepos: new(false), + SecretScanningEnabledForNewRepos: new(false), + SecretScanningPushProtectionEnabledForNewRepos: new(false), + }, }, { name: "create_includes_true_booleans", @@ -651,27 +665,79 @@ func Test_buildOrganizationSettings(t *testing.T) { "billing_email": "org@example.com", "web_commit_signoff_required": true, }, - wantBillingEmail: new("org@example.com"), - wantWebCommitSignoffRequired: new(true), + want: &github.Organization{ + BillingEmail: new("org@example.com"), + HasOrganizationProjects: new(true), + HasRepositoryProjects: new(true), + DefaultRepoPermission: new("read"), + MembersCanCreateRepos: new(true), + MembersCanCreatePrivateRepos: new(true), + MembersCanCreatePublicRepos: new(true), + MembersCanCreatePages: new(true), + MembersCanCreatePublicPages: new(true), + MembersCanCreatePrivatePages: new(true), + MembersCanForkPrivateRepos: new(false), + WebCommitSignoffRequired: new(true), + AdvancedSecurityEnabledForNewRepos: new(false), + DependabotAlertsEnabledForNewRepos: new(false), + DependabotSecurityUpdatesEnabledForNewRepos: new(false), + DependencyGraphEnabledForNewRepos: new(false), + SecretScanningEnabledForNewRepos: new(false), + SecretScanningPushProtectionEnabledForNewRepos: new(false), + }, }, { // Only booleans are included unconditionally on create; an - // unconfigured optional string stays omitted. + // unconfigured optional string stays absent from the payload. name: "create_omits_unconfigured_optional_string", raw: map[string]any{ "billing_email": "org@example.com", }, - wantBillingEmail: new("org@example.com"), - wantOmitted: []string{"company"}, + want: &github.Organization{ + BillingEmail: new("org@example.com"), + HasOrganizationProjects: new(true), + HasRepositoryProjects: new(true), + DefaultRepoPermission: new("read"), + MembersCanCreateRepos: new(true), + MembersCanCreatePrivateRepos: new(true), + MembersCanCreatePublicRepos: new(true), + MembersCanCreatePages: new(true), + MembersCanCreatePublicPages: new(true), + MembersCanCreatePrivatePages: new(true), + MembersCanForkPrivateRepos: new(false), + WebCommitSignoffRequired: new(false), + AdvancedSecurityEnabledForNewRepos: new(false), + DependabotAlertsEnabledForNewRepos: new(false), + DependabotSecurityUpdatesEnabledForNewRepos: new(false), + DependencyGraphEnabledForNewRepos: new(false), + SecretScanningEnabledForNewRepos: new(false), + SecretScanningPushProtectionEnabledForNewRepos: new(false), + }, }, { + // On update a field is only sent when it has changed, so the + // explicitly configured has_organization_projects is dropped. The + // attributes that do survive are an artifact of TestResourceDataRaw: + // one left out of raw reads as the zero value from state but as its + // schema default from d.Get, so every attribute defaulting to a + // non-zero value (true, "read") looks changed here. name: "update_omits_unchanged_booleans", raw: map[string]any{ "billing_email": "org@example.com", "has_organization_projects": false, }, - id: "example-org", - wantOmitted: []string{"has_organization_projects"}, + id: "example-org", + want: &github.Organization{ + BillingEmail: new("org@example.com"), + HasRepositoryProjects: new(true), + DefaultRepoPermission: new("read"), + MembersCanCreateRepos: new(true), + MembersCanCreatePrivateRepos: new(true), + MembersCanCreatePublicRepos: new(true), + MembersCanCreatePages: new(true), + MembersCanCreatePublicPages: new(true), + MembersCanCreatePrivatePages: new(true), + }, }, { name: "enterprise_create_includes_internal_repositories_boolean", @@ -679,16 +745,57 @@ func Test_buildOrganizationSettings(t *testing.T) { "billing_email": "org@example.com", "members_can_create_internal_repositories": false, }, - isEnterprise: true, - wantMembersCanCreateInternalRepos: new(false), + isEnterprise: true, + want: &github.Organization{ + BillingEmail: new("org@example.com"), + HasOrganizationProjects: new(true), + HasRepositoryProjects: new(true), + DefaultRepoPermission: new("read"), + MembersCanCreateRepos: new(true), + MembersCanCreateInternalRepos: new(false), + MembersCanCreatePrivateRepos: new(true), + MembersCanCreatePublicRepos: new(true), + MembersCanCreatePages: new(true), + MembersCanCreatePublicPages: new(true), + MembersCanCreatePrivatePages: new(true), + MembersCanForkPrivateRepos: new(false), + WebCommitSignoffRequired: new(false), + AdvancedSecurityEnabledForNewRepos: new(false), + DependabotAlertsEnabledForNewRepos: new(false), + DependabotSecurityUpdatesEnabledForNewRepos: new(false), + DependencyGraphEnabledForNewRepos: new(false), + SecretScanningEnabledForNewRepos: new(false), + SecretScanningPushProtectionEnabledForNewRepos: new(false), + }, }, { + // The enterprise-only attribute must never reach the payload for a + // non-enterprise organization, even when it is configured. name: "non_enterprise_create_omits_internal_repositories_boolean", raw: map[string]any{ "billing_email": "org@example.com", "members_can_create_internal_repositories": false, }, - wantOmitted: []string{"members_can_create_internal_repositories"}, + want: &github.Organization{ + BillingEmail: new("org@example.com"), + HasOrganizationProjects: new(true), + HasRepositoryProjects: new(true), + DefaultRepoPermission: new("read"), + MembersCanCreateRepos: new(true), + MembersCanCreatePrivateRepos: new(true), + MembersCanCreatePublicRepos: new(true), + MembersCanCreatePages: new(true), + MembersCanCreatePublicPages: new(true), + MembersCanCreatePrivatePages: new(true), + MembersCanForkPrivateRepos: new(false), + WebCommitSignoffRequired: new(false), + AdvancedSecurityEnabledForNewRepos: new(false), + DependabotAlertsEnabledForNewRepos: new(false), + DependabotSecurityUpdatesEnabledForNewRepos: new(false), + DependencyGraphEnabledForNewRepos: new(false), + SecretScanningEnabledForNewRepos: new(false), + SecretScanningPushProtectionEnabledForNewRepos: new(false), + }, }, } { t.Run(tt.name, func(t *testing.T) { @@ -701,62 +808,9 @@ func Test_buildOrganizationSettings(t *testing.T) { got := buildOrganizationSettings(d, tt.isEnterprise) - assertStringPtr(t, "billing_email", got.BillingEmail, tt.wantBillingEmail) - assertStringPtr(t, "company", got.Company, tt.wantCompany) - assertBoolPtr(t, "has_organization_projects", got.HasOrganizationProjects, tt.wantHasOrganizationProjects) - assertBoolPtr(t, "web_commit_signoff_required", got.WebCommitSignoffRequired, tt.wantWebCommitSignoffRequired) - assertBoolPtr(t, "members_can_create_internal_repositories", got.MembersCanCreateInternalRepos, tt.wantMembersCanCreateInternalRepos) - - omitted := map[string]bool{ - "billing_email": got.BillingEmail == nil, - "company": got.Company == nil, - "has_organization_projects": got.HasOrganizationProjects == nil, - "web_commit_signoff_required": got.WebCommitSignoffRequired == nil, - "members_can_create_internal_repositories": got.MembersCanCreateInternalRepos == nil, - } - for _, field := range tt.wantOmitted { - isOmitted, ok := omitted[field] - if !ok { - t.Fatalf("wantOmitted references unknown field %q", field) - } - if !isOmitted { - t.Errorf("%s should have been omitted from the payload", field) - } + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Errorf("buildOrganizationSettings() mismatch (-want +got):\n%s", diff) } }) } } - -// assertBoolPtr checks a boolean field of the built payload. A nil want means -// the field is not asserted by this test case; use wantOmitted to require that -// a field is absent. -func assertBoolPtr(t *testing.T, field string, got, want *bool) { - t.Helper() - - if want == nil { - return - } - if got == nil { - t.Errorf("%s was omitted from the payload, want %v", field, *want) - return - } - if *got != *want { - t.Errorf("%s = %v, want %v", field, *got, *want) - } -} - -// assertStringPtr is the string counterpart of assertBoolPtr. -func assertStringPtr(t *testing.T, field string, got, want *string) { - t.Helper() - - if want == nil { - return - } - if got == nil { - t.Errorf("%s was omitted from the payload, want %q", field, *want) - return - } - if *got != *want { - t.Errorf("%s = %q, want %q", field, *got, *want) - } -} From da376ee8739eee7fe209ed939e3d8dd96b5e7eac Mon Sep 17 00:00:00 2001 From: somaz Date: Thu, 10 Sep 2026 11:24:32 +0900 Subject: [PATCH 05/11] refactor: split organization settings create and update handlers --- .../resource_github_organization_settings.go | 464 +++++++++++------- ...ource_github_organization_settings_test.go | 245 ++++----- 2 files changed, 402 insertions(+), 307 deletions(-) diff --git a/github/resource_github_organization_settings.go b/github/resource_github_organization_settings.go index e5f1aa1841..9593d63cb1 100644 --- a/github/resource_github_organization_settings.go +++ b/github/resource_github_organization_settings.go @@ -7,16 +7,17 @@ import ( "strconv" "github.com/google/go-github/v92/github" + "github.com/hashicorp/terraform-plugin-sdk/v2/diag" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" ) func resourceGithubOrganizationSettings() *schema.Resource { return &schema.Resource{ - Create: resourceGithubOrganizationSettingsCreateOrUpdate, - Read: resourceGithubOrganizationSettingsRead, - Update: resourceGithubOrganizationSettingsCreateOrUpdate, - Delete: resourceGithubOrganizationSettingsDelete, + CreateContext: resourceGithubOrganizationSettingsCreate, + ReadContext: resourceGithubOrganizationSettingsRead, + UpdateContext: resourceGithubOrganizationSettingsUpdate, + DeleteContext: resourceGithubOrganizationSettingsDelete, Importer: &schema.ResourceImporter{ StateContext: schema.ImportStatePassthroughContext, }, @@ -173,163 +174,235 @@ func resourceGithubOrganizationSettings() *schema.Resource { } } -// buildOrganizationSettings creates a github.Organization struct. -// On create, boolean fields are always included (their schema default is a -// definite value and d.GetOk cannot distinguish an explicit false from unset), -// while non-boolean fields are included only when explicitly configured. -// On update, it only includes fields that have actually changed to avoid API -// validation errors. -func buildOrganizationSettings(d *schema.ResourceData, isEnterprise bool) *github.Organization { +// 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 +} + +// organizationSettingsForCreate builds the payload sent when the resource is +// created. An attribute is included only when d.GetOk reports it as configured. +// +// Note that d.GetOk cannot distinguish an explicitly configured false from an +// unset attribute, so a boolean configured as false is currently dropped from +// the create payload. That behaviour is unchanged here and is tracked +// separately in #3493. +func organizationSettingsForCreate(d *schema.ResourceData, isEnterprise bool) *github.Organization { settings := &github.Organization{} - // Check if this is an update (has ID) or create (no ID) - isUpdate := d.Id() != "" + // The API rejects the request without billing_email, so it is always sent. + if _, ok := d.GetOk("billing_email"); ok { + settings.BillingEmail = new(stringAttr(d, "billing_email")) + } - // 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 + if _, ok := d.GetOk("company"); ok { + settings.Company = new(stringAttr(d, "company")) + } + if _, ok := d.GetOk("email"); ok { + settings.Email = new(stringAttr(d, "email")) + } + if _, ok := d.GetOk("twitter_username"); ok { + settings.TwitterUsername = new(stringAttr(d, "twitter_username")) + } + if _, ok := d.GetOk("location"); ok { + settings.Location = new(stringAttr(d, "location")) + } + if _, ok := d.GetOk("name"); ok { + settings.Name = new(stringAttr(d, "name")) + } + if _, ok := d.GetOk("description"); ok { + settings.Description = new(stringAttr(d, "description")) + } + if _, ok := d.GetOk("blog"); ok { + settings.Blog = new(stringAttr(d, "blog")) } - // 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) + if _, ok := d.GetOk("has_organization_projects"); ok { + settings.HasOrganizationProjects = new(boolAttr(d, "has_organization_projects")) + } + if _, ok := d.GetOk("has_repository_projects"); ok { + settings.HasRepositoryProjects = new(boolAttr(d, "has_repository_projects")) + } + if _, ok := d.GetOk("default_repository_permission"); ok { + settings.DefaultRepoPermission = new(stringAttr(d, "default_repository_permission")) + } + if _, ok := d.GetOk("members_can_create_repositories"); ok { + settings.MembersCanCreateRepos = new(boolAttr(d, "members_can_create_repositories")) + } + if _, ok := d.GetOk("members_can_create_private_repositories"); ok { + settings.MembersCanCreatePrivateRepos = new(boolAttr(d, "members_can_create_private_repositories")) + } + if _, ok := d.GetOk("members_can_create_public_repositories"); ok { + settings.MembersCanCreatePublicRepos = new(boolAttr(d, "members_can_create_public_repositories")) + } + if _, ok := d.GetOk("members_can_create_pages"); ok { + settings.MembersCanCreatePages = new(boolAttr(d, "members_can_create_pages")) + } + if _, ok := d.GetOk("members_can_create_public_pages"); ok { + settings.MembersCanCreatePublicPages = new(boolAttr(d, "members_can_create_public_pages")) } + if _, ok := d.GetOk("members_can_create_private_pages"); ok { + settings.MembersCanCreatePrivatePages = new(boolAttr(d, "members_can_create_private_pages")) + } + if _, ok := d.GetOk("members_can_fork_private_repositories"); ok { + settings.MembersCanForkPrivateRepos = new(boolAttr(d, "members_can_fork_private_repositories")) + } + if _, ok := d.GetOk("web_commit_signoff_required"); ok { + settings.WebCommitSignoffRequired = new(boolAttr(d, "web_commit_signoff_required")) + } + if _, ok := d.GetOk("advanced_security_enabled_for_new_repositories"); ok { + settings.AdvancedSecurityEnabledForNewRepos = new(boolAttr(d, "advanced_security_enabled_for_new_repositories")) + } + if _, ok := d.GetOk("dependabot_alerts_enabled_for_new_repositories"); ok { + settings.DependabotAlertsEnabledForNewRepos = new(boolAttr(d, "dependabot_alerts_enabled_for_new_repositories")) + } + if _, ok := d.GetOk("dependabot_security_updates_enabled_for_new_repositories"); ok { + settings.DependabotSecurityUpdatesEnabledForNewRepos = new(boolAttr(d, "dependabot_security_updates_enabled_for_new_repositories")) + } + if _, ok := d.GetOk("dependency_graph_enabled_for_new_repositories"); ok { + settings.DependencyGraphEnabledForNewRepos = new(boolAttr(d, "dependency_graph_enabled_for_new_repositories")) + } + if _, ok := d.GetOk("secret_scanning_enabled_for_new_repositories"); ok { + settings.SecretScanningEnabledForNewRepos = new(boolAttr(d, "secret_scanning_enabled_for_new_repositories")) + } + if _, ok := d.GetOk("secret_scanning_push_protection_enabled_for_new_repositories"); ok { + settings.SecretScanningPushProtectionEnabledForNewRepos = new(boolAttr(d, "secret_scanning_push_protection_enabled_for_new_repositories")) + } + + if isEnterprise { + if _, ok := d.GetOk("members_can_create_internal_repositories"); ok { + settings.MembersCanCreateInternalRepos = new(boolAttr(d, "members_can_create_internal_repositories")) + } + } + + return settings +} + +// organizationSettingsForUpdate builds the payload sent when the resource is +// updated. Only attributes that actually changed are included, which keeps +// unconfigured attributes out of the request and avoids the API validation +// errors reported in #2305. +func organizationSettingsForUpdate(d *schema.ResourceData, isEnterprise bool) *github.Organization { + settings := &github.Organization{} - // Required field - always include if configured (API requires it even if unchanged) - if billingEmail, ok := d.GetOk("billing_email"); ok { - settings.BillingEmail = new(billingEmail.(string)) + // The API rejects the request without billing_email, so it is always sent. + if _, ok := d.GetOk("billing_email"); ok { + settings.BillingEmail = new(stringAttr(d, "billing_email")) } - // Optional string fields - only set if should be included - if shouldInclude("company") { - if company, ok := d.GetOk("company"); ok { - settings.Company = new(company.(string)) + if d.HasChange("company") { + if _, ok := d.GetOk("company"); ok { + settings.Company = new(stringAttr(d, "company")) } } - if shouldInclude("email") { - if email, ok := d.GetOk("email"); ok { - settings.Email = new(email.(string)) + if d.HasChange("email") { + if _, ok := d.GetOk("email"); ok { + settings.Email = new(stringAttr(d, "email")) } } - if shouldInclude("twitter_username") { - if twitterUsername, ok := d.GetOk("twitter_username"); ok { - settings.TwitterUsername = new(twitterUsername.(string)) + if d.HasChange("twitter_username") { + if _, ok := d.GetOk("twitter_username"); ok { + settings.TwitterUsername = new(stringAttr(d, "twitter_username")) } } - if shouldInclude("location") { - if location, ok := d.GetOk("location"); ok { - settings.Location = new(location.(string)) + if d.HasChange("location") { + if _, ok := d.GetOk("location"); ok { + settings.Location = new(stringAttr(d, "location")) } } - if shouldInclude("name") { - if name, ok := d.GetOk("name"); ok { - settings.Name = new(name.(string)) + if d.HasChange("name") { + if _, ok := d.GetOk("name"); ok { + settings.Name = new(stringAttr(d, "name")) } } - if shouldInclude("description") { - if description, ok := d.GetOk("description"); ok { - settings.Description = new(description.(string)) + if d.HasChange("description") { + if _, ok := d.GetOk("description"); ok { + settings.Description = new(stringAttr(d, "description")) } } - if shouldInclude("blog") { - if blog, ok := d.GetOk("blog"); ok { - settings.Blog = new(blog.(string)) + if d.HasChange("blog") { + if _, ok := d.GetOk("blog"); ok { + settings.Blog = new(stringAttr(d, "blog")) } } - // Boolean fields - only set if should be included - // Use d.Get() instead of d.GetOk() when includeBool() returns true, - // because we already know the field should be included, and d.Get() correctly handles false values - if includeBool("has_organization_projects") { - settings.HasOrganizationProjects = new(d.Get("has_organization_projects").(bool)) + if d.HasChange("has_organization_projects") { + settings.HasOrganizationProjects = new(boolAttr(d, "has_organization_projects")) } - if includeBool("has_repository_projects") { - settings.HasRepositoryProjects = new(d.Get("has_repository_projects").(bool)) + if d.HasChange("has_repository_projects") { + settings.HasRepositoryProjects = new(boolAttr(d, "has_repository_projects")) } - if shouldInclude("default_repository_permission") { - if defaultRepoPermission, ok := d.GetOk("default_repository_permission"); ok { - settings.DefaultRepoPermission = new(defaultRepoPermission.(string)) + if d.HasChange("default_repository_permission") { + if _, ok := d.GetOk("default_repository_permission"); ok { + settings.DefaultRepoPermission = new(stringAttr(d, "default_repository_permission")) } } - if includeBool("members_can_create_repositories") { - settings.MembersCanCreateRepos = new(d.Get("members_can_create_repositories").(bool)) + if d.HasChange("members_can_create_repositories") { + settings.MembersCanCreateRepos = new(boolAttr(d, "members_can_create_repositories")) } - if includeBool("members_can_create_private_repositories") { - settings.MembersCanCreatePrivateRepos = new(d.Get("members_can_create_private_repositories").(bool)) + if d.HasChange("members_can_create_private_repositories") { + settings.MembersCanCreatePrivateRepos = new(boolAttr(d, "members_can_create_private_repositories")) } - if includeBool("members_can_create_public_repositories") { - settings.MembersCanCreatePublicRepos = new(d.Get("members_can_create_public_repositories").(bool)) + if d.HasChange("members_can_create_public_repositories") { + settings.MembersCanCreatePublicRepos = new(boolAttr(d, "members_can_create_public_repositories")) } - if includeBool("members_can_create_pages") { - settings.MembersCanCreatePages = new(d.Get("members_can_create_pages").(bool)) + if d.HasChange("members_can_create_pages") { + settings.MembersCanCreatePages = new(boolAttr(d, "members_can_create_pages")) } - if includeBool("members_can_create_public_pages") { - settings.MembersCanCreatePublicPages = new(d.Get("members_can_create_public_pages").(bool)) + if d.HasChange("members_can_create_public_pages") { + settings.MembersCanCreatePublicPages = new(boolAttr(d, "members_can_create_public_pages")) } - if includeBool("members_can_create_private_pages") { - settings.MembersCanCreatePrivatePages = new(d.Get("members_can_create_private_pages").(bool)) + if d.HasChange("members_can_create_private_pages") { + settings.MembersCanCreatePrivatePages = new(boolAttr(d, "members_can_create_private_pages")) } - if includeBool("members_can_fork_private_repositories") { - settings.MembersCanForkPrivateRepos = new(d.Get("members_can_fork_private_repositories").(bool)) + if d.HasChange("members_can_fork_private_repositories") { + settings.MembersCanForkPrivateRepos = new(boolAttr(d, "members_can_fork_private_repositories")) } - if includeBool("web_commit_signoff_required") { - settings.WebCommitSignoffRequired = new(d.Get("web_commit_signoff_required").(bool)) + if d.HasChange("web_commit_signoff_required") { + settings.WebCommitSignoffRequired = new(boolAttr(d, "web_commit_signoff_required")) } - if includeBool("advanced_security_enabled_for_new_repositories") { - settings.AdvancedSecurityEnabledForNewRepos = new(d.Get("advanced_security_enabled_for_new_repositories").(bool)) + if d.HasChange("advanced_security_enabled_for_new_repositories") { + settings.AdvancedSecurityEnabledForNewRepos = new(boolAttr(d, "advanced_security_enabled_for_new_repositories")) } - if includeBool("dependabot_alerts_enabled_for_new_repositories") { - settings.DependabotAlertsEnabledForNewRepos = new(d.Get("dependabot_alerts_enabled_for_new_repositories").(bool)) + if d.HasChange("dependabot_alerts_enabled_for_new_repositories") { + settings.DependabotAlertsEnabledForNewRepos = new(boolAttr(d, "dependabot_alerts_enabled_for_new_repositories")) } - if includeBool("dependabot_security_updates_enabled_for_new_repositories") { - settings.DependabotSecurityUpdatesEnabledForNewRepos = new(d.Get("dependabot_security_updates_enabled_for_new_repositories").(bool)) + if d.HasChange("dependabot_security_updates_enabled_for_new_repositories") { + settings.DependabotSecurityUpdatesEnabledForNewRepos = new(boolAttr(d, "dependabot_security_updates_enabled_for_new_repositories")) } - if includeBool("dependency_graph_enabled_for_new_repositories") { - settings.DependencyGraphEnabledForNewRepos = new(d.Get("dependency_graph_enabled_for_new_repositories").(bool)) + if d.HasChange("dependency_graph_enabled_for_new_repositories") { + settings.DependencyGraphEnabledForNewRepos = new(boolAttr(d, "dependency_graph_enabled_for_new_repositories")) } - if includeBool("secret_scanning_enabled_for_new_repositories") { - settings.SecretScanningEnabledForNewRepos = new(d.Get("secret_scanning_enabled_for_new_repositories").(bool)) + if d.HasChange("secret_scanning_enabled_for_new_repositories") { + settings.SecretScanningEnabledForNewRepos = new(boolAttr(d, "secret_scanning_enabled_for_new_repositories")) } - if includeBool("secret_scanning_push_protection_enabled_for_new_repositories") { - settings.SecretScanningPushProtectionEnabledForNewRepos = new(d.Get("secret_scanning_push_protection_enabled_for_new_repositories").(bool)) + if d.HasChange("secret_scanning_push_protection_enabled_for_new_repositories") { + settings.SecretScanningPushProtectionEnabledForNewRepos = new(boolAttr(d, "secret_scanning_push_protection_enabled_for_new_repositories")) } - // Enterprise-specific field if isEnterprise { - if includeBool("members_can_create_internal_repositories") { - settings.MembersCanCreateInternalRepos = new(d.Get("members_can_create_internal_repositories").(bool)) + if d.HasChange("members_can_create_internal_repositories") { + settings.MembersCanCreateInternalRepos = new(boolAttr(d, "members_can_create_internal_repositories")) } } return settings } -func resourceGithubOrganizationSettingsCreateOrUpdate(d *schema.ResourceData, meta any) error { - err := checkOrganization(meta) - if err != nil { - return err - } - client := meta.(*Owner).v3client - ctx := context.WithValue(context.Background(), ctxId, d.Id()) - org := meta.(*Owner).name - - orgInfo, _, err := client.Organizations.Get(ctx, org) - if err != nil { - return err - } - - // Build settings using helper function - isEnterprise := orgInfo.GetPlan().GetName() == "enterprise" - settings := buildOrganizationSettings(d, isEnterprise) - - // 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) log.Printf("[DEBUG] Built settings for org %s (enterprise: %v)", org, isEnterprise) if settings.BillingEmail != nil { log.Printf("[DEBUG] BillingEmail: %s", *settings.BillingEmail) @@ -409,8 +482,22 @@ func resourceGithubOrganizationSettingsCreateOrUpdate(d *schema.ResourceData, me if settings.SecretScanningPushProtectionEnabledForNewRepos != nil { log.Printf("[DEBUG] SecretScanningPushProtectionEnabledForNewRepos: %v", *settings.SecretScanningPushProtectionEnabledForNewRepos) } +} - orgSettings, _, err := client.Organizations.Edit(ctx, org, settings) +// organizationPlanIsEnterprise reports whether the organization is on an +// enterprise plan, which gates the enterprise-only attributes. +func organizationPlanIsEnterprise(ctx context.Context, owner *Owner) (bool, error) { + orgInfo, _, err := owner.v3client.Organizations.Get(ctx, owner.name) + if err != nil { + return false, err + } + + return orgInfo.GetPlan().GetName() == "enterprise", nil +} + +// editOrganizationSettings sends the payload and refreshes state from the API. +func editOrganizationSettings(ctx context.Context, d *schema.ResourceData, owner *Owner, settings *github.Organization) diag.Diagnostics { + orgSettings, _, err := owner.v3client.Organizations.Edit(ctx, owner.name, settings) if err != nil { // Log detailed error information for debugging if ghErr, ok := errors.AsType[*github.ErrorResponse](err); ok { @@ -422,129 +509,177 @@ func resourceGithubOrganizationSettingsCreateOrUpdate(d *schema.ResourceData, me } } } - return err + + return diag.FromErr(err) } - id := strconv.FormatInt(orgSettings.GetID(), 10) - d.SetId(id) - return resourceGithubOrganizationSettingsRead(d, meta) + d.SetId(strconv.FormatInt(orgSettings.GetID(), 10)) + + return resourceGithubOrganizationSettingsRead(ctx, d, owner) } -func resourceGithubOrganizationSettingsRead(d *schema.ResourceData, meta any) error { - err := checkOrganization(meta) +func resourceGithubOrganizationSettingsCreate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { + if err := checkOrganization(meta); err != nil { + return diag.FromErr(err) + } + + owner, ok := meta.(*Owner) + if !ok { + return diag.Errorf("unexpected provider meta type %T", meta) + } + + ctx = context.WithValue(ctx, ctxId, d.Id()) + + isEnterprise, err := organizationPlanIsEnterprise(ctx, owner) if err != nil { - return err + return diag.FromErr(err) } - client := meta.(*Owner).v3client - ctx := context.Background() - org := meta.(*Owner).name - orgSettings, _, err := client.Organizations.Get(ctx, org) + settings := organizationSettingsForCreate(d, isEnterprise) + logOrganizationSettings(owner.name, isEnterprise, settings) + + return editOrganizationSettings(ctx, d, owner, settings) +} + +func resourceGithubOrganizationSettingsUpdate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { + if err := checkOrganization(meta); err != nil { + return diag.FromErr(err) + } + + owner, ok := meta.(*Owner) + if !ok { + return diag.Errorf("unexpected provider meta type %T", meta) + } + + ctx = context.WithValue(ctx, ctxId, d.Id()) + + isEnterprise, err := organizationPlanIsEnterprise(ctx, owner) + if err != nil { + return diag.FromErr(err) + } + + settings := organizationSettingsForUpdate(d, isEnterprise) + logOrganizationSettings(owner.name, isEnterprise, settings) + + return editOrganizationSettings(ctx, d, owner, settings) +} + +func resourceGithubOrganizationSettingsRead(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { + if err := checkOrganization(meta); err != nil { + return diag.FromErr(err) + } + + owner, ok := meta.(*Owner) + if !ok { + return diag.Errorf("unexpected provider meta type %T", meta) + } + + orgSettings, _, err := owner.v3client.Organizations.Get(ctx, owner.name) if err != nil { - return err + return diag.FromErr(err) } if err = d.Set("billing_email", orgSettings.GetBillingEmail()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("company", orgSettings.GetCompany()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("email", orgSettings.GetEmail()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("twitter_username", orgSettings.GetTwitterUsername()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("location", orgSettings.GetLocation()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("name", orgSettings.GetName()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("description", orgSettings.GetDescription()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("has_organization_projects", orgSettings.GetHasOrganizationProjects()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("has_repository_projects", orgSettings.GetHasRepositoryProjects()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("default_repository_permission", orgSettings.GetDefaultRepoPermission()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("members_can_create_repositories", orgSettings.GetMembersCanCreateRepos()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("members_can_create_internal_repositories", orgSettings.GetMembersCanCreateInternalRepos()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("members_can_create_private_repositories", orgSettings.GetMembersCanCreatePrivateRepos()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("members_can_create_public_repositories", orgSettings.GetMembersCanCreatePublicRepos()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("members_can_create_pages", orgSettings.GetMembersCanCreatePages()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("members_can_create_public_pages", orgSettings.GetMembersCanCreatePublicPages()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("members_can_create_private_pages", orgSettings.GetMembersCanCreatePrivatePages()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("members_can_fork_private_repositories", orgSettings.GetMembersCanForkPrivateRepos()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("web_commit_signoff_required", orgSettings.GetWebCommitSignoffRequired()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("blog", orgSettings.GetBlog()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("advanced_security_enabled_for_new_repositories", orgSettings.GetAdvancedSecurityEnabledForNewRepos()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("dependabot_alerts_enabled_for_new_repositories", orgSettings.GetDependabotAlertsEnabledForNewRepos()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("dependabot_security_updates_enabled_for_new_repositories", orgSettings.GetDependabotSecurityUpdatesEnabledForNewRepos()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("dependency_graph_enabled_for_new_repositories", orgSettings.GetDependencyGraphEnabledForNewRepos()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("secret_scanning_enabled_for_new_repositories", orgSettings.GetSecretScanningEnabledForNewRepos()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("secret_scanning_push_protection_enabled_for_new_repositories", orgSettings.GetSecretScanningPushProtectionEnabledForNewRepos()); err != nil { - return err + return diag.FromErr(err) } return nil } -func resourceGithubOrganizationSettingsDelete(d *schema.ResourceData, meta any) error { - err := checkOrganization(meta) - if err != nil { - return err +func resourceGithubOrganizationSettingsDelete(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { + if err := checkOrganization(meta); err != nil { + return diag.FromErr(err) } - client := meta.(*Owner).v3client - ctx := context.WithValue(context.Background(), ctxId, d.Id()) - org := meta.(*Owner).name + owner, ok := meta.(*Owner) + if !ok { + return diag.Errorf("unexpected provider meta type %T", meta) + } - log.Printf("[DEBUG] Reverting Organization Settings to default values: %s", org) + ctx = context.WithValue(ctx, ctxId, d.Id()) - // Get organization info to determine if it's enterprise - orgInfo, _, err := client.Organizations.Get(ctx, org) + log.Printf("[DEBUG] Reverting Organization Settings to default values: %s", owner.name) + + isEnterprise, err := organizationPlanIsEnterprise(ctx, owner) if err != nil { - return err + return diag.FromErr(err) } // Build minimal settings with only required fields - isEnterprise := orgInfo.GetPlan().GetName() == "enterprise" defaultSettings := &github.Organization{ BillingEmail: new("email@example.com"), } @@ -554,9 +689,8 @@ func resourceGithubOrganizationSettingsDelete(d *schema.ResourceData, meta any) defaultSettings.MembersCanCreateInternalRepos = new(true) } - _, _, err = client.Organizations.Edit(ctx, org, defaultSettings) - if err != nil { - return err + if _, _, err := owner.v3client.Organizations.Edit(ctx, owner.name, defaultSettings); err != nil { + return diag.FromErr(err) } return nil diff --git a/github/resource_github_organization_settings_test.go b/github/resource_github_organization_settings_test.go index 201d0f9a25..5ebae8d344 100644 --- a/github/resource_github_organization_settings_test.go +++ b/github/resource_github_organization_settings_test.go @@ -617,156 +617,81 @@ func TestAccGithubOrganizationSettings(t *testing.T) { }) } -func Test_buildOrganizationSettings(t *testing.T) { +// Attributes whose schema Default is a non-zero value are reported as +// configured by d.GetOk even when the practitioner omits them, so they appear +// in every create payload. Attributes defaulting to false or "" do not. +func createBaseline() *github.Organization { + return &github.Organization{ + BillingEmail: new("org@example.com"), + HasOrganizationProjects: new(true), + HasRepositoryProjects: new(true), + DefaultRepoPermission: new("read"), + MembersCanCreateRepos: new(true), + MembersCanCreatePrivateRepos: new(true), + MembersCanCreatePublicRepos: new(true), + MembersCanCreatePages: new(true), + MembersCanCreatePublicPages: new(true), + MembersCanCreatePrivatePages: new(true), + } +} + +func Test_organizationSettingsForCreate(t *testing.T) { t.Parallel() for _, tt := range []struct { name string raw map[string]any - id string // a non-empty ID exercises the update path isEnterprise bool want *github.Organization }{ { - // Regression test for the create path dropping booleans configured as - // false, which only corrected itself on a second apply through the - // update/HasChange path. Every boolean carries a definite value on - // create, so the full payload is asserted here. - name: "create_includes_explicitly_false_booleans", + // Documents the known create-path limitation tracked in #3493: + // d.GetOk reports a boolean configured as false the same way it + // reports an unset one, so the attribute never reaches the payload + // and only corrects itself on a second apply via HasChange. + name: "create_drops_explicitly_false_booleans", raw: map[string]any{ "billing_email": "org@example.com", "has_organization_projects": false, "web_commit_signoff_required": false, }, - want: &github.Organization{ - BillingEmail: new("org@example.com"), - HasOrganizationProjects: new(false), - HasRepositoryProjects: new(true), - DefaultRepoPermission: new("read"), - MembersCanCreateRepos: new(true), - MembersCanCreatePrivateRepos: new(true), - MembersCanCreatePublicRepos: new(true), - MembersCanCreatePages: new(true), - MembersCanCreatePublicPages: new(true), - MembersCanCreatePrivatePages: new(true), - MembersCanForkPrivateRepos: new(false), - WebCommitSignoffRequired: new(false), - AdvancedSecurityEnabledForNewRepos: new(false), - DependabotAlertsEnabledForNewRepos: new(false), - DependabotSecurityUpdatesEnabledForNewRepos: new(false), - DependencyGraphEnabledForNewRepos: new(false), - SecretScanningEnabledForNewRepos: new(false), - SecretScanningPushProtectionEnabledForNewRepos: new(false), - }, + want: func() *github.Organization { + want := createBaseline() + want.HasOrganizationProjects = nil + return want + }(), }, { - name: "create_includes_true_booleans", + name: "create_includes_explicitly_true_booleans", raw: map[string]any{ "billing_email": "org@example.com", "web_commit_signoff_required": true, }, - want: &github.Organization{ - BillingEmail: new("org@example.com"), - HasOrganizationProjects: new(true), - HasRepositoryProjects: new(true), - DefaultRepoPermission: new("read"), - MembersCanCreateRepos: new(true), - MembersCanCreatePrivateRepos: new(true), - MembersCanCreatePublicRepos: new(true), - MembersCanCreatePages: new(true), - MembersCanCreatePublicPages: new(true), - MembersCanCreatePrivatePages: new(true), - MembersCanForkPrivateRepos: new(false), - WebCommitSignoffRequired: new(true), - AdvancedSecurityEnabledForNewRepos: new(false), - DependabotAlertsEnabledForNewRepos: new(false), - DependabotSecurityUpdatesEnabledForNewRepos: new(false), - DependencyGraphEnabledForNewRepos: new(false), - SecretScanningEnabledForNewRepos: new(false), - SecretScanningPushProtectionEnabledForNewRepos: new(false), - }, + want: func() *github.Organization { + want := createBaseline() + want.WebCommitSignoffRequired = new(true) + return want + }(), }, { - // Only booleans are included unconditionally on create; an - // unconfigured optional string stays absent from the payload. name: "create_omits_unconfigured_optional_string", raw: map[string]any{ "billing_email": "org@example.com", }, - want: &github.Organization{ - BillingEmail: new("org@example.com"), - HasOrganizationProjects: new(true), - HasRepositoryProjects: new(true), - DefaultRepoPermission: new("read"), - MembersCanCreateRepos: new(true), - MembersCanCreatePrivateRepos: new(true), - MembersCanCreatePublicRepos: new(true), - MembersCanCreatePages: new(true), - MembersCanCreatePublicPages: new(true), - MembersCanCreatePrivatePages: new(true), - MembersCanForkPrivateRepos: new(false), - WebCommitSignoffRequired: new(false), - AdvancedSecurityEnabledForNewRepos: new(false), - DependabotAlertsEnabledForNewRepos: new(false), - DependabotSecurityUpdatesEnabledForNewRepos: new(false), - DependencyGraphEnabledForNewRepos: new(false), - SecretScanningEnabledForNewRepos: new(false), - SecretScanningPushProtectionEnabledForNewRepos: new(false), - }, - }, - { - // On update a field is only sent when it has changed, so the - // explicitly configured has_organization_projects is dropped. The - // attributes that do survive are an artifact of TestResourceDataRaw: - // one left out of raw reads as the zero value from state but as its - // schema default from d.Get, so every attribute defaulting to a - // non-zero value (true, "read") looks changed here. - name: "update_omits_unchanged_booleans", - raw: map[string]any{ - "billing_email": "org@example.com", - "has_organization_projects": false, - }, - id: "example-org", - want: &github.Organization{ - BillingEmail: new("org@example.com"), - HasRepositoryProjects: new(true), - DefaultRepoPermission: new("read"), - MembersCanCreateRepos: new(true), - MembersCanCreatePrivateRepos: new(true), - MembersCanCreatePublicRepos: new(true), - MembersCanCreatePages: new(true), - MembersCanCreatePublicPages: new(true), - MembersCanCreatePrivatePages: new(true), - }, + want: createBaseline(), }, { name: "enterprise_create_includes_internal_repositories_boolean", raw: map[string]any{ "billing_email": "org@example.com", - "members_can_create_internal_repositories": false, + "members_can_create_internal_repositories": true, }, isEnterprise: true, - want: &github.Organization{ - BillingEmail: new("org@example.com"), - HasOrganizationProjects: new(true), - HasRepositoryProjects: new(true), - DefaultRepoPermission: new("read"), - MembersCanCreateRepos: new(true), - MembersCanCreateInternalRepos: new(false), - MembersCanCreatePrivateRepos: new(true), - MembersCanCreatePublicRepos: new(true), - MembersCanCreatePages: new(true), - MembersCanCreatePublicPages: new(true), - MembersCanCreatePrivatePages: new(true), - MembersCanForkPrivateRepos: new(false), - WebCommitSignoffRequired: new(false), - AdvancedSecurityEnabledForNewRepos: new(false), - DependabotAlertsEnabledForNewRepos: new(false), - DependabotSecurityUpdatesEnabledForNewRepos: new(false), - DependencyGraphEnabledForNewRepos: new(false), - SecretScanningEnabledForNewRepos: new(false), - SecretScanningPushProtectionEnabledForNewRepos: new(false), - }, + want: func() *github.Organization { + want := createBaseline() + want.MembersCanCreateInternalRepos = new(true) + return want + }(), }, { // The enterprise-only attribute must never reach the payload for a @@ -774,42 +699,78 @@ func Test_buildOrganizationSettings(t *testing.T) { name: "non_enterprise_create_omits_internal_repositories_boolean", raw: map[string]any{ "billing_email": "org@example.com", - "members_can_create_internal_repositories": false, - }, - want: &github.Organization{ - BillingEmail: new("org@example.com"), - HasOrganizationProjects: new(true), - HasRepositoryProjects: new(true), - DefaultRepoPermission: new("read"), - MembersCanCreateRepos: new(true), - MembersCanCreatePrivateRepos: new(true), - MembersCanCreatePublicRepos: new(true), - MembersCanCreatePages: new(true), - MembersCanCreatePublicPages: new(true), - MembersCanCreatePrivatePages: new(true), - MembersCanForkPrivateRepos: new(false), - WebCommitSignoffRequired: new(false), - AdvancedSecurityEnabledForNewRepos: new(false), - DependabotAlertsEnabledForNewRepos: new(false), - DependabotSecurityUpdatesEnabledForNewRepos: new(false), - DependencyGraphEnabledForNewRepos: new(false), - SecretScanningEnabledForNewRepos: new(false), - SecretScanningPushProtectionEnabledForNewRepos: new(false), + "members_can_create_internal_repositories": true, }, + want: createBaseline(), }, } { t.Run(tt.name, func(t *testing.T) { t.Parallel() d := schema.TestResourceDataRaw(t, resourceGithubOrganizationSettings().Schema, tt.raw) - if tt.id != "" { - d.SetId(tt.id) + + got := organizationSettingsForCreate(d, tt.isEnterprise) + + if diff := cmp.Diff(tt.want, got); diff != "" { + t.Errorf("organizationSettingsForCreate() mismatch (-want +got):\n%s", diff) } + }) + } +} + +func Test_organizationSettingsForUpdate(t *testing.T) { + t.Parallel() + + for _, tt := range []struct { + name string + raw map[string]any + isEnterprise bool + want *github.Organization + }{ + { + // Only changed attributes are sent, which is what keeps + // unconfigured attributes out of the request (#2305). The + // attributes that do survive here are an artifact of + // TestResourceDataRaw: one left out of raw reads as the zero value + // from state but as its schema default from d.Get, so every + // attribute defaulting to a non-zero value looks changed. + name: "update_omits_unchanged_booleans", + raw: map[string]any{ + "billing_email": "org@example.com", + "has_organization_projects": false, + }, + want: func() *github.Organization { + want := createBaseline() + want.HasOrganizationProjects = nil + return want + }(), + }, + { + // A configured optional string is a change against empty state, so + // it is included; the create path reaches the same result through + // GetOk instead. + name: "update_includes_changed_optional_string", + raw: map[string]any{ + "billing_email": "org@example.com", + "description": "example description", + }, + want: func() *github.Organization { + want := createBaseline() + want.Description = new("example description") + return want + }(), + }, + } { + t.Run(tt.name, func(t *testing.T) { + t.Parallel() + + d := schema.TestResourceDataRaw(t, resourceGithubOrganizationSettings().Schema, tt.raw) + d.SetId("example-org") - got := buildOrganizationSettings(d, tt.isEnterprise) + got := organizationSettingsForUpdate(d, tt.isEnterprise) if diff := cmp.Diff(tt.want, got); diff != "" { - t.Errorf("buildOrganizationSettings() mismatch (-want +got):\n%s", diff) + t.Errorf("organizationSettingsForUpdate() mismatch (-want +got):\n%s", diff) } }) } From 94dd7ca7c0eaeb880e31de0a7a7f5c6e8a0c8d4b Mon Sep 17 00:00:00 2001 From: somaz Date: Thu, 10 Sep 2026 11:24:49 +0900 Subject: [PATCH 06/11] fix: decide organization_settings create payload from the raw config --- .../resource_github_organization_settings.go | 93 ++++++++++------ ...ource_github_organization_settings_test.go | 104 ++++++++++++++++-- 2 files changed, 157 insertions(+), 40 deletions(-) diff --git a/github/resource_github_organization_settings.go b/github/resource_github_organization_settings.go index 9593d63cb1..730e527ecb 100644 --- a/github/resource_github_organization_settings.go +++ b/github/resource_github_organization_settings.go @@ -7,6 +7,7 @@ import ( "strconv" "github.com/google/go-github/v92/github" + "github.com/hashicorp/go-cty/cty" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" @@ -191,97 +192,127 @@ func boolAttr(d *schema.ResourceData, name string) bool { return v } +// 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() +} + // organizationSettingsForCreate builds the payload sent when the resource is -// created. An attribute is included only when d.GetOk reports it as configured. +// created. An attribute is included only when the user explicitly set it, so +// unconfigured attributes stay out of the request and the org keeps whatever +// the API defaults to. Keeping the payload narrow is what avoids the +// validation errors reported in #2305. // -// Note that d.GetOk cannot distinguish an explicitly configured false from an -// unset attribute, so a boolean configured as false is currently dropped from -// the create payload. That behaviour is unchanged here and is tracked -// separately in #3493. +// Inclusion is decided from the raw configuration rather than d.GetOk, which +// cannot tell an explicitly configured false from an unset attribute and so +// used to drop a boolean configured as false from the create payload (#3493). func organizationSettingsForCreate(d *schema.ResourceData, isEnterprise bool) *github.Organization { settings := &github.Organization{} // The API rejects the request without billing_email, so it is always sent. - if _, ok := d.GetOk("billing_email"); ok { + if isConfigured(d, "billing_email") { settings.BillingEmail = new(stringAttr(d, "billing_email")) } - if _, ok := d.GetOk("company"); ok { + if isConfigured(d, "company") { settings.Company = new(stringAttr(d, "company")) } - if _, ok := d.GetOk("email"); ok { + if isConfigured(d, "email") { settings.Email = new(stringAttr(d, "email")) } - if _, ok := d.GetOk("twitter_username"); ok { + if isConfigured(d, "twitter_username") { settings.TwitterUsername = new(stringAttr(d, "twitter_username")) } - if _, ok := d.GetOk("location"); ok { + if isConfigured(d, "location") { settings.Location = new(stringAttr(d, "location")) } - if _, ok := d.GetOk("name"); ok { + if isConfigured(d, "name") { settings.Name = new(stringAttr(d, "name")) } - if _, ok := d.GetOk("description"); ok { + if isConfigured(d, "description") { settings.Description = new(stringAttr(d, "description")) } - if _, ok := d.GetOk("blog"); ok { + if isConfigured(d, "blog") { settings.Blog = new(stringAttr(d, "blog")) } - if _, ok := d.GetOk("has_organization_projects"); ok { + if isConfigured(d, "has_organization_projects") { settings.HasOrganizationProjects = new(boolAttr(d, "has_organization_projects")) } - if _, ok := d.GetOk("has_repository_projects"); ok { + if isConfigured(d, "has_repository_projects") { settings.HasRepositoryProjects = new(boolAttr(d, "has_repository_projects")) } - if _, ok := d.GetOk("default_repository_permission"); ok { + if isConfigured(d, "default_repository_permission") { settings.DefaultRepoPermission = new(stringAttr(d, "default_repository_permission")) } - if _, ok := d.GetOk("members_can_create_repositories"); ok { + if isConfigured(d, "members_can_create_repositories") { settings.MembersCanCreateRepos = new(boolAttr(d, "members_can_create_repositories")) } - if _, ok := d.GetOk("members_can_create_private_repositories"); ok { + if isConfigured(d, "members_can_create_private_repositories") { settings.MembersCanCreatePrivateRepos = new(boolAttr(d, "members_can_create_private_repositories")) } - if _, ok := d.GetOk("members_can_create_public_repositories"); ok { + if isConfigured(d, "members_can_create_public_repositories") { settings.MembersCanCreatePublicRepos = new(boolAttr(d, "members_can_create_public_repositories")) } - if _, ok := d.GetOk("members_can_create_pages"); ok { + if isConfigured(d, "members_can_create_pages") { settings.MembersCanCreatePages = new(boolAttr(d, "members_can_create_pages")) } - if _, ok := d.GetOk("members_can_create_public_pages"); ok { + if isConfigured(d, "members_can_create_public_pages") { settings.MembersCanCreatePublicPages = new(boolAttr(d, "members_can_create_public_pages")) } - if _, ok := d.GetOk("members_can_create_private_pages"); ok { + if isConfigured(d, "members_can_create_private_pages") { settings.MembersCanCreatePrivatePages = new(boolAttr(d, "members_can_create_private_pages")) } - if _, ok := d.GetOk("members_can_fork_private_repositories"); ok { + if isConfigured(d, "members_can_fork_private_repositories") { settings.MembersCanForkPrivateRepos = new(boolAttr(d, "members_can_fork_private_repositories")) } - if _, ok := d.GetOk("web_commit_signoff_required"); ok { + if isConfigured(d, "web_commit_signoff_required") { settings.WebCommitSignoffRequired = new(boolAttr(d, "web_commit_signoff_required")) } - if _, ok := d.GetOk("advanced_security_enabled_for_new_repositories"); ok { + if isConfigured(d, "advanced_security_enabled_for_new_repositories") { settings.AdvancedSecurityEnabledForNewRepos = new(boolAttr(d, "advanced_security_enabled_for_new_repositories")) } - if _, ok := d.GetOk("dependabot_alerts_enabled_for_new_repositories"); ok { + if isConfigured(d, "dependabot_alerts_enabled_for_new_repositories") { settings.DependabotAlertsEnabledForNewRepos = new(boolAttr(d, "dependabot_alerts_enabled_for_new_repositories")) } - if _, ok := d.GetOk("dependabot_security_updates_enabled_for_new_repositories"); ok { + if isConfigured(d, "dependabot_security_updates_enabled_for_new_repositories") { settings.DependabotSecurityUpdatesEnabledForNewRepos = new(boolAttr(d, "dependabot_security_updates_enabled_for_new_repositories")) } - if _, ok := d.GetOk("dependency_graph_enabled_for_new_repositories"); ok { + if isConfigured(d, "dependency_graph_enabled_for_new_repositories") { settings.DependencyGraphEnabledForNewRepos = new(boolAttr(d, "dependency_graph_enabled_for_new_repositories")) } - if _, ok := d.GetOk("secret_scanning_enabled_for_new_repositories"); ok { + if isConfigured(d, "secret_scanning_enabled_for_new_repositories") { settings.SecretScanningEnabledForNewRepos = new(boolAttr(d, "secret_scanning_enabled_for_new_repositories")) } - if _, ok := d.GetOk("secret_scanning_push_protection_enabled_for_new_repositories"); ok { + if isConfigured(d, "secret_scanning_push_protection_enabled_for_new_repositories") { settings.SecretScanningPushProtectionEnabledForNewRepos = new(boolAttr(d, "secret_scanning_push_protection_enabled_for_new_repositories")) } if isEnterprise { - if _, ok := d.GetOk("members_can_create_internal_repositories"); ok { + if isConfigured(d, "members_can_create_internal_repositories") { settings.MembersCanCreateInternalRepos = new(boolAttr(d, "members_can_create_internal_repositories")) } } diff --git a/github/resource_github_organization_settings_test.go b/github/resource_github_organization_settings_test.go index 5ebae8d344..d60bcf0543 100644 --- a/github/resource_github_organization_settings_test.go +++ b/github/resource_github_organization_settings_test.go @@ -2,11 +2,14 @@ package github import ( "fmt" + "strconv" "testing" "github.com/google/go-cmp/cmp" "github.com/google/go-github/v89/github" + "github.com/hashicorp/go-cty/cty" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" + "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" "github.com/hashicorp/terraform-plugin-testing/helper/resource" ) @@ -620,7 +623,78 @@ func TestAccGithubOrganizationSettings(t *testing.T) { // Attributes whose schema Default is a non-zero value are reported as // configured by d.GetOk even when the practitioner omits them, so they appear // in every create payload. Attributes defaulting to false or "" do not. +// testResourceDataWithConfig builds ResourceData carrying both the attributes +// d.Get reads and the raw configuration isConfigured reads. Attributes absent +// from raw are recorded as null in the configuration, which is how an unset +// attribute is told apart from one explicitly configured as false. +// +// schema.TestResourceDataRaw cannot be used here: it leaves the raw +// configuration null, and that is the one distinction these cases exercise. +func testResourceDataWithConfig(t *testing.T, raw map[string]any) *schema.ResourceData { + t.Helper() + + res := resourceGithubOrganizationSettings() + attributes := map[string]string{} + config := map[string]cty.Value{} + + for name, attrSchema := range res.Schema { + value, configured := raw[name] + + switch attrSchema.Type { + case schema.TypeBool: + if !configured { + config[name] = cty.NullVal(cty.Bool) + + continue + } + + b, ok := value.(bool) + if !ok { + t.Fatalf("attribute %q is a bool in the schema but %T in the test case", name, value) + } + attributes[name] = strconv.FormatBool(b) + config[name] = cty.BoolVal(b) + case schema.TypeString: + if !configured { + config[name] = cty.NullVal(cty.String) + + continue + } + + s, ok := value.(string) + if !ok { + t.Fatalf("attribute %q is a string in the schema but %T in the test case", name, value) + } + attributes[name] = s + config[name] = cty.StringVal(s) + default: + t.Fatalf("attribute %q has unsupported type %s; extend this helper", name, attrSchema.Type) + } + } + + return res.Data(&terraform.InstanceState{ + Attributes: attributes, + RawConfig: cty.ObjectVal(config), + }) +} + func createBaseline() *github.Organization { + // Only attributes the configuration actually sets reach the create payload, + // so a case that configures nothing but billing_email sends nothing else. + // Attributes left out keep whatever the API defaults to, which is what + // keeps the request narrow enough to avoid #2305. + return &github.Organization{ + BillingEmail: new("org@example.com"), + } +} + +// updateBaseline is the update-path counterpart of createBaseline. The update +// payload is driven by d.HasChange, and under schema.TestResourceDataRaw an +// attribute left out of raw reads as the zero value from state but as its +// schema default from d.Get, so every attribute whose default is non-zero +// looks changed. These entries are that artifact, not a behaviour the +// provider intends. +func updateBaseline() *github.Organization { return &github.Organization{ BillingEmail: new("org@example.com"), HasOrganizationProjects: new(true), @@ -645,11 +719,12 @@ func Test_organizationSettingsForCreate(t *testing.T) { want *github.Organization }{ { - // Documents the known create-path limitation tracked in #3493: - // d.GetOk reports a boolean configured as false the same way it - // reports an unset one, so the attribute never reaches the payload - // and only corrects itself on a second apply via HasChange. - name: "create_drops_explicitly_false_booleans", + // The bug reported in #3493: a boolean configured as false used to + // be dropped from the create payload, so the API applied its own + // default and the first apply silently produced the opposite of the + // configuration. Both attributes here are explicitly false and both + // must reach the payload. + name: "create_includes_explicitly_false_booleans", raw: map[string]any{ "billing_email": "org@example.com", "has_organization_projects": false, @@ -657,10 +732,21 @@ func Test_organizationSettingsForCreate(t *testing.T) { }, want: func() *github.Organization { want := createBaseline() - want.HasOrganizationProjects = nil + want.HasOrganizationProjects = new(false) + want.WebCommitSignoffRequired = new(false) return want }(), }, + { + // An attribute the user never wrote must stay out of the payload + // even though its schema default is a definite true, so the org + // keeps whatever the API defaults to. + name: "create_omits_unconfigured_default_true_boolean", + raw: map[string]any{ + "billing_email": "org@example.com", + }, + want: createBaseline(), + }, { name: "create_includes_explicitly_true_booleans", raw: map[string]any{ @@ -707,7 +793,7 @@ func Test_organizationSettingsForCreate(t *testing.T) { t.Run(tt.name, func(t *testing.T) { t.Parallel() - d := schema.TestResourceDataRaw(t, resourceGithubOrganizationSettings().Schema, tt.raw) + d := testResourceDataWithConfig(t, tt.raw) got := organizationSettingsForCreate(d, tt.isEnterprise) @@ -740,7 +826,7 @@ func Test_organizationSettingsForUpdate(t *testing.T) { "has_organization_projects": false, }, want: func() *github.Organization { - want := createBaseline() + want := updateBaseline() want.HasOrganizationProjects = nil return want }(), @@ -755,7 +841,7 @@ func Test_organizationSettingsForUpdate(t *testing.T) { "description": "example description", }, want: func() *github.Organization { - want := createBaseline() + want := updateBaseline() want.Description = new("example description") return want }(), From f79d0aecee01e51da7bbf439bdf9ba517968b48e Mon Sep 17 00:00:00 2001 From: somaz Date: Thu, 10 Sep 2026 15:39:43 +0900 Subject: [PATCH 07/11] fix: keep non-zero schema defaults in the organization_settings create payload --- .../resource_github_organization_settings.go | 89 ++++++++++++------- ...ource_github_organization_settings_test.go | 43 ++++++--- 2 files changed, 86 insertions(+), 46 deletions(-) diff --git a/github/resource_github_organization_settings.go b/github/resource_github_organization_settings.go index 730e527ecb..42036b5cf3 100644 --- a/github/resource_github_organization_settings.go +++ b/github/resource_github_organization_settings.go @@ -220,99 +220,122 @@ func isConfigured(d *schema.ResourceData, name string) bool { return !v.IsNull() } +// sendOnCreate reports whether the named attribute belongs in the create +// payload: when the user explicitly configured it, or when d.GetOk reports it +// because its schema default is a non-zero value. +// +// The second half keeps the create payload identical to what #2807 sent. +// Attributes with a non-zero default (has_organization_projects, +// default_repository_permission, ...) have always been sent on create, and +// leaving them out would let the org keep a value that differs from the +// schema default, so the first plan after create is not clean and only the +// second apply converges. Attributes defaulting to false or "" are still +// only sent when explicitly configured, which is what keeps the request +// narrow enough to avoid the validation errors reported in #2305. +func sendOnCreate(d *schema.ResourceData, name string) bool { + if isConfigured(d, name) { + return true + } + + _, ok := d.GetOk(name) + + return ok +} + // organizationSettingsForCreate builds the payload sent when the resource is -// created. An attribute is included only when the user explicitly set it, so -// unconfigured attributes stay out of the request and the org keeps whatever -// the API defaults to. Keeping the payload narrow is what avoids the -// validation errors reported in #2305. +// created. An attribute is included when the user explicitly set it or when +// its schema default is a non-zero value; everything else stays out of the +// request and the org keeps whatever the API defaults to. Keeping the payload +// narrow is what avoids the validation errors reported in #2305. // -// Inclusion is decided from the raw configuration rather than d.GetOk, which -// cannot tell an explicitly configured false from an unset attribute and so -// used to drop a boolean configured as false from the create payload (#3493). +// Explicit configuration is read from the raw configuration rather than +// d.GetOk, which cannot tell an explicitly configured false from an unset +// attribute and so used to drop a boolean configured as false from the +// create payload (#3493). func organizationSettingsForCreate(d *schema.ResourceData, isEnterprise bool) *github.Organization { settings := &github.Organization{} // The API rejects the request without billing_email, so it is always sent. - if isConfigured(d, "billing_email") { + if sendOnCreate(d, "billing_email") { settings.BillingEmail = new(stringAttr(d, "billing_email")) } - if isConfigured(d, "company") { + if sendOnCreate(d, "company") { settings.Company = new(stringAttr(d, "company")) } - if isConfigured(d, "email") { + if sendOnCreate(d, "email") { settings.Email = new(stringAttr(d, "email")) } - if isConfigured(d, "twitter_username") { + if sendOnCreate(d, "twitter_username") { settings.TwitterUsername = new(stringAttr(d, "twitter_username")) } - if isConfigured(d, "location") { + if sendOnCreate(d, "location") { settings.Location = new(stringAttr(d, "location")) } - if isConfigured(d, "name") { + if sendOnCreate(d, "name") { settings.Name = new(stringAttr(d, "name")) } - if isConfigured(d, "description") { + if sendOnCreate(d, "description") { settings.Description = new(stringAttr(d, "description")) } - if isConfigured(d, "blog") { + if sendOnCreate(d, "blog") { settings.Blog = new(stringAttr(d, "blog")) } - if isConfigured(d, "has_organization_projects") { + if sendOnCreate(d, "has_organization_projects") { settings.HasOrganizationProjects = new(boolAttr(d, "has_organization_projects")) } - if isConfigured(d, "has_repository_projects") { + if sendOnCreate(d, "has_repository_projects") { settings.HasRepositoryProjects = new(boolAttr(d, "has_repository_projects")) } - if isConfigured(d, "default_repository_permission") { + if sendOnCreate(d, "default_repository_permission") { settings.DefaultRepoPermission = new(stringAttr(d, "default_repository_permission")) } - if isConfigured(d, "members_can_create_repositories") { + if sendOnCreate(d, "members_can_create_repositories") { settings.MembersCanCreateRepos = new(boolAttr(d, "members_can_create_repositories")) } - if isConfigured(d, "members_can_create_private_repositories") { + if sendOnCreate(d, "members_can_create_private_repositories") { settings.MembersCanCreatePrivateRepos = new(boolAttr(d, "members_can_create_private_repositories")) } - if isConfigured(d, "members_can_create_public_repositories") { + if sendOnCreate(d, "members_can_create_public_repositories") { settings.MembersCanCreatePublicRepos = new(boolAttr(d, "members_can_create_public_repositories")) } - if isConfigured(d, "members_can_create_pages") { + if sendOnCreate(d, "members_can_create_pages") { settings.MembersCanCreatePages = new(boolAttr(d, "members_can_create_pages")) } - if isConfigured(d, "members_can_create_public_pages") { + if sendOnCreate(d, "members_can_create_public_pages") { settings.MembersCanCreatePublicPages = new(boolAttr(d, "members_can_create_public_pages")) } - if isConfigured(d, "members_can_create_private_pages") { + if sendOnCreate(d, "members_can_create_private_pages") { settings.MembersCanCreatePrivatePages = new(boolAttr(d, "members_can_create_private_pages")) } - if isConfigured(d, "members_can_fork_private_repositories") { + if sendOnCreate(d, "members_can_fork_private_repositories") { settings.MembersCanForkPrivateRepos = new(boolAttr(d, "members_can_fork_private_repositories")) } - if isConfigured(d, "web_commit_signoff_required") { + if sendOnCreate(d, "web_commit_signoff_required") { settings.WebCommitSignoffRequired = new(boolAttr(d, "web_commit_signoff_required")) } - if isConfigured(d, "advanced_security_enabled_for_new_repositories") { + if sendOnCreate(d, "advanced_security_enabled_for_new_repositories") { settings.AdvancedSecurityEnabledForNewRepos = new(boolAttr(d, "advanced_security_enabled_for_new_repositories")) } - if isConfigured(d, "dependabot_alerts_enabled_for_new_repositories") { + if sendOnCreate(d, "dependabot_alerts_enabled_for_new_repositories") { settings.DependabotAlertsEnabledForNewRepos = new(boolAttr(d, "dependabot_alerts_enabled_for_new_repositories")) } - if isConfigured(d, "dependabot_security_updates_enabled_for_new_repositories") { + if sendOnCreate(d, "dependabot_security_updates_enabled_for_new_repositories") { settings.DependabotSecurityUpdatesEnabledForNewRepos = new(boolAttr(d, "dependabot_security_updates_enabled_for_new_repositories")) } - if isConfigured(d, "dependency_graph_enabled_for_new_repositories") { + if sendOnCreate(d, "dependency_graph_enabled_for_new_repositories") { settings.DependencyGraphEnabledForNewRepos = new(boolAttr(d, "dependency_graph_enabled_for_new_repositories")) } - if isConfigured(d, "secret_scanning_enabled_for_new_repositories") { + if sendOnCreate(d, "secret_scanning_enabled_for_new_repositories") { settings.SecretScanningEnabledForNewRepos = new(boolAttr(d, "secret_scanning_enabled_for_new_repositories")) } - if isConfigured(d, "secret_scanning_push_protection_enabled_for_new_repositories") { + if sendOnCreate(d, "secret_scanning_push_protection_enabled_for_new_repositories") { settings.SecretScanningPushProtectionEnabledForNewRepos = new(boolAttr(d, "secret_scanning_push_protection_enabled_for_new_repositories")) } if isEnterprise { - if isConfigured(d, "members_can_create_internal_repositories") { + if sendOnCreate(d, "members_can_create_internal_repositories") { settings.MembersCanCreateInternalRepos = new(boolAttr(d, "members_can_create_internal_repositories")) } } diff --git a/github/resource_github_organization_settings_test.go b/github/resource_github_organization_settings_test.go index d60bcf0543..21dfbe7767 100644 --- a/github/resource_github_organization_settings_test.go +++ b/github/resource_github_organization_settings_test.go @@ -620,13 +620,12 @@ func TestAccGithubOrganizationSettings(t *testing.T) { }) } -// Attributes whose schema Default is a non-zero value are reported as -// configured by d.GetOk even when the practitioner omits them, so they appear -// in every create payload. Attributes defaulting to false or "" do not. // testResourceDataWithConfig builds ResourceData carrying both the attributes // d.Get reads and the raw configuration isConfigured reads. Attributes absent // from raw are recorded as null in the configuration, which is how an unset -// attribute is told apart from one explicitly configured as false. +// attribute is told apart from one explicitly configured as false, and carry +// their schema Default in the attributes, which is what the planned state +// holds during a real create and what d.GetOk answers from. // // schema.TestResourceDataRaw cannot be used here: it leaves the raw // configuration null, and that is the one distinction these cases exercise. @@ -644,6 +643,9 @@ func testResourceDataWithConfig(t *testing.T, raw map[string]any) *schema.Resour case schema.TypeBool: if !configured { config[name] = cty.NullVal(cty.Bool) + if def, ok := attrSchema.Default.(bool); ok { + attributes[name] = strconv.FormatBool(def) + } continue } @@ -657,6 +659,9 @@ func testResourceDataWithConfig(t *testing.T, raw map[string]any) *schema.Resour case schema.TypeString: if !configured { config[name] = cty.NullVal(cty.String) + if def, ok := attrSchema.Default.(string); ok { + attributes[name] = def + } continue } @@ -678,13 +683,24 @@ func testResourceDataWithConfig(t *testing.T, raw map[string]any) *schema.Resour }) } +// createBaseline is the payload a configuration setting nothing but +// billing_email produces. Attributes whose schema Default is a non-zero value +// are reported by d.GetOk even when the practitioner omits them, so they are +// sent on every create, exactly as #2807 did. Attributes defaulting to false +// or "" stay out unless configured, which keeps the request narrow enough to +// avoid #2305. func createBaseline() *github.Organization { - // Only attributes the configuration actually sets reach the create payload, - // so a case that configures nothing but billing_email sends nothing else. - // Attributes left out keep whatever the API defaults to, which is what - // keeps the request narrow enough to avoid #2305. return &github.Organization{ - BillingEmail: new("org@example.com"), + BillingEmail: new("org@example.com"), + HasOrganizationProjects: new(true), + HasRepositoryProjects: new(true), + DefaultRepoPermission: new("read"), + MembersCanCreateRepos: new(true), + MembersCanCreatePrivateRepos: new(true), + MembersCanCreatePublicRepos: new(true), + MembersCanCreatePages: new(true), + MembersCanCreatePublicPages: new(true), + MembersCanCreatePrivatePages: new(true), } } @@ -738,10 +754,11 @@ func Test_organizationSettingsForCreate(t *testing.T) { }(), }, { - // An attribute the user never wrote must stay out of the payload - // even though its schema default is a definite true, so the org - // keeps whatever the API defaults to. - name: "create_omits_unconfigured_default_true_boolean", + // An attribute the user never wrote is still sent when its schema + // default is a definite true, as #2807 already did. Leaving it out + // would let the org keep a differing value, so the first plan after + // create would not be clean. + name: "create_includes_unconfigured_default_true_boolean", raw: map[string]any{ "billing_email": "org@example.com", }, From 4ef176a772246b8dacdb45410f3c744f65c5ec88 Mon Sep 17 00:00:00 2001 From: somaz Date: Fri, 11 Sep 2026 11:25:34 +0900 Subject: [PATCH 08/11] test: replace organization_settings unit tests with an acceptance test --- ...ource_github_organization_settings_test.go | 303 +++--------------- 1 file changed, 38 insertions(+), 265 deletions(-) diff --git a/github/resource_github_organization_settings_test.go b/github/resource_github_organization_settings_test.go index 21dfbe7767..21b55843c5 100644 --- a/github/resource_github_organization_settings_test.go +++ b/github/resource_github_organization_settings_test.go @@ -2,14 +2,8 @@ package github import ( "fmt" - "strconv" "testing" - "github.com/google/go-cmp/cmp" - "github.com/google/go-github/v89/github" - "github.com/hashicorp/go-cty/cty" - "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" - "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" "github.com/hashicorp/terraform-plugin-testing/helper/resource" ) @@ -228,6 +222,44 @@ func TestAccGithubOrganizationSettings(t *testing.T) { }) }) + t.Run("sends booleans configured as false on create", func(t *testing.T) { + // These attributes default to true, so a false value used to be dropped + // from the create payload and only reached the API on a second apply + // (#3493). The plan-only step fails if the first apply did not converge. + config := ` + resource "github_organization_settings" "test" { + billing_email = "test@example.com" + has_organization_projects = false + has_repository_projects = false + members_can_create_pages = false + members_can_create_public_pages = false + members_can_create_private_pages = false + }` + + check := resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr("github_organization_settings.test", "has_organization_projects", "false"), + resource.TestCheckResourceAttr("github_organization_settings.test", "has_repository_projects", "false"), + resource.TestCheckResourceAttr("github_organization_settings.test", "members_can_create_pages", "false"), + resource.TestCheckResourceAttr("github_organization_settings.test", "members_can_create_public_pages", "false"), + resource.TestCheckResourceAttr("github_organization_settings.test", "members_can_create_private_pages", "false"), + ) + + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnlessHasOrgs(t) }, + ProviderFactories: providerFactories, + Steps: []resource.TestStep{ + { + Config: config, + Check: check, + }, + { + Config: config, + PlanOnly: true, + }, + }, + }) + }) + t.Run("handles mixed boolean values correctly", func(t *testing.T) { config := ` resource "github_organization_settings" "test" { @@ -619,262 +651,3 @@ func TestAccGithubOrganizationSettings(t *testing.T) { }) }) } - -// testResourceDataWithConfig builds ResourceData carrying both the attributes -// d.Get reads and the raw configuration isConfigured reads. Attributes absent -// from raw are recorded as null in the configuration, which is how an unset -// attribute is told apart from one explicitly configured as false, and carry -// their schema Default in the attributes, which is what the planned state -// holds during a real create and what d.GetOk answers from. -// -// schema.TestResourceDataRaw cannot be used here: it leaves the raw -// configuration null, and that is the one distinction these cases exercise. -func testResourceDataWithConfig(t *testing.T, raw map[string]any) *schema.ResourceData { - t.Helper() - - res := resourceGithubOrganizationSettings() - attributes := map[string]string{} - config := map[string]cty.Value{} - - for name, attrSchema := range res.Schema { - value, configured := raw[name] - - switch attrSchema.Type { - case schema.TypeBool: - if !configured { - config[name] = cty.NullVal(cty.Bool) - if def, ok := attrSchema.Default.(bool); ok { - attributes[name] = strconv.FormatBool(def) - } - - continue - } - - b, ok := value.(bool) - if !ok { - t.Fatalf("attribute %q is a bool in the schema but %T in the test case", name, value) - } - attributes[name] = strconv.FormatBool(b) - config[name] = cty.BoolVal(b) - case schema.TypeString: - if !configured { - config[name] = cty.NullVal(cty.String) - if def, ok := attrSchema.Default.(string); ok { - attributes[name] = def - } - - continue - } - - s, ok := value.(string) - if !ok { - t.Fatalf("attribute %q is a string in the schema but %T in the test case", name, value) - } - attributes[name] = s - config[name] = cty.StringVal(s) - default: - t.Fatalf("attribute %q has unsupported type %s; extend this helper", name, attrSchema.Type) - } - } - - return res.Data(&terraform.InstanceState{ - Attributes: attributes, - RawConfig: cty.ObjectVal(config), - }) -} - -// createBaseline is the payload a configuration setting nothing but -// billing_email produces. Attributes whose schema Default is a non-zero value -// are reported by d.GetOk even when the practitioner omits them, so they are -// sent on every create, exactly as #2807 did. Attributes defaulting to false -// or "" stay out unless configured, which keeps the request narrow enough to -// avoid #2305. -func createBaseline() *github.Organization { - return &github.Organization{ - BillingEmail: new("org@example.com"), - HasOrganizationProjects: new(true), - HasRepositoryProjects: new(true), - DefaultRepoPermission: new("read"), - MembersCanCreateRepos: new(true), - MembersCanCreatePrivateRepos: new(true), - MembersCanCreatePublicRepos: new(true), - MembersCanCreatePages: new(true), - MembersCanCreatePublicPages: new(true), - MembersCanCreatePrivatePages: new(true), - } -} - -// updateBaseline is the update-path counterpart of createBaseline. The update -// payload is driven by d.HasChange, and under schema.TestResourceDataRaw an -// attribute left out of raw reads as the zero value from state but as its -// schema default from d.Get, so every attribute whose default is non-zero -// looks changed. These entries are that artifact, not a behaviour the -// provider intends. -func updateBaseline() *github.Organization { - return &github.Organization{ - BillingEmail: new("org@example.com"), - HasOrganizationProjects: new(true), - HasRepositoryProjects: new(true), - DefaultRepoPermission: new("read"), - MembersCanCreateRepos: new(true), - MembersCanCreatePrivateRepos: new(true), - MembersCanCreatePublicRepos: new(true), - MembersCanCreatePages: new(true), - MembersCanCreatePublicPages: new(true), - MembersCanCreatePrivatePages: new(true), - } -} - -func Test_organizationSettingsForCreate(t *testing.T) { - t.Parallel() - - for _, tt := range []struct { - name string - raw map[string]any - isEnterprise bool - want *github.Organization - }{ - { - // The bug reported in #3493: a boolean configured as false used to - // be dropped from the create payload, so the API applied its own - // default and the first apply silently produced the opposite of the - // configuration. Both attributes here are explicitly false and both - // must reach the payload. - name: "create_includes_explicitly_false_booleans", - raw: map[string]any{ - "billing_email": "org@example.com", - "has_organization_projects": false, - "web_commit_signoff_required": false, - }, - want: func() *github.Organization { - want := createBaseline() - want.HasOrganizationProjects = new(false) - want.WebCommitSignoffRequired = new(false) - return want - }(), - }, - { - // An attribute the user never wrote is still sent when its schema - // default is a definite true, as #2807 already did. Leaving it out - // would let the org keep a differing value, so the first plan after - // create would not be clean. - name: "create_includes_unconfigured_default_true_boolean", - raw: map[string]any{ - "billing_email": "org@example.com", - }, - want: createBaseline(), - }, - { - name: "create_includes_explicitly_true_booleans", - raw: map[string]any{ - "billing_email": "org@example.com", - "web_commit_signoff_required": true, - }, - want: func() *github.Organization { - want := createBaseline() - want.WebCommitSignoffRequired = new(true) - return want - }(), - }, - { - name: "create_omits_unconfigured_optional_string", - raw: map[string]any{ - "billing_email": "org@example.com", - }, - want: createBaseline(), - }, - { - name: "enterprise_create_includes_internal_repositories_boolean", - raw: map[string]any{ - "billing_email": "org@example.com", - "members_can_create_internal_repositories": true, - }, - isEnterprise: true, - want: func() *github.Organization { - want := createBaseline() - want.MembersCanCreateInternalRepos = new(true) - return want - }(), - }, - { - // The enterprise-only attribute must never reach the payload for a - // non-enterprise organization, even when it is configured. - name: "non_enterprise_create_omits_internal_repositories_boolean", - raw: map[string]any{ - "billing_email": "org@example.com", - "members_can_create_internal_repositories": true, - }, - want: createBaseline(), - }, - } { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - d := testResourceDataWithConfig(t, tt.raw) - - got := organizationSettingsForCreate(d, tt.isEnterprise) - - if diff := cmp.Diff(tt.want, got); diff != "" { - t.Errorf("organizationSettingsForCreate() mismatch (-want +got):\n%s", diff) - } - }) - } -} - -func Test_organizationSettingsForUpdate(t *testing.T) { - t.Parallel() - - for _, tt := range []struct { - name string - raw map[string]any - isEnterprise bool - want *github.Organization - }{ - { - // Only changed attributes are sent, which is what keeps - // unconfigured attributes out of the request (#2305). The - // attributes that do survive here are an artifact of - // TestResourceDataRaw: one left out of raw reads as the zero value - // from state but as its schema default from d.Get, so every - // attribute defaulting to a non-zero value looks changed. - name: "update_omits_unchanged_booleans", - raw: map[string]any{ - "billing_email": "org@example.com", - "has_organization_projects": false, - }, - want: func() *github.Organization { - want := updateBaseline() - want.HasOrganizationProjects = nil - return want - }(), - }, - { - // A configured optional string is a change against empty state, so - // it is included; the create path reaches the same result through - // GetOk instead. - name: "update_includes_changed_optional_string", - raw: map[string]any{ - "billing_email": "org@example.com", - "description": "example description", - }, - want: func() *github.Organization { - want := updateBaseline() - want.Description = new("example description") - return want - }(), - }, - } { - t.Run(tt.name, func(t *testing.T) { - t.Parallel() - - d := schema.TestResourceDataRaw(t, resourceGithubOrganizationSettings().Schema, tt.raw) - d.SetId("example-org") - - got := organizationSettingsForUpdate(d, tt.isEnterprise) - - if diff := cmp.Diff(tt.want, got); diff != "" { - t.Errorf("organizationSettingsForUpdate() mismatch (-want +got):\n%s", diff) - } - }) - } -} From a137caad568817c07ad97fe10138c469a0318473 Mon Sep 17 00:00:00 2001 From: somaz Date: Fri, 11 Sep 2026 11:25:35 +0900 Subject: [PATCH 09/11] refactor: inline organization_settings create and update logic into the CRUD functions --- .../resource_github_organization_settings.go | 544 +++++++----------- github/util.go | 27 + 2 files changed, 240 insertions(+), 331 deletions(-) diff --git a/github/resource_github_organization_settings.go b/github/resource_github_organization_settings.go index 42036b5cf3..9e86a5724b 100644 --- a/github/resource_github_organization_settings.go +++ b/github/resource_github_organization_settings.go @@ -3,11 +3,10 @@ package github import ( "context" "errors" - "log" "strconv" "github.com/google/go-github/v92/github" - "github.com/hashicorp/go-cty/cty" + "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/hashicorp/terraform-plugin-sdk/v2/diag" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" @@ -175,447 +174,329 @@ func resourceGithubOrganizationSettings() *schema.Resource { } } -// 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 -} - -// 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 +func resourceGithubOrganizationSettingsCreate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { + if err := checkOrganization(meta); err != nil { + return diag.FromErr(err) } - v, diags := d.GetRawConfigAt(cty.GetAttrPath(name)) - if diags.HasError() { - _, ok := d.GetOk(name) - - return ok + owner, ok := meta.(*Owner) + if !ok { + return diag.Errorf("unexpected provider meta type %T", meta) } - return !v.IsNull() -} + ctx = context.WithValue(ctx, ctxId, d.Id()) -// sendOnCreate reports whether the named attribute belongs in the create -// payload: when the user explicitly configured it, or when d.GetOk reports it -// because its schema default is a non-zero value. -// -// The second half keeps the create payload identical to what #2807 sent. -// Attributes with a non-zero default (has_organization_projects, -// default_repository_permission, ...) have always been sent on create, and -// leaving them out would let the org keep a value that differs from the -// schema default, so the first plan after create is not clean and only the -// second apply converges. Attributes defaulting to false or "" are still -// only sent when explicitly configured, which is what keeps the request -// narrow enough to avoid the validation errors reported in #2305. -func sendOnCreate(d *schema.ResourceData, name string) bool { - if isConfigured(d, name) { - return true - } - - _, ok := d.GetOk(name) - - return ok -} + orgInfo, _, err := owner.v3client.Organizations.Get(ctx, owner.name) + if err != nil { + return diag.FromErr(err) + } + isEnterprise := orgInfo.GetPlan().GetName() == "enterprise" -// organizationSettingsForCreate builds the payload sent when the resource is -// created. An attribute is included when the user explicitly set it or when -// its schema default is a non-zero value; everything else stays out of the -// request and the org keeps whatever the API defaults to. Keeping the payload -// narrow is what avoids the validation errors reported in #2305. -// -// Explicit configuration is read from the raw configuration rather than -// d.GetOk, which cannot tell an explicitly configured false from an unset -// attribute and so used to drop a boolean configured as false from the -// create payload (#3493). -func organizationSettingsForCreate(d *schema.ResourceData, isEnterprise bool) *github.Organization { + // An attribute is sent on create when the user configured it, including a + // boolean configured as false (#3493), or when its schema default is a + // non-zero value. Everything else stays out of the request, which keeps it + // narrow enough to avoid the validation errors reported in #2305. settings := &github.Organization{} - // The API rejects the request without billing_email, so it is always sent. - if sendOnCreate(d, "billing_email") { - settings.BillingEmail = new(stringAttr(d, "billing_email")) + if _, ok := d.GetOk("billing_email"); ok || isConfigured(d, "billing_email") { + v, _ := d.Get("billing_email").(string) + settings.BillingEmail = new(v) } - - if sendOnCreate(d, "company") { - settings.Company = new(stringAttr(d, "company")) + if _, ok := d.GetOk("company"); ok || isConfigured(d, "company") { + v, _ := d.Get("company").(string) + settings.Company = new(v) } - if sendOnCreate(d, "email") { - settings.Email = new(stringAttr(d, "email")) + if _, ok := d.GetOk("email"); ok || isConfigured(d, "email") { + v, _ := d.Get("email").(string) + settings.Email = new(v) } - if sendOnCreate(d, "twitter_username") { - settings.TwitterUsername = new(stringAttr(d, "twitter_username")) + if _, ok := d.GetOk("twitter_username"); ok || isConfigured(d, "twitter_username") { + v, _ := d.Get("twitter_username").(string) + settings.TwitterUsername = new(v) } - if sendOnCreate(d, "location") { - settings.Location = new(stringAttr(d, "location")) + if _, ok := d.GetOk("location"); ok || isConfigured(d, "location") { + v, _ := d.Get("location").(string) + settings.Location = new(v) } - if sendOnCreate(d, "name") { - settings.Name = new(stringAttr(d, "name")) + if _, ok := d.GetOk("name"); ok || isConfigured(d, "name") { + v, _ := d.Get("name").(string) + settings.Name = new(v) } - if sendOnCreate(d, "description") { - settings.Description = new(stringAttr(d, "description")) + if _, ok := d.GetOk("description"); ok || isConfigured(d, "description") { + v, _ := d.Get("description").(string) + settings.Description = new(v) } - if sendOnCreate(d, "blog") { - settings.Blog = new(stringAttr(d, "blog")) + if _, ok := d.GetOk("blog"); ok || isConfigured(d, "blog") { + v, _ := d.Get("blog").(string) + settings.Blog = new(v) } - - if sendOnCreate(d, "has_organization_projects") { - settings.HasOrganizationProjects = new(boolAttr(d, "has_organization_projects")) + if _, ok := d.GetOk("has_organization_projects"); ok || isConfigured(d, "has_organization_projects") { + v, _ := d.Get("has_organization_projects").(bool) + settings.HasOrganizationProjects = new(v) } - if sendOnCreate(d, "has_repository_projects") { - settings.HasRepositoryProjects = new(boolAttr(d, "has_repository_projects")) + if _, ok := d.GetOk("has_repository_projects"); ok || isConfigured(d, "has_repository_projects") { + v, _ := d.Get("has_repository_projects").(bool) + settings.HasRepositoryProjects = new(v) } - if sendOnCreate(d, "default_repository_permission") { - settings.DefaultRepoPermission = new(stringAttr(d, "default_repository_permission")) + if _, ok := d.GetOk("default_repository_permission"); ok || isConfigured(d, "default_repository_permission") { + v, _ := d.Get("default_repository_permission").(string) + settings.DefaultRepoPermission = new(v) } - if sendOnCreate(d, "members_can_create_repositories") { - settings.MembersCanCreateRepos = new(boolAttr(d, "members_can_create_repositories")) + if _, ok := d.GetOk("members_can_create_repositories"); ok || isConfigured(d, "members_can_create_repositories") { + v, _ := d.Get("members_can_create_repositories").(bool) + settings.MembersCanCreateRepos = new(v) } - if sendOnCreate(d, "members_can_create_private_repositories") { - settings.MembersCanCreatePrivateRepos = new(boolAttr(d, "members_can_create_private_repositories")) + if _, ok := d.GetOk("members_can_create_private_repositories"); ok || isConfigured(d, "members_can_create_private_repositories") { + v, _ := d.Get("members_can_create_private_repositories").(bool) + settings.MembersCanCreatePrivateRepos = new(v) } - if sendOnCreate(d, "members_can_create_public_repositories") { - settings.MembersCanCreatePublicRepos = new(boolAttr(d, "members_can_create_public_repositories")) + if _, ok := d.GetOk("members_can_create_public_repositories"); ok || isConfigured(d, "members_can_create_public_repositories") { + v, _ := d.Get("members_can_create_public_repositories").(bool) + settings.MembersCanCreatePublicRepos = new(v) } - if sendOnCreate(d, "members_can_create_pages") { - settings.MembersCanCreatePages = new(boolAttr(d, "members_can_create_pages")) + if _, ok := d.GetOk("members_can_create_pages"); ok || isConfigured(d, "members_can_create_pages") { + v, _ := d.Get("members_can_create_pages").(bool) + settings.MembersCanCreatePages = new(v) } - if sendOnCreate(d, "members_can_create_public_pages") { - settings.MembersCanCreatePublicPages = new(boolAttr(d, "members_can_create_public_pages")) + if _, ok := d.GetOk("members_can_create_public_pages"); ok || isConfigured(d, "members_can_create_public_pages") { + v, _ := d.Get("members_can_create_public_pages").(bool) + settings.MembersCanCreatePublicPages = new(v) } - if sendOnCreate(d, "members_can_create_private_pages") { - settings.MembersCanCreatePrivatePages = new(boolAttr(d, "members_can_create_private_pages")) + if _, ok := d.GetOk("members_can_create_private_pages"); ok || isConfigured(d, "members_can_create_private_pages") { + v, _ := d.Get("members_can_create_private_pages").(bool) + settings.MembersCanCreatePrivatePages = new(v) } - if sendOnCreate(d, "members_can_fork_private_repositories") { - settings.MembersCanForkPrivateRepos = new(boolAttr(d, "members_can_fork_private_repositories")) + if _, ok := d.GetOk("members_can_fork_private_repositories"); ok || isConfigured(d, "members_can_fork_private_repositories") { + v, _ := d.Get("members_can_fork_private_repositories").(bool) + settings.MembersCanForkPrivateRepos = new(v) } - if sendOnCreate(d, "web_commit_signoff_required") { - settings.WebCommitSignoffRequired = new(boolAttr(d, "web_commit_signoff_required")) + if _, ok := d.GetOk("web_commit_signoff_required"); ok || isConfigured(d, "web_commit_signoff_required") { + v, _ := d.Get("web_commit_signoff_required").(bool) + settings.WebCommitSignoffRequired = new(v) } - if sendOnCreate(d, "advanced_security_enabled_for_new_repositories") { - settings.AdvancedSecurityEnabledForNewRepos = new(boolAttr(d, "advanced_security_enabled_for_new_repositories")) + if _, ok := d.GetOk("advanced_security_enabled_for_new_repositories"); ok || isConfigured(d, "advanced_security_enabled_for_new_repositories") { + v, _ := d.Get("advanced_security_enabled_for_new_repositories").(bool) + settings.AdvancedSecurityEnabledForNewRepos = new(v) } - if sendOnCreate(d, "dependabot_alerts_enabled_for_new_repositories") { - settings.DependabotAlertsEnabledForNewRepos = new(boolAttr(d, "dependabot_alerts_enabled_for_new_repositories")) + if _, ok := d.GetOk("dependabot_alerts_enabled_for_new_repositories"); ok || isConfigured(d, "dependabot_alerts_enabled_for_new_repositories") { + v, _ := d.Get("dependabot_alerts_enabled_for_new_repositories").(bool) + settings.DependabotAlertsEnabledForNewRepos = new(v) } - if sendOnCreate(d, "dependabot_security_updates_enabled_for_new_repositories") { - settings.DependabotSecurityUpdatesEnabledForNewRepos = new(boolAttr(d, "dependabot_security_updates_enabled_for_new_repositories")) + if _, ok := d.GetOk("dependabot_security_updates_enabled_for_new_repositories"); ok || isConfigured(d, "dependabot_security_updates_enabled_for_new_repositories") { + v, _ := d.Get("dependabot_security_updates_enabled_for_new_repositories").(bool) + settings.DependabotSecurityUpdatesEnabledForNewRepos = new(v) } - if sendOnCreate(d, "dependency_graph_enabled_for_new_repositories") { - settings.DependencyGraphEnabledForNewRepos = new(boolAttr(d, "dependency_graph_enabled_for_new_repositories")) + if _, ok := d.GetOk("dependency_graph_enabled_for_new_repositories"); ok || isConfigured(d, "dependency_graph_enabled_for_new_repositories") { + v, _ := d.Get("dependency_graph_enabled_for_new_repositories").(bool) + settings.DependencyGraphEnabledForNewRepos = new(v) } - if sendOnCreate(d, "secret_scanning_enabled_for_new_repositories") { - settings.SecretScanningEnabledForNewRepos = new(boolAttr(d, "secret_scanning_enabled_for_new_repositories")) + if _, ok := d.GetOk("secret_scanning_enabled_for_new_repositories"); ok || isConfigured(d, "secret_scanning_enabled_for_new_repositories") { + v, _ := d.Get("secret_scanning_enabled_for_new_repositories").(bool) + settings.SecretScanningEnabledForNewRepos = new(v) } - if sendOnCreate(d, "secret_scanning_push_protection_enabled_for_new_repositories") { - settings.SecretScanningPushProtectionEnabledForNewRepos = new(boolAttr(d, "secret_scanning_push_protection_enabled_for_new_repositories")) + if _, ok := d.GetOk("secret_scanning_push_protection_enabled_for_new_repositories"); ok || isConfigured(d, "secret_scanning_push_protection_enabled_for_new_repositories") { + v, _ := d.Get("secret_scanning_push_protection_enabled_for_new_repositories").(bool) + settings.SecretScanningPushProtectionEnabledForNewRepos = new(v) } if isEnterprise { - if sendOnCreate(d, "members_can_create_internal_repositories") { - settings.MembersCanCreateInternalRepos = new(boolAttr(d, "members_can_create_internal_repositories")) + if _, ok := d.GetOk("members_can_create_internal_repositories"); ok || isConfigured(d, "members_can_create_internal_repositories") { + v, _ := d.Get("members_can_create_internal_repositories").(bool) + settings.MembersCanCreateInternalRepos = new(v) } } - return settings + tflog.Debug(ctx, "Creating organization settings", map[string]any{ + "org": owner.name, + "enterprise": isEnterprise, + "settings": settings.String(), + }) + + orgSettings, _, err := owner.v3client.Organizations.Edit(ctx, owner.name, settings) + if err != nil { + if ghErr, ok := errors.AsType[*github.ErrorResponse](err); ok { + tflog.Debug(ctx, "GitHub API error", map[string]any{ + "status": ghErr.Response.StatusCode, + "message": ghErr.Message, + "errors": ghErr.Errors, + }) + } + + return diag.FromErr(err) + } + + d.SetId(strconv.FormatInt(orgSettings.GetID(), 10)) + + return nil } -// organizationSettingsForUpdate builds the payload sent when the resource is -// updated. Only attributes that actually changed are included, which keeps -// unconfigured attributes out of the request and avoids the API validation -// errors reported in #2305. -func organizationSettingsForUpdate(d *schema.ResourceData, isEnterprise bool) *github.Organization { +func resourceGithubOrganizationSettingsUpdate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { + if err := checkOrganization(meta); err != nil { + return diag.FromErr(err) + } + + owner, ok := meta.(*Owner) + if !ok { + return diag.Errorf("unexpected provider meta type %T", meta) + } + + ctx = context.WithValue(ctx, ctxId, d.Id()) + + orgInfo, _, err := owner.v3client.Organizations.Get(ctx, owner.name) + if err != nil { + return diag.FromErr(err) + } + isEnterprise := orgInfo.GetPlan().GetName() == "enterprise" + + // Only attributes that changed are sent, which keeps unconfigured attributes + // out of the request and avoids the validation errors reported in #2305. settings := &github.Organization{} // The API rejects the request without billing_email, so it is always sent. - if _, ok := d.GetOk("billing_email"); ok { - settings.BillingEmail = new(stringAttr(d, "billing_email")) + if v, ok := d.GetOk("billing_email"); ok { + billingEmail, _ := v.(string) + settings.BillingEmail = new(billingEmail) } if d.HasChange("company") { - if _, ok := d.GetOk("company"); ok { - settings.Company = new(stringAttr(d, "company")) + if v, ok := d.GetOk("company"); ok { + s, _ := v.(string) + settings.Company = new(s) } } if d.HasChange("email") { - if _, ok := d.GetOk("email"); ok { - settings.Email = new(stringAttr(d, "email")) + if v, ok := d.GetOk("email"); ok { + s, _ := v.(string) + settings.Email = new(s) } } if d.HasChange("twitter_username") { - if _, ok := d.GetOk("twitter_username"); ok { - settings.TwitterUsername = new(stringAttr(d, "twitter_username")) + if v, ok := d.GetOk("twitter_username"); ok { + s, _ := v.(string) + settings.TwitterUsername = new(s) } } if d.HasChange("location") { - if _, ok := d.GetOk("location"); ok { - settings.Location = new(stringAttr(d, "location")) + if v, ok := d.GetOk("location"); ok { + s, _ := v.(string) + settings.Location = new(s) } } if d.HasChange("name") { - if _, ok := d.GetOk("name"); ok { - settings.Name = new(stringAttr(d, "name")) + if v, ok := d.GetOk("name"); ok { + s, _ := v.(string) + settings.Name = new(s) } } if d.HasChange("description") { - if _, ok := d.GetOk("description"); ok { - settings.Description = new(stringAttr(d, "description")) + if v, ok := d.GetOk("description"); ok { + s, _ := v.(string) + settings.Description = new(s) } } if d.HasChange("blog") { - if _, ok := d.GetOk("blog"); ok { - settings.Blog = new(stringAttr(d, "blog")) + if v, ok := d.GetOk("blog"); ok { + s, _ := v.(string) + settings.Blog = new(s) } } - if d.HasChange("has_organization_projects") { - settings.HasOrganizationProjects = new(boolAttr(d, "has_organization_projects")) + v, _ := d.Get("has_organization_projects").(bool) + settings.HasOrganizationProjects = new(v) } if d.HasChange("has_repository_projects") { - settings.HasRepositoryProjects = new(boolAttr(d, "has_repository_projects")) + v, _ := d.Get("has_repository_projects").(bool) + settings.HasRepositoryProjects = new(v) } if d.HasChange("default_repository_permission") { - if _, ok := d.GetOk("default_repository_permission"); ok { - settings.DefaultRepoPermission = new(stringAttr(d, "default_repository_permission")) + if v, ok := d.GetOk("default_repository_permission"); ok { + s, _ := v.(string) + settings.DefaultRepoPermission = new(s) } } if d.HasChange("members_can_create_repositories") { - settings.MembersCanCreateRepos = new(boolAttr(d, "members_can_create_repositories")) + v, _ := d.Get("members_can_create_repositories").(bool) + settings.MembersCanCreateRepos = new(v) } if d.HasChange("members_can_create_private_repositories") { - settings.MembersCanCreatePrivateRepos = new(boolAttr(d, "members_can_create_private_repositories")) + v, _ := d.Get("members_can_create_private_repositories").(bool) + settings.MembersCanCreatePrivateRepos = new(v) } if d.HasChange("members_can_create_public_repositories") { - settings.MembersCanCreatePublicRepos = new(boolAttr(d, "members_can_create_public_repositories")) + v, _ := d.Get("members_can_create_public_repositories").(bool) + settings.MembersCanCreatePublicRepos = new(v) } if d.HasChange("members_can_create_pages") { - settings.MembersCanCreatePages = new(boolAttr(d, "members_can_create_pages")) + v, _ := d.Get("members_can_create_pages").(bool) + settings.MembersCanCreatePages = new(v) } if d.HasChange("members_can_create_public_pages") { - settings.MembersCanCreatePublicPages = new(boolAttr(d, "members_can_create_public_pages")) + v, _ := d.Get("members_can_create_public_pages").(bool) + settings.MembersCanCreatePublicPages = new(v) } if d.HasChange("members_can_create_private_pages") { - settings.MembersCanCreatePrivatePages = new(boolAttr(d, "members_can_create_private_pages")) + v, _ := d.Get("members_can_create_private_pages").(bool) + settings.MembersCanCreatePrivatePages = new(v) } if d.HasChange("members_can_fork_private_repositories") { - settings.MembersCanForkPrivateRepos = new(boolAttr(d, "members_can_fork_private_repositories")) + v, _ := d.Get("members_can_fork_private_repositories").(bool) + settings.MembersCanForkPrivateRepos = new(v) } if d.HasChange("web_commit_signoff_required") { - settings.WebCommitSignoffRequired = new(boolAttr(d, "web_commit_signoff_required")) + v, _ := d.Get("web_commit_signoff_required").(bool) + settings.WebCommitSignoffRequired = new(v) } if d.HasChange("advanced_security_enabled_for_new_repositories") { - settings.AdvancedSecurityEnabledForNewRepos = new(boolAttr(d, "advanced_security_enabled_for_new_repositories")) + v, _ := d.Get("advanced_security_enabled_for_new_repositories").(bool) + settings.AdvancedSecurityEnabledForNewRepos = new(v) } if d.HasChange("dependabot_alerts_enabled_for_new_repositories") { - settings.DependabotAlertsEnabledForNewRepos = new(boolAttr(d, "dependabot_alerts_enabled_for_new_repositories")) + v, _ := d.Get("dependabot_alerts_enabled_for_new_repositories").(bool) + settings.DependabotAlertsEnabledForNewRepos = new(v) } if d.HasChange("dependabot_security_updates_enabled_for_new_repositories") { - settings.DependabotSecurityUpdatesEnabledForNewRepos = new(boolAttr(d, "dependabot_security_updates_enabled_for_new_repositories")) + v, _ := d.Get("dependabot_security_updates_enabled_for_new_repositories").(bool) + settings.DependabotSecurityUpdatesEnabledForNewRepos = new(v) } if d.HasChange("dependency_graph_enabled_for_new_repositories") { - settings.DependencyGraphEnabledForNewRepos = new(boolAttr(d, "dependency_graph_enabled_for_new_repositories")) + v, _ := d.Get("dependency_graph_enabled_for_new_repositories").(bool) + settings.DependencyGraphEnabledForNewRepos = new(v) } if d.HasChange("secret_scanning_enabled_for_new_repositories") { - settings.SecretScanningEnabledForNewRepos = new(boolAttr(d, "secret_scanning_enabled_for_new_repositories")) + v, _ := d.Get("secret_scanning_enabled_for_new_repositories").(bool) + settings.SecretScanningEnabledForNewRepos = new(v) } if d.HasChange("secret_scanning_push_protection_enabled_for_new_repositories") { - settings.SecretScanningPushProtectionEnabledForNewRepos = new(boolAttr(d, "secret_scanning_push_protection_enabled_for_new_repositories")) + v, _ := d.Get("secret_scanning_push_protection_enabled_for_new_repositories").(bool) + settings.SecretScanningPushProtectionEnabledForNewRepos = new(v) } if isEnterprise { if d.HasChange("members_can_create_internal_repositories") { - settings.MembersCanCreateInternalRepos = new(boolAttr(d, "members_can_create_internal_repositories")) + v, _ := d.Get("members_can_create_internal_repositories").(bool) + settings.MembersCanCreateInternalRepos = new(v) } } - return settings -} + tflog.Debug(ctx, "Updating organization settings", map[string]any{ + "org": owner.name, + "enterprise": isEnterprise, + "settings": settings.String(), + }) -// 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) - log.Printf("[DEBUG] Built settings for org %s (enterprise: %v)", org, isEnterprise) - if settings.BillingEmail != nil { - log.Printf("[DEBUG] BillingEmail: %s", *settings.BillingEmail) - } - if settings.Company != nil { - log.Printf("[DEBUG] Company: %s", *settings.Company) - } - if settings.Email != nil { - log.Printf("[DEBUG] Email: %s", *settings.Email) - } - if settings.TwitterUsername != nil { - log.Printf("[DEBUG] TwitterUsername: %s", *settings.TwitterUsername) - } - if settings.Location != nil { - log.Printf("[DEBUG] Location: %s", *settings.Location) - } - if settings.Name != nil { - log.Printf("[DEBUG] Name: %s", *settings.Name) - } - if settings.Description != nil { - log.Printf("[DEBUG] Description: %s", *settings.Description) - } - if settings.Blog != nil { - log.Printf("[DEBUG] Blog: %s", *settings.Blog) - } - if settings.HasOrganizationProjects != nil { - log.Printf("[DEBUG] HasOrganizationProjects: %v", *settings.HasOrganizationProjects) - } - if settings.HasRepositoryProjects != nil { - log.Printf("[DEBUG] HasRepositoryProjects: %v", *settings.HasRepositoryProjects) - } - if settings.DefaultRepoPermission != nil { - log.Printf("[DEBUG] DefaultRepoPermission: %s", *settings.DefaultRepoPermission) - } - if settings.MembersCanCreateRepos != nil { - log.Printf("[DEBUG] MembersCanCreateRepos: %v", *settings.MembersCanCreateRepos) - } - if settings.MembersCanCreatePrivateRepos != nil { - log.Printf("[DEBUG] MembersCanCreatePrivateRepos: %v", *settings.MembersCanCreatePrivateRepos) - } - if settings.MembersCanCreatePublicRepos != nil { - log.Printf("[DEBUG] MembersCanCreatePublicRepos: %v", *settings.MembersCanCreatePublicRepos) - } - if settings.MembersCanCreateInternalRepos != nil { - log.Printf("[DEBUG] MembersCanCreateInternalRepos: %v", *settings.MembersCanCreateInternalRepos) - } - if settings.MembersCanCreatePages != nil { - log.Printf("[DEBUG] MembersCanCreatePages: %v", *settings.MembersCanCreatePages) - } - if settings.MembersCanCreatePublicPages != nil { - log.Printf("[DEBUG] MembersCanCreatePublicPages: %v", *settings.MembersCanCreatePublicPages) - } - if settings.MembersCanCreatePrivatePages != nil { - log.Printf("[DEBUG] MembersCanCreatePrivatePages: %v", *settings.MembersCanCreatePrivatePages) - } - if settings.MembersCanForkPrivateRepos != nil { - log.Printf("[DEBUG] MembersCanForkPrivateRepos: %v", *settings.MembersCanForkPrivateRepos) - } - if settings.WebCommitSignoffRequired != nil { - log.Printf("[DEBUG] WebCommitSignoffRequired: %v", *settings.WebCommitSignoffRequired) - } - if settings.AdvancedSecurityEnabledForNewRepos != nil { - log.Printf("[DEBUG] AdvancedSecurityEnabledForNewRepos: %v", *settings.AdvancedSecurityEnabledForNewRepos) - } - if settings.DependabotAlertsEnabledForNewRepos != nil { - log.Printf("[DEBUG] DependabotAlertsEnabledForNewRepos: %v", *settings.DependabotAlertsEnabledForNewRepos) - } - if settings.DependabotSecurityUpdatesEnabledForNewRepos != nil { - log.Printf("[DEBUG] DependabotSecurityUpdatesEnabledForNewRepos: %v", *settings.DependabotSecurityUpdatesEnabledForNewRepos) - } - if settings.DependencyGraphEnabledForNewRepos != nil { - log.Printf("[DEBUG] DependencyGraphEnabledForNewRepos: %v", *settings.DependencyGraphEnabledForNewRepos) - } - if settings.SecretScanningEnabledForNewRepos != nil { - log.Printf("[DEBUG] SecretScanningEnabledForNewRepos: %v", *settings.SecretScanningEnabledForNewRepos) - } - if settings.SecretScanningPushProtectionEnabledForNewRepos != nil { - log.Printf("[DEBUG] SecretScanningPushProtectionEnabledForNewRepos: %v", *settings.SecretScanningPushProtectionEnabledForNewRepos) - } -} - -// organizationPlanIsEnterprise reports whether the organization is on an -// enterprise plan, which gates the enterprise-only attributes. -func organizationPlanIsEnterprise(ctx context.Context, owner *Owner) (bool, error) { - orgInfo, _, err := owner.v3client.Organizations.Get(ctx, owner.name) - if err != nil { - return false, err - } - - return orgInfo.GetPlan().GetName() == "enterprise", nil -} - -// editOrganizationSettings sends the payload and refreshes state from the API. -func editOrganizationSettings(ctx context.Context, d *schema.ResourceData, owner *Owner, settings *github.Organization) diag.Diagnostics { - orgSettings, _, err := owner.v3client.Organizations.Edit(ctx, owner.name, settings) - if err != nil { - // Log detailed error information for debugging + if _, _, err := owner.v3client.Organizations.Edit(ctx, owner.name, settings); err != nil { if ghErr, ok := errors.AsType[*github.ErrorResponse](err); ok { - log.Printf("[DEBUG] GitHub API Error: Status=%d, Message=%s", ghErr.Response.StatusCode, ghErr.Message) - if len(ghErr.Errors) > 0 { - for i, apiErr := range ghErr.Errors { - log.Printf("[DEBUG] Error[%d]: Resource=%s, Field=%s, Code=%s, Message=%s", - i, apiErr.Resource, apiErr.Field, apiErr.Code, apiErr.Message) - } - } + tflog.Debug(ctx, "GitHub API error", map[string]any{ + "status": ghErr.Response.StatusCode, + "message": ghErr.Message, + "errors": ghErr.Errors, + }) } return diag.FromErr(err) } - d.SetId(strconv.FormatInt(orgSettings.GetID(), 10)) - - return resourceGithubOrganizationSettingsRead(ctx, d, owner) -} - -func resourceGithubOrganizationSettingsCreate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { - if err := checkOrganization(meta); err != nil { - return diag.FromErr(err) - } - - owner, ok := meta.(*Owner) - if !ok { - return diag.Errorf("unexpected provider meta type %T", meta) - } - - ctx = context.WithValue(ctx, ctxId, d.Id()) - - isEnterprise, err := organizationPlanIsEnterprise(ctx, owner) - if err != nil { - return diag.FromErr(err) - } - - settings := organizationSettingsForCreate(d, isEnterprise) - logOrganizationSettings(owner.name, isEnterprise, settings) - - return editOrganizationSettings(ctx, d, owner, settings) -} - -func resourceGithubOrganizationSettingsUpdate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { - if err := checkOrganization(meta); err != nil { - return diag.FromErr(err) - } - - owner, ok := meta.(*Owner) - if !ok { - return diag.Errorf("unexpected provider meta type %T", meta) - } - - ctx = context.WithValue(ctx, ctxId, d.Id()) - - isEnterprise, err := organizationPlanIsEnterprise(ctx, owner) - if err != nil { - return diag.FromErr(err) - } - - settings := organizationSettingsForUpdate(d, isEnterprise) - logOrganizationSettings(owner.name, isEnterprise, settings) - - return editOrganizationSettings(ctx, d, owner, settings) + return nil } func resourceGithubOrganizationSettingsRead(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { @@ -726,12 +607,13 @@ func resourceGithubOrganizationSettingsDelete(ctx context.Context, d *schema.Res ctx = context.WithValue(ctx, ctxId, d.Id()) - log.Printf("[DEBUG] Reverting Organization Settings to default values: %s", owner.name) + tflog.Debug(ctx, "Reverting organization settings to default values", map[string]any{"org": owner.name}) - isEnterprise, err := organizationPlanIsEnterprise(ctx, owner) + orgInfo, _, err := owner.v3client.Organizations.Get(ctx, owner.name) if err != nil { return diag.FromErr(err) } + isEnterprise := orgInfo.GetPlan().GetName() == "enterprise" // Build minimal settings with only required fields defaultSettings := &github.Organization{ diff --git a/github/util.go b/github/util.go index ea737b7a5c..e8ae5f41a8 100644 --- a/github/util.go +++ b/github/util.go @@ -263,3 +263,30 @@ func toInt64(v any) int64 { return 0 } } + +// 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. 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 +// d.GetOk 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() +} From afb42fabd07df66f1f6f0eb86f133b4d8d98f302 Mon Sep 17 00:00:00 2001 From: somaz Date: Mon, 14 Sep 2026 10:37:59 +0900 Subject: [PATCH 10/11] test: snapshot and restore organization settings so the acceptance test can run --- github/acc_helpers_test.go | 52 +++++++++++++++++++ ...ource_github_organization_settings_test.go | 7 ++- 2 files changed, 58 insertions(+), 1 deletion(-) diff --git a/github/acc_helpers_test.go b/github/acc_helpers_test.go index f7f88b735e..bd3689264c 100644 --- a/github/acc_helpers_test.go +++ b/github/acc_helpers_test.go @@ -80,6 +80,58 @@ func mustAssignOrganizationRoleToUser(t *testing.T, username string, roleID int6 }) } +func mustSnapshotOrganizationSettings(t *testing.T) { + t.Helper() + + org, _, err := testAccConf.meta.v3client.Organizations.Get(t.Context(), testAccConf.meta.name) + if err != nil { + t.Fatalf("failed to read test organization settings: %v", err) + } + + // Organizations.Edit patches the organization and the Get response carries + // read-only fields the API rejects, so capture only the attributes that + // github_organization_settings manages. + settings := &github.Organization{ + BillingEmail: org.BillingEmail, + Company: org.Company, + Email: org.Email, + TwitterUsername: org.TwitterUsername, + Location: org.Location, + Name: org.Name, + Description: org.Description, + Blog: org.Blog, + HasOrganizationProjects: org.HasOrganizationProjects, + HasRepositoryProjects: org.HasRepositoryProjects, + DefaultRepoPermission: org.DefaultRepoPermission, + MembersCanCreateRepos: org.MembersCanCreateRepos, + MembersCanCreatePrivateRepos: org.MembersCanCreatePrivateRepos, + MembersCanCreatePublicRepos: org.MembersCanCreatePublicRepos, + MembersCanCreatePages: org.MembersCanCreatePages, + MembersCanCreatePublicPages: org.MembersCanCreatePublicPages, + MembersCanCreatePrivatePages: org.MembersCanCreatePrivatePages, + MembersCanForkPrivateRepos: org.MembersCanForkPrivateRepos, + WebCommitSignoffRequired: org.WebCommitSignoffRequired, + AdvancedSecurityEnabledForNewRepos: org.AdvancedSecurityEnabledForNewRepos, + DependabotAlertsEnabledForNewRepos: org.DependabotAlertsEnabledForNewRepos, + DependabotSecurityUpdatesEnabledForNewRepos: org.DependabotSecurityUpdatesEnabledForNewRepos, + DependencyGraphEnabledForNewRepos: org.DependencyGraphEnabledForNewRepos, + SecretScanningEnabledForNewRepos: org.SecretScanningEnabledForNewRepos, + SecretScanningPushProtectionEnabledForNewRepos: org.SecretScanningPushProtectionEnabledForNewRepos, + } + + // members_can_create_internal_repositories is only accepted on enterprise + // plans, matching the resource's own delete path. + if org.GetPlan().GetName() == "enterprise" { + settings.MembersCanCreateInternalRepos = org.MembersCanCreateInternalRepos + } + + t.Cleanup(func() { + if _, _, err := testAccConf.meta.v3client.Organizations.Edit(context.Background(), testAccConf.meta.name, settings); err != nil { + t.Logf("failed to restore test organization settings: %v", err) + } + }) +} + func mustCreateTestOrganizationRepositoryRole(t *testing.T) *github.CustomRepoRoles { t.Helper() diff --git a/github/resource_github_organization_settings_test.go b/github/resource_github_organization_settings_test.go index 21b55843c5..06e3a4d72f 100644 --- a/github/resource_github_organization_settings_test.go +++ b/github/resource_github_organization_settings_test.go @@ -10,7 +10,12 @@ import ( func TestAccGithubOrganizationSettings(t *testing.T) { // IMPORTANT: Do not run these tests in parallel as they modify the organization state. - t.Skip("TODO: Make this test cleanup correctly") + skipUnlessHasOrgs(t) + + // The resource's delete reverts to hardcoded defaults rather than the + // organization's prior state, so snapshot the settings and put them back + // once every subtest has run. + mustSnapshotOrganizationSettings(t) t.Run("creates organization settings without error", func(t *testing.T) { config := ` From aadcd7ac966066d3d9da5c3ca531ce5b35cafd94 Mon Sep 17 00:00:00 2001 From: somaz Date: Wed, 16 Sep 2026 12:23:59 +0900 Subject: [PATCH 11/11] refactor: align organization_settings with the current CRUD conventions --- .../resource_github_organization_settings.go | 261 ++++++++---------- 1 file changed, 112 insertions(+), 149 deletions(-) diff --git a/github/resource_github_organization_settings.go b/github/resource_github_organization_settings.go index 9e86a5724b..045804840a 100644 --- a/github/resource_github_organization_settings.go +++ b/github/resource_github_organization_settings.go @@ -2,7 +2,6 @@ package github import ( "context" - "errors" "strconv" "github.com/google/go-github/v92/github" @@ -174,19 +173,16 @@ func resourceGithubOrganizationSettings() *schema.Resource { } } -func resourceGithubOrganizationSettingsCreate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { - if err := checkOrganization(meta); err != nil { - return diag.FromErr(err) - } +func resourceGithubOrganizationSettingsCreate(ctx context.Context, d *schema.ResourceData, m any) diag.Diagnostics { + meta, _ := m.(*Owner) - owner, ok := meta.(*Owner) - if !ok { - return diag.Errorf("unexpected provider meta type %T", meta) + if ok, diags := checkOrganizationOK(meta); !ok { + return diags } ctx = context.WithValue(ctx, ctxId, d.Id()) - orgInfo, _, err := owner.v3client.Organizations.Get(ctx, owner.name) + orgInfo, _, err := meta.v3client.Organizations.Get(ctx, meta.name) if err != nil { return diag.FromErr(err) } @@ -306,22 +302,10 @@ func resourceGithubOrganizationSettingsCreate(ctx context.Context, d *schema.Res } } - tflog.Debug(ctx, "Creating organization settings", map[string]any{ - "org": owner.name, - "enterprise": isEnterprise, - "settings": settings.String(), - }) + tflog.Debug(ctx, "Creating organization settings", map[string]any{"org": meta.name, "enterprise": isEnterprise, "settings": settings.String()}) - orgSettings, _, err := owner.v3client.Organizations.Edit(ctx, owner.name, settings) + orgSettings, _, err := meta.v3client.Organizations.Edit(ctx, meta.name, settings) if err != nil { - if ghErr, ok := errors.AsType[*github.ErrorResponse](err); ok { - tflog.Debug(ctx, "GitHub API error", map[string]any{ - "status": ghErr.Response.StatusCode, - "message": ghErr.Message, - "errors": ghErr.Errors, - }) - } - return diag.FromErr(err) } @@ -330,19 +314,109 @@ func resourceGithubOrganizationSettingsCreate(ctx context.Context, d *schema.Res return nil } -func resourceGithubOrganizationSettingsUpdate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { - if err := checkOrganization(meta); err != nil { +func resourceGithubOrganizationSettingsRead(ctx context.Context, d *schema.ResourceData, m any) diag.Diagnostics { + meta, _ := m.(*Owner) + + if ok, diags := checkOrganizationOK(meta); !ok { + return diags + } + + orgSettings, _, err := meta.v3client.Organizations.Get(ctx, meta.name) + if err != nil { + return diag.FromErr(err) + } + + if err = d.Set("billing_email", orgSettings.GetBillingEmail()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("company", orgSettings.GetCompany()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("email", orgSettings.GetEmail()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("twitter_username", orgSettings.GetTwitterUsername()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("location", orgSettings.GetLocation()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("name", orgSettings.GetName()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("description", orgSettings.GetDescription()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("has_organization_projects", orgSettings.GetHasOrganizationProjects()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("has_repository_projects", orgSettings.GetHasRepositoryProjects()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("default_repository_permission", orgSettings.GetDefaultRepoPermission()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("members_can_create_repositories", orgSettings.GetMembersCanCreateRepos()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("members_can_create_internal_repositories", orgSettings.GetMembersCanCreateInternalRepos()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("members_can_create_private_repositories", orgSettings.GetMembersCanCreatePrivateRepos()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("members_can_create_public_repositories", orgSettings.GetMembersCanCreatePublicRepos()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("members_can_create_pages", orgSettings.GetMembersCanCreatePages()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("members_can_create_public_pages", orgSettings.GetMembersCanCreatePublicPages()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("members_can_create_private_pages", orgSettings.GetMembersCanCreatePrivatePages()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("members_can_fork_private_repositories", orgSettings.GetMembersCanForkPrivateRepos()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("web_commit_signoff_required", orgSettings.GetWebCommitSignoffRequired()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("blog", orgSettings.GetBlog()); err != nil { return diag.FromErr(err) } + if err = d.Set("advanced_security_enabled_for_new_repositories", orgSettings.GetAdvancedSecurityEnabledForNewRepos()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("dependabot_alerts_enabled_for_new_repositories", orgSettings.GetDependabotAlertsEnabledForNewRepos()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("dependabot_security_updates_enabled_for_new_repositories", orgSettings.GetDependabotSecurityUpdatesEnabledForNewRepos()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("dependency_graph_enabled_for_new_repositories", orgSettings.GetDependencyGraphEnabledForNewRepos()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("secret_scanning_enabled_for_new_repositories", orgSettings.GetSecretScanningEnabledForNewRepos()); err != nil { + return diag.FromErr(err) + } + if err = d.Set("secret_scanning_push_protection_enabled_for_new_repositories", orgSettings.GetSecretScanningPushProtectionEnabledForNewRepos()); err != nil { + return diag.FromErr(err) + } + return nil +} + +func resourceGithubOrganizationSettingsUpdate(ctx context.Context, d *schema.ResourceData, m any) diag.Diagnostics { + meta, _ := m.(*Owner) - owner, ok := meta.(*Owner) - if !ok { - return diag.Errorf("unexpected provider meta type %T", meta) + if ok, diags := checkOrganizationOK(meta); !ok { + return diags } ctx = context.WithValue(ctx, ctxId, d.Id()) - orgInfo, _, err := owner.v3client.Organizations.Get(ctx, owner.name) + orgInfo, _, err := meta.v3client.Organizations.Get(ctx, meta.name) if err != nil { return diag.FromErr(err) } @@ -478,138 +552,27 @@ func resourceGithubOrganizationSettingsUpdate(ctx context.Context, d *schema.Res } } - tflog.Debug(ctx, "Updating organization settings", map[string]any{ - "org": owner.name, - "enterprise": isEnterprise, - "settings": settings.String(), - }) - - if _, _, err := owner.v3client.Organizations.Edit(ctx, owner.name, settings); err != nil { - if ghErr, ok := errors.AsType[*github.ErrorResponse](err); ok { - tflog.Debug(ctx, "GitHub API error", map[string]any{ - "status": ghErr.Response.StatusCode, - "message": ghErr.Message, - "errors": ghErr.Errors, - }) - } - - return diag.FromErr(err) - } - - return nil -} - -func resourceGithubOrganizationSettingsRead(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { - if err := checkOrganization(meta); err != nil { - return diag.FromErr(err) - } - - owner, ok := meta.(*Owner) - if !ok { - return diag.Errorf("unexpected provider meta type %T", meta) - } + tflog.Debug(ctx, "Updating organization settings", map[string]any{"org": meta.name, "enterprise": isEnterprise, "settings": settings.String()}) - orgSettings, _, err := owner.v3client.Organizations.Get(ctx, owner.name) - if err != nil { + if _, _, err := meta.v3client.Organizations.Edit(ctx, meta.name, settings); err != nil { return diag.FromErr(err) } - if err = d.Set("billing_email", orgSettings.GetBillingEmail()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("company", orgSettings.GetCompany()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("email", orgSettings.GetEmail()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("twitter_username", orgSettings.GetTwitterUsername()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("location", orgSettings.GetLocation()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("name", orgSettings.GetName()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("description", orgSettings.GetDescription()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("has_organization_projects", orgSettings.GetHasOrganizationProjects()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("has_repository_projects", orgSettings.GetHasRepositoryProjects()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("default_repository_permission", orgSettings.GetDefaultRepoPermission()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("members_can_create_repositories", orgSettings.GetMembersCanCreateRepos()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("members_can_create_internal_repositories", orgSettings.GetMembersCanCreateInternalRepos()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("members_can_create_private_repositories", orgSettings.GetMembersCanCreatePrivateRepos()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("members_can_create_public_repositories", orgSettings.GetMembersCanCreatePublicRepos()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("members_can_create_pages", orgSettings.GetMembersCanCreatePages()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("members_can_create_public_pages", orgSettings.GetMembersCanCreatePublicPages()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("members_can_create_private_pages", orgSettings.GetMembersCanCreatePrivatePages()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("members_can_fork_private_repositories", orgSettings.GetMembersCanForkPrivateRepos()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("web_commit_signoff_required", orgSettings.GetWebCommitSignoffRequired()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("blog", orgSettings.GetBlog()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("advanced_security_enabled_for_new_repositories", orgSettings.GetAdvancedSecurityEnabledForNewRepos()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("dependabot_alerts_enabled_for_new_repositories", orgSettings.GetDependabotAlertsEnabledForNewRepos()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("dependabot_security_updates_enabled_for_new_repositories", orgSettings.GetDependabotSecurityUpdatesEnabledForNewRepos()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("dependency_graph_enabled_for_new_repositories", orgSettings.GetDependencyGraphEnabledForNewRepos()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("secret_scanning_enabled_for_new_repositories", orgSettings.GetSecretScanningEnabledForNewRepos()); err != nil { - return diag.FromErr(err) - } - if err = d.Set("secret_scanning_push_protection_enabled_for_new_repositories", orgSettings.GetSecretScanningPushProtectionEnabledForNewRepos()); err != nil { - return diag.FromErr(err) - } return nil } -func resourceGithubOrganizationSettingsDelete(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { - if err := checkOrganization(meta); err != nil { - return diag.FromErr(err) - } +func resourceGithubOrganizationSettingsDelete(ctx context.Context, d *schema.ResourceData, m any) diag.Diagnostics { + meta, _ := m.(*Owner) - owner, ok := meta.(*Owner) - if !ok { - return diag.Errorf("unexpected provider meta type %T", meta) + if ok, diags := checkOrganizationOK(meta); !ok { + return diags } ctx = context.WithValue(ctx, ctxId, d.Id()) - tflog.Debug(ctx, "Reverting organization settings to default values", map[string]any{"org": owner.name}) + tflog.Debug(ctx, "Reverting organization settings to default values", map[string]any{"org": meta.name}) - orgInfo, _, err := owner.v3client.Organizations.Get(ctx, owner.name) + orgInfo, _, err := meta.v3client.Organizations.Get(ctx, meta.name) if err != nil { return diag.FromErr(err) } @@ -625,7 +588,7 @@ func resourceGithubOrganizationSettingsDelete(ctx context.Context, d *schema.Res defaultSettings.MembersCanCreateInternalRepos = new(true) } - if _, _, err := owner.v3client.Organizations.Edit(ctx, owner.name, defaultSettings); err != nil { + if _, _, err := meta.v3client.Organizations.Edit(ctx, meta.name, defaultSettings); err != nil { return diag.FromErr(err) }