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.go b/github/resource_github_organization_settings.go index b8ba89060c..045804840a 100644 --- a/github/resource_github_organization_settings.go +++ b/github/resource_github_organization_settings.go @@ -2,21 +2,21 @@ package github import ( "context" - "errors" - "log" "strconv" "github.com/google/go-github/v92/github" + "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" ) 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,368 +173,412 @@ 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. -func buildOrganizationSettings(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() != "" +func resourceGithubOrganizationSettingsCreate(ctx context.Context, d *schema.ResourceData, m any) diag.Diagnostics { + meta, _ := m.(*Owner) - // 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 ok, diags := checkOrganizationOK(meta); !ok { + return diags } - // 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)) + ctx = context.WithValue(ctx, ctxId, d.Id()) + + orgInfo, _, err := meta.v3client.Organizations.Get(ctx, meta.name) + if err != nil { + return diag.FromErr(err) } + isEnterprise := orgInfo.GetPlan().GetName() == "enterprise" - // Optional string fields - only set if should be included - if shouldInclude("company") { - if company, ok := d.GetOk("company"); ok { - settings.Company = new(company.(string)) - } + // 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{} + + if _, ok := d.GetOk("billing_email"); ok || isConfigured(d, "billing_email") { + v, _ := d.Get("billing_email").(string) + settings.BillingEmail = new(v) } - if shouldInclude("email") { - if email, ok := d.GetOk("email"); ok { - settings.Email = new(email.(string)) - } + if _, ok := d.GetOk("company"); ok || isConfigured(d, "company") { + v, _ := d.Get("company").(string) + settings.Company = new(v) } - if shouldInclude("twitter_username") { - if twitterUsername, ok := d.GetOk("twitter_username"); ok { - settings.TwitterUsername = new(twitterUsername.(string)) - } + if _, ok := d.GetOk("email"); ok || isConfigured(d, "email") { + v, _ := d.Get("email").(string) + settings.Email = new(v) } - if shouldInclude("location") { - if location, ok := d.GetOk("location"); ok { - settings.Location = new(location.(string)) - } + if _, ok := d.GetOk("twitter_username"); ok || isConfigured(d, "twitter_username") { + v, _ := d.Get("twitter_username").(string) + settings.TwitterUsername = new(v) } - if shouldInclude("name") { - if name, ok := d.GetOk("name"); ok { - settings.Name = new(name.(string)) - } + if _, ok := d.GetOk("location"); ok || isConfigured(d, "location") { + v, _ := d.Get("location").(string) + settings.Location = new(v) } - if shouldInclude("description") { - if description, ok := d.GetOk("description"); ok { - settings.Description = new(description.(string)) - } + if _, ok := d.GetOk("name"); ok || isConfigured(d, "name") { + v, _ := d.Get("name").(string) + settings.Name = new(v) } - if shouldInclude("blog") { - if blog, ok := d.GetOk("blog"); ok { - settings.Blog = new(blog.(string)) - } + if _, ok := d.GetOk("description"); ok || isConfigured(d, "description") { + v, _ := d.Get("description").(string) + settings.Description = new(v) } - - // Boolean fields - only set if should be included - // Use d.Get() instead of d.GetOk() when shouldInclude() returns true, - // because we already know the field should be included, and d.Get() correctly handles false values - if shouldInclude("has_organization_projects") { - settings.HasOrganizationProjects = new(d.Get("has_organization_projects").(bool)) + if _, ok := d.GetOk("blog"); ok || isConfigured(d, "blog") { + v, _ := d.Get("blog").(string) + settings.Blog = new(v) } - if shouldInclude("has_repository_projects") { - settings.HasRepositoryProjects = new(d.Get("has_repository_projects").(bool)) + 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 shouldInclude("default_repository_permission") { - if defaultRepoPermission, ok := d.GetOk("default_repository_permission"); ok { - settings.DefaultRepoPermission = new(defaultRepoPermission.(string)) - } + 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 _, ok := d.GetOk("default_repository_permission"); ok || isConfigured(d, "default_repository_permission") { + v, _ := d.Get("default_repository_permission").(string) + settings.DefaultRepoPermission = new(v) } - if shouldInclude("members_can_create_repositories") { - settings.MembersCanCreateRepos = new(d.Get("members_can_create_repositories").(bool)) + 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 shouldInclude("members_can_create_private_repositories") { - settings.MembersCanCreatePrivateRepos = new(d.Get("members_can_create_private_repositories").(bool)) + 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 shouldInclude("members_can_create_public_repositories") { - settings.MembersCanCreatePublicRepos = new(d.Get("members_can_create_public_repositories").(bool)) + 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 shouldInclude("members_can_create_pages") { - settings.MembersCanCreatePages = new(d.Get("members_can_create_pages").(bool)) + 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 shouldInclude("members_can_create_public_pages") { - settings.MembersCanCreatePublicPages = new(d.Get("members_can_create_public_pages").(bool)) + 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 shouldInclude("members_can_create_private_pages") { - settings.MembersCanCreatePrivatePages = new(d.Get("members_can_create_private_pages").(bool)) + 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 shouldInclude("members_can_fork_private_repositories") { - settings.MembersCanForkPrivateRepos = new(d.Get("members_can_fork_private_repositories").(bool)) + 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 shouldInclude("web_commit_signoff_required") { - settings.WebCommitSignoffRequired = new(d.Get("web_commit_signoff_required").(bool)) + 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 shouldInclude("advanced_security_enabled_for_new_repositories") { - settings.AdvancedSecurityEnabledForNewRepos = new(d.Get("advanced_security_enabled_for_new_repositories").(bool)) + 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 shouldInclude("dependabot_alerts_enabled_for_new_repositories") { - settings.DependabotAlertsEnabledForNewRepos = new(d.Get("dependabot_alerts_enabled_for_new_repositories").(bool)) + 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 shouldInclude("dependabot_security_updates_enabled_for_new_repositories") { - settings.DependabotSecurityUpdatesEnabledForNewRepos = new(d.Get("dependabot_security_updates_enabled_for_new_repositories").(bool)) + 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 shouldInclude("dependency_graph_enabled_for_new_repositories") { - settings.DependencyGraphEnabledForNewRepos = new(d.Get("dependency_graph_enabled_for_new_repositories").(bool)) + 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 shouldInclude("secret_scanning_enabled_for_new_repositories") { - settings.SecretScanningEnabledForNewRepos = new(d.Get("secret_scanning_enabled_for_new_repositories").(bool)) + 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 shouldInclude("secret_scanning_push_protection_enabled_for_new_repositories") { - settings.SecretScanningPushProtectionEnabledForNewRepos = new(d.Get("secret_scanning_push_protection_enabled_for_new_repositories").(bool)) + 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) } - // Enterprise-specific field if isEnterprise { - if shouldInclude("members_can_create_internal_repositories") { - settings.MembersCanCreateInternalRepos = new(d.Get("members_can_create_internal_repositories").(bool)) + 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": meta.name, "enterprise": isEnterprise, "settings": settings.String()}) -func resourceGithubOrganizationSettingsCreateOrUpdate(d *schema.ResourceData, meta any) error { - err := checkOrganization(meta) + orgSettings, _, err := meta.v3client.Organizations.Edit(ctx, meta.name, settings) if err != nil { - return err + return diag.FromErr(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 + d.SetId(strconv.FormatInt(orgSettings.GetID(), 10)) + + return nil +} + +func resourceGithubOrganizationSettingsRead(ctx context.Context, d *schema.ResourceData, m any) diag.Diagnostics { + meta, _ := m.(*Owner) + + if ok, diags := checkOrganizationOK(meta); !ok { + return diags } - // Build settings using helper function - isEnterprise := orgInfo.GetPlan().GetName() == "enterprise" - settings := buildOrganizationSettings(d, isEnterprise) + orgSettings, _, err := meta.v3client.Organizations.Get(ctx, meta.name) + if err != nil { + return diag.FromErr(err) + } - // Debug: Log the settings being sent to the API with detailed field information - log.Printf("[DEBUG] Built settings for org %s (enterprise: %v)", org, isEnterprise) - if settings.BillingEmail != nil { - log.Printf("[DEBUG] BillingEmail: %s", *settings.BillingEmail) + if err = d.Set("billing_email", orgSettings.GetBillingEmail()); err != nil { + return diag.FromErr(err) } - if settings.Company != nil { - log.Printf("[DEBUG] Company: %s", *settings.Company) + if err = d.Set("company", orgSettings.GetCompany()); err != nil { + return diag.FromErr(err) } - if settings.Email != nil { - log.Printf("[DEBUG] Email: %s", *settings.Email) + if err = d.Set("email", orgSettings.GetEmail()); err != nil { + return diag.FromErr(err) } - if settings.TwitterUsername != nil { - log.Printf("[DEBUG] TwitterUsername: %s", *settings.TwitterUsername) + if err = d.Set("twitter_username", orgSettings.GetTwitterUsername()); err != nil { + return diag.FromErr(err) } - if settings.Location != nil { - log.Printf("[DEBUG] Location: %s", *settings.Location) + if err = d.Set("location", orgSettings.GetLocation()); err != nil { + return diag.FromErr(err) } - if settings.Name != nil { - log.Printf("[DEBUG] Name: %s", *settings.Name) + if err = d.Set("name", orgSettings.GetName()); err != nil { + return diag.FromErr(err) } - if settings.Description != nil { - log.Printf("[DEBUG] Description: %s", *settings.Description) + if err = d.Set("description", orgSettings.GetDescription()); err != nil { + return diag.FromErr(err) } - if settings.Blog != nil { - log.Printf("[DEBUG] Blog: %s", *settings.Blog) + if err = d.Set("has_organization_projects", orgSettings.GetHasOrganizationProjects()); err != nil { + return diag.FromErr(err) } - if settings.HasOrganizationProjects != nil { - log.Printf("[DEBUG] HasOrganizationProjects: %v", *settings.HasOrganizationProjects) + if err = d.Set("has_repository_projects", orgSettings.GetHasRepositoryProjects()); err != nil { + return diag.FromErr(err) } - if settings.HasRepositoryProjects != nil { - log.Printf("[DEBUG] HasRepositoryProjects: %v", *settings.HasRepositoryProjects) + if err = d.Set("default_repository_permission", orgSettings.GetDefaultRepoPermission()); err != nil { + return diag.FromErr(err) } - if settings.DefaultRepoPermission != nil { - log.Printf("[DEBUG] DefaultRepoPermission: %s", *settings.DefaultRepoPermission) + if err = d.Set("members_can_create_repositories", orgSettings.GetMembersCanCreateRepos()); err != nil { + return diag.FromErr(err) } - if settings.MembersCanCreateRepos != nil { - log.Printf("[DEBUG] MembersCanCreateRepos: %v", *settings.MembersCanCreateRepos) + if err = d.Set("members_can_create_internal_repositories", orgSettings.GetMembersCanCreateInternalRepos()); err != nil { + return diag.FromErr(err) } - if settings.MembersCanCreatePrivateRepos != nil { - log.Printf("[DEBUG] MembersCanCreatePrivateRepos: %v", *settings.MembersCanCreatePrivateRepos) + if err = d.Set("members_can_create_private_repositories", orgSettings.GetMembersCanCreatePrivateRepos()); err != nil { + return diag.FromErr(err) } - if settings.MembersCanCreatePublicRepos != nil { - log.Printf("[DEBUG] MembersCanCreatePublicRepos: %v", *settings.MembersCanCreatePublicRepos) + if err = d.Set("members_can_create_public_repositories", orgSettings.GetMembersCanCreatePublicRepos()); err != nil { + return diag.FromErr(err) } - if settings.MembersCanCreateInternalRepos != nil { - log.Printf("[DEBUG] MembersCanCreateInternalRepos: %v", *settings.MembersCanCreateInternalRepos) + if err = d.Set("members_can_create_pages", orgSettings.GetMembersCanCreatePages()); err != nil { + return diag.FromErr(err) } - if settings.MembersCanCreatePages != nil { - log.Printf("[DEBUG] MembersCanCreatePages: %v", *settings.MembersCanCreatePages) + if err = d.Set("members_can_create_public_pages", orgSettings.GetMembersCanCreatePublicPages()); err != nil { + return diag.FromErr(err) } - if settings.MembersCanCreatePublicPages != nil { - log.Printf("[DEBUG] MembersCanCreatePublicPages: %v", *settings.MembersCanCreatePublicPages) + if err = d.Set("members_can_create_private_pages", orgSettings.GetMembersCanCreatePrivatePages()); err != nil { + return diag.FromErr(err) } - if settings.MembersCanCreatePrivatePages != nil { - log.Printf("[DEBUG] MembersCanCreatePrivatePages: %v", *settings.MembersCanCreatePrivatePages) + if err = d.Set("members_can_fork_private_repositories", orgSettings.GetMembersCanForkPrivateRepos()); err != nil { + return diag.FromErr(err) } - if settings.MembersCanForkPrivateRepos != nil { - log.Printf("[DEBUG] MembersCanForkPrivateRepos: %v", *settings.MembersCanForkPrivateRepos) + if err = d.Set("web_commit_signoff_required", orgSettings.GetWebCommitSignoffRequired()); err != nil { + return diag.FromErr(err) } - if settings.WebCommitSignoffRequired != nil { - log.Printf("[DEBUG] WebCommitSignoffRequired: %v", *settings.WebCommitSignoffRequired) + if err = d.Set("blog", orgSettings.GetBlog()); err != nil { + return diag.FromErr(err) } - if settings.AdvancedSecurityEnabledForNewRepos != nil { - log.Printf("[DEBUG] AdvancedSecurityEnabledForNewRepos: %v", *settings.AdvancedSecurityEnabledForNewRepos) + if err = d.Set("advanced_security_enabled_for_new_repositories", orgSettings.GetAdvancedSecurityEnabledForNewRepos()); err != nil { + return diag.FromErr(err) } - if settings.DependabotAlertsEnabledForNewRepos != nil { - log.Printf("[DEBUG] DependabotAlertsEnabledForNewRepos: %v", *settings.DependabotAlertsEnabledForNewRepos) + if err = d.Set("dependabot_alerts_enabled_for_new_repositories", orgSettings.GetDependabotAlertsEnabledForNewRepos()); err != nil { + return diag.FromErr(err) } - if settings.DependabotSecurityUpdatesEnabledForNewRepos != nil { - log.Printf("[DEBUG] DependabotSecurityUpdatesEnabledForNewRepos: %v", *settings.DependabotSecurityUpdatesEnabledForNewRepos) + if err = d.Set("dependabot_security_updates_enabled_for_new_repositories", orgSettings.GetDependabotSecurityUpdatesEnabledForNewRepos()); err != nil { + return diag.FromErr(err) } - if settings.DependencyGraphEnabledForNewRepos != nil { - log.Printf("[DEBUG] DependencyGraphEnabledForNewRepos: %v", *settings.DependencyGraphEnabledForNewRepos) + if err = d.Set("dependency_graph_enabled_for_new_repositories", orgSettings.GetDependencyGraphEnabledForNewRepos()); err != nil { + return diag.FromErr(err) } - if settings.SecretScanningEnabledForNewRepos != nil { - log.Printf("[DEBUG] SecretScanningEnabledForNewRepos: %v", *settings.SecretScanningEnabledForNewRepos) + if err = d.Set("secret_scanning_enabled_for_new_repositories", orgSettings.GetSecretScanningEnabledForNewRepos()); err != nil { + return diag.FromErr(err) } - if settings.SecretScanningPushProtectionEnabledForNewRepos != nil { - log.Printf("[DEBUG] SecretScanningPushProtectionEnabledForNewRepos: %v", *settings.SecretScanningPushProtectionEnabledForNewRepos) + if err = d.Set("secret_scanning_push_protection_enabled_for_new_repositories", orgSettings.GetSecretScanningPushProtectionEnabledForNewRepos()); err != nil { + return diag.FromErr(err) } + return nil +} - orgSettings, _, err := client.Organizations.Edit(ctx, org, settings) - if err != nil { - // Log detailed error information for debugging - 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) - } - } - } - return err +func resourceGithubOrganizationSettingsUpdate(ctx context.Context, d *schema.ResourceData, m any) diag.Diagnostics { + meta, _ := m.(*Owner) + + if ok, diags := checkOrganizationOK(meta); !ok { + return diags } - id := strconv.FormatInt(orgSettings.GetID(), 10) - d.SetId(id) - return resourceGithubOrganizationSettingsRead(d, meta) -} + ctx = context.WithValue(ctx, ctxId, d.Id()) -func resourceGithubOrganizationSettingsRead(d *schema.ResourceData, meta any) error { - err := checkOrganization(meta) + orgInfo, _, err := meta.v3client.Organizations.Get(ctx, meta.name) if err != nil { - return err + return diag.FromErr(err) } - client := meta.(*Owner).v3client - ctx := context.Background() - org := meta.(*Owner).name + isEnterprise := orgInfo.GetPlan().GetName() == "enterprise" - orgSettings, _, err := client.Organizations.Get(ctx, org) - if err != nil { - return err + // 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 v, ok := d.GetOk("billing_email"); ok { + billingEmail, _ := v.(string) + settings.BillingEmail = new(billingEmail) } - if err = d.Set("billing_email", orgSettings.GetBillingEmail()); err != nil { - return err + if d.HasChange("company") { + if v, ok := d.GetOk("company"); ok { + s, _ := v.(string) + settings.Company = new(s) + } } - if err = d.Set("company", orgSettings.GetCompany()); err != nil { - return err + if d.HasChange("email") { + if v, ok := d.GetOk("email"); ok { + s, _ := v.(string) + settings.Email = new(s) + } } - if err = d.Set("email", orgSettings.GetEmail()); err != nil { - return err + if d.HasChange("twitter_username") { + if v, ok := d.GetOk("twitter_username"); ok { + s, _ := v.(string) + settings.TwitterUsername = new(s) + } } - if err = d.Set("twitter_username", orgSettings.GetTwitterUsername()); err != nil { - return err + if d.HasChange("location") { + if v, ok := d.GetOk("location"); ok { + s, _ := v.(string) + settings.Location = new(s) + } } - if err = d.Set("location", orgSettings.GetLocation()); err != nil { - return err + if d.HasChange("name") { + if v, ok := d.GetOk("name"); ok { + s, _ := v.(string) + settings.Name = new(s) + } } - if err = d.Set("name", orgSettings.GetName()); err != nil { - return err + if d.HasChange("description") { + if v, ok := d.GetOk("description"); ok { + s, _ := v.(string) + settings.Description = new(s) + } } - if err = d.Set("description", orgSettings.GetDescription()); err != nil { - return err + if d.HasChange("blog") { + if v, ok := d.GetOk("blog"); ok { + s, _ := v.(string) + settings.Blog = new(s) + } } - if err = d.Set("has_organization_projects", orgSettings.GetHasOrganizationProjects()); err != nil { - return err + if d.HasChange("has_organization_projects") { + v, _ := d.Get("has_organization_projects").(bool) + settings.HasOrganizationProjects = new(v) } - if err = d.Set("has_repository_projects", orgSettings.GetHasRepositoryProjects()); err != nil { - return err + if d.HasChange("has_repository_projects") { + v, _ := d.Get("has_repository_projects").(bool) + settings.HasRepositoryProjects = new(v) } - if err = d.Set("default_repository_permission", orgSettings.GetDefaultRepoPermission()); err != nil { - return err + if d.HasChange("default_repository_permission") { + if v, ok := d.GetOk("default_repository_permission"); ok { + s, _ := v.(string) + settings.DefaultRepoPermission = new(s) + } } - if err = d.Set("members_can_create_repositories", orgSettings.GetMembersCanCreateRepos()); err != nil { - return err + if d.HasChange("members_can_create_repositories") { + v, _ := d.Get("members_can_create_repositories").(bool) + settings.MembersCanCreateRepos = new(v) } - if err = d.Set("members_can_create_internal_repositories", orgSettings.GetMembersCanCreateInternalRepos()); err != nil { - return err + if d.HasChange("members_can_create_private_repositories") { + v, _ := d.Get("members_can_create_private_repositories").(bool) + settings.MembersCanCreatePrivateRepos = new(v) } - if err = d.Set("members_can_create_private_repositories", orgSettings.GetMembersCanCreatePrivateRepos()); err != nil { - return err + if d.HasChange("members_can_create_public_repositories") { + v, _ := d.Get("members_can_create_public_repositories").(bool) + settings.MembersCanCreatePublicRepos = new(v) } - if err = d.Set("members_can_create_public_repositories", orgSettings.GetMembersCanCreatePublicRepos()); err != nil { - return err + if d.HasChange("members_can_create_pages") { + v, _ := d.Get("members_can_create_pages").(bool) + settings.MembersCanCreatePages = new(v) } - if err = d.Set("members_can_create_pages", orgSettings.GetMembersCanCreatePages()); err != nil { - return err + if d.HasChange("members_can_create_public_pages") { + v, _ := d.Get("members_can_create_public_pages").(bool) + settings.MembersCanCreatePublicPages = new(v) } - if err = d.Set("members_can_create_public_pages", orgSettings.GetMembersCanCreatePublicPages()); err != nil { - return err + if d.HasChange("members_can_create_private_pages") { + v, _ := d.Get("members_can_create_private_pages").(bool) + settings.MembersCanCreatePrivatePages = new(v) } - if err = d.Set("members_can_create_private_pages", orgSettings.GetMembersCanCreatePrivatePages()); err != nil { - return err + if d.HasChange("members_can_fork_private_repositories") { + v, _ := d.Get("members_can_fork_private_repositories").(bool) + settings.MembersCanForkPrivateRepos = new(v) } - if err = d.Set("members_can_fork_private_repositories", orgSettings.GetMembersCanForkPrivateRepos()); err != nil { - return err + if d.HasChange("web_commit_signoff_required") { + v, _ := d.Get("web_commit_signoff_required").(bool) + settings.WebCommitSignoffRequired = new(v) } - if err = d.Set("web_commit_signoff_required", orgSettings.GetWebCommitSignoffRequired()); err != nil { - return err + if d.HasChange("advanced_security_enabled_for_new_repositories") { + v, _ := d.Get("advanced_security_enabled_for_new_repositories").(bool) + settings.AdvancedSecurityEnabledForNewRepos = new(v) } - if err = d.Set("blog", orgSettings.GetBlog()); err != nil { - return err + if d.HasChange("dependabot_alerts_enabled_for_new_repositories") { + v, _ := d.Get("dependabot_alerts_enabled_for_new_repositories").(bool) + settings.DependabotAlertsEnabledForNewRepos = new(v) } - if err = d.Set("advanced_security_enabled_for_new_repositories", orgSettings.GetAdvancedSecurityEnabledForNewRepos()); err != nil { - return err + if d.HasChange("dependabot_security_updates_enabled_for_new_repositories") { + v, _ := d.Get("dependabot_security_updates_enabled_for_new_repositories").(bool) + settings.DependabotSecurityUpdatesEnabledForNewRepos = new(v) } - if err = d.Set("dependabot_alerts_enabled_for_new_repositories", orgSettings.GetDependabotAlertsEnabledForNewRepos()); err != nil { - return err + if d.HasChange("dependency_graph_enabled_for_new_repositories") { + v, _ := d.Get("dependency_graph_enabled_for_new_repositories").(bool) + settings.DependencyGraphEnabledForNewRepos = new(v) } - if err = d.Set("dependabot_security_updates_enabled_for_new_repositories", orgSettings.GetDependabotSecurityUpdatesEnabledForNewRepos()); err != nil { - return err + if d.HasChange("secret_scanning_enabled_for_new_repositories") { + v, _ := d.Get("secret_scanning_enabled_for_new_repositories").(bool) + settings.SecretScanningEnabledForNewRepos = new(v) } - if err = d.Set("dependency_graph_enabled_for_new_repositories", orgSettings.GetDependencyGraphEnabledForNewRepos()); err != nil { - return err + if d.HasChange("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 err = d.Set("secret_scanning_enabled_for_new_repositories", orgSettings.GetSecretScanningEnabledForNewRepos()); err != nil { - return err + + if isEnterprise { + if d.HasChange("members_can_create_internal_repositories") { + v, _ := d.Get("members_can_create_internal_repositories").(bool) + settings.MembersCanCreateInternalRepos = new(v) + } } - if err = d.Set("secret_scanning_push_protection_enabled_for_new_repositories", orgSettings.GetSecretScanningPushProtectionEnabledForNewRepos()); err != nil { - return err + + tflog.Debug(ctx, "Updating organization settings", map[string]any{"org": meta.name, "enterprise": isEnterprise, "settings": settings.String()}) + + if _, _, err := meta.v3client.Organizations.Edit(ctx, meta.name, settings); err != nil { + 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, m any) diag.Diagnostics { + meta, _ := m.(*Owner) + + if ok, diags := checkOrganizationOK(meta); !ok { + return diags } - client := meta.(*Owner).v3client - ctx := context.WithValue(context.Background(), ctxId, d.Id()) - org := meta.(*Owner).name + ctx = context.WithValue(ctx, ctxId, d.Id()) - log.Printf("[DEBUG] Reverting Organization Settings to default values: %s", org) + tflog.Debug(ctx, "Reverting organization settings to default values", map[string]any{"org": meta.name}) - // Get organization info to determine if it's enterprise - orgInfo, _, err := client.Organizations.Get(ctx, org) + orgInfo, _, err := meta.v3client.Organizations.Get(ctx, meta.name) if err != nil { - return err + return diag.FromErr(err) } + isEnterprise := orgInfo.GetPlan().GetName() == "enterprise" // Build minimal settings with only required fields - isEnterprise := orgInfo.GetPlan().GetName() == "enterprise" defaultSettings := &github.Organization{ BillingEmail: new("email@example.com"), } @@ -544,9 +588,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 := meta.v3client.Organizations.Edit(ctx, meta.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 f85b80bd6a..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 := ` @@ -222,6 +227,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" { 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() +}