diff --git a/github/resource_github_branch_protection_v3_test.go b/github/resource_github_branch_protection_v3_test.go index 2ced28774a..45c093ea2b 100644 --- a/github/resource_github_branch_protection_v3_test.go +++ b/github/resource_github_branch_protection_v3_test.go @@ -6,6 +6,9 @@ import ( "github.com/hashicorp/terraform-plugin-testing/helper/acctest" "github.com/hashicorp/terraform-plugin-testing/helper/resource" + "github.com/hashicorp/terraform-plugin-testing/knownvalue" + "github.com/hashicorp/terraform-plugin-testing/statecheck" + "github.com/hashicorp/terraform-plugin-testing/tfjsonpath" ) func TestAccGithubBranchProtectionV3_required_pull_request_reviews(t *testing.T) { @@ -360,6 +363,64 @@ func TestAccGithubBranchProtectionV3_computed_status_contexts_no_churn(t *testin }) } +func TestAccGithubBranchProtectionV3_update_with_status_checks(t *testing.T) { + t.Parallel() + + // A read populates both `contexts` and `checks` from the API response, so updating + // any other setting used to send every required check twice, which GitHub rejects + // with "Context must be unique per branch protection". + for _, statusChecksField := range []string{"contexts", "checks"} { + t.Run(fmt.Sprintf("updates other settings when %s is set", statusChecksField), func(t *testing.T) { + t.Parallel() + + repo := mustCreateTestRepository(t) + + config := func(enforceAdmins bool) string { + return fmt.Sprintf(` + resource "github_branch_protection_v3" "test" { + repository = "%s" + branch = "main" + enforce_admins = %t + + required_status_checks { + strict = true + %s = [ + "ci/test", + "ci/build" + ] + } + } + `, repo.GetName(), enforceAdmins, statusChecksField) + } + + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnauthenticated(t) }, + ProviderFactories: providerFactories, + Steps: []resource.TestStep{ + { + Config: config(false), + }, + { + Config: config(true), + ConfigStateChecks: []statecheck.StateCheck{ + statecheck.ExpectKnownValue( + "github_branch_protection_v3.test", + tfjsonpath.New("enforce_admins"), + knownvalue.Bool(true), + ), + statecheck.ExpectKnownValue( + "github_branch_protection_v3.test", + tfjsonpath.New("required_status_checks").AtSliceIndex(0).AtMapKey(statusChecksField), + knownvalue.SetSizeExact(2), + ), + }, + }, + }, + }) + }) + } +} + func TestAccGithubBranchProtectionV3(t *testing.T) { t.Parallel() diff --git a/github/resource_github_branch_protection_v3_utils.go b/github/resource_github_branch_protection_v3_utils.go index 05ff6ab1e9..03991bfe32 100644 --- a/github/resource_github_branch_protection_v3_utils.go +++ b/github/resource_github_branch_protection_v3_utils.go @@ -261,17 +261,7 @@ func expandRequiredStatusChecks(d *schema.ResourceData) (*github.RequiredStatusC // Initialise empty literal to ensure an empty array is passed mitigating schema errors like so: // For 'anyOf/1', {"strict"=>true, "checks"=>nil} is not a null. [] rscChecks := []*github.RequiredStatusCheck{} - - // TODO: Remove once contexts is deprecated - // Iterate and parse contexts into checks using -1 as default to allow checks from all apps. - contexts := expandNestedSet(m, "contexts") - for _, c := range contexts { - appID := int64(-1) // Default - rscChecks = append(rscChecks, &github.RequiredStatusCheck{ - Context: c, - AppID: &appID, - }) - } + rscChecksByContext := make(map[string]*github.RequiredStatusCheck) // Iterate and parse checks checks := expandNestedSet(m, "checks") @@ -303,6 +293,29 @@ func expandRequiredStatusChecks(d *schema.ResourceData) (*github.RequiredStatusC // Append rscChecks = append(rscChecks, rscCheck) + rscChecksByContext[cContext] = rscCheck + } + + // TODO: Remove once contexts is deprecated + // Iterate and parse contexts into checks using -1 as default to allow checks from all apps. + // A context can appear in both fields even though configuration cannot set both, because + // the API returns every required check under `contexts` as well as `checks`, so a read + // populates both. GitHub rejects a payload that repeats a context, so skip the duplicates. + contexts := expandNestedSet(m, "contexts") + for _, c := range contexts { + appID := int64(-1) // Default + if rscCheck, ok := rscChecksByContext[c]; ok { + if rscCheck.AppID == nil { + // `app_id: null` was read back, meaning any app is allowed. Omitting app_id + // instead would let GitHub pick an app, so keep it explicit. + rscCheck.AppID = &appID + } + continue + } + rscChecks = append(rscChecks, &github.RequiredStatusCheck{ + Context: c, + AppID: &appID, + }) } // Assign after looping both checks and contexts rsc.Checks = &rscChecks diff --git a/github/resource_github_branch_protection_v3_utils_test.go b/github/resource_github_branch_protection_v3_utils_test.go new file mode 100644 index 0000000000..bd906fb137 --- /dev/null +++ b/github/resource_github_branch_protection_v3_utils_test.go @@ -0,0 +1,98 @@ +package github + +import ( + "testing" + + "github.com/google/go-github/v89/github" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" +) + +func TestExpandRequiredStatusChecks(t *testing.T) { + appID := int64(15368) + anyApp := int64(-1) + + testCases := map[string]struct { + contexts []any + checks []any + want []*github.RequiredStatusCheck + }{ + "contexts only": { + contexts: []any{"ci/test"}, + want: []*github.RequiredStatusCheck{ + {Context: "ci/test", AppID: &anyApp}, + }, + }, + "checks only": { + checks: []any{"ci/test:15368"}, + want: []*github.RequiredStatusCheck{ + {Context: "ci/test", AppID: &appID}, + }, + }, + // A read populates both fields from the API response, so state can hold the + // same context twice even though configuration cannot set both fields. + "same context in both fields is sent once": { + contexts: []any{"ci/test", "ci/build"}, + checks: []any{"ci/test", "ci/build"}, + want: []*github.RequiredStatusCheck{ + {Context: "ci/test", AppID: &anyApp}, + {Context: "ci/build", AppID: &anyApp}, + }, + }, + "app_id from checks wins over the context duplicate": { + contexts: []any{"ci/test"}, + checks: []any{"ci/test:15368"}, + want: []*github.RequiredStatusCheck{ + {Context: "ci/test", AppID: &appID}, + }, + }, + } + + for name, tc := range testCases { + t.Run(name, func(t *testing.T) { + statusChecks := map[string]any{"strict": true} + if tc.contexts != nil { + statusChecks["contexts"] = schema.NewSet(schema.HashString, tc.contexts) + } + if tc.checks != nil { + statusChecks["checks"] = schema.NewSet(schema.HashString, tc.checks) + } + + d := schema.TestResourceDataRaw(t, resourceGithubBranchProtectionV3().Schema, map[string]any{ + "repository": "test", + "branch": "main", + }) + if err := d.Set("required_status_checks", []any{statusChecks}); err != nil { + t.Fatalf("failed to set required_status_checks: %v", err) + } + + rsc, err := expandRequiredStatusChecks(d) + if err != nil { + t.Fatalf("expected no error, got: %v", err) + } + if !rsc.Strict { + t.Error("expected strict to be true") + } + + got := map[string]*int64{} + for _, check := range *rsc.Checks { + if _, ok := got[check.Context]; ok { + t.Fatalf("context %q sent more than once, GitHub rejects duplicates", check.Context) + } + got[check.Context] = check.AppID + } + + if len(got) != len(tc.want) { + t.Fatalf("expected %d checks, got %d: %v", len(tc.want), len(got), got) + } + for _, want := range tc.want { + gotAppID, ok := got[want.Context] + if !ok { + t.Fatalf("expected context %q to be present", want.Context) + } + if gotAppID == nil || *gotAppID != *want.AppID { + t.Errorf("context %q: expected app_id %d, got %v", want.Context, *want.AppID, gotAppID) + } + } + }) + } +}