diff --git a/github/config.go b/github/config.go index 3e0e2643e4..548897c635 100644 --- a/github/config.go +++ b/github/config.go @@ -47,7 +47,7 @@ var GHECDataResidencyHostMatch = regexp.MustCompile(`^[a-zA-Z0-9.\-]+\.ghe\.com\ func RateLimitedHTTPClient(client *http.Client, writeDelay, readDelay, retryDelay time.Duration, parallelRequests bool, retryableErrors map[int]bool, maxRetries int) *http.Client { client.Transport = NewEtagTransport(client.Transport) client.Transport = NewRateLimitTransport(client.Transport, WithWriteDelay(writeDelay), WithReadDelay(readDelay), WithParallelRequests(parallelRequests)) - client.Transport = logging.NewSubsystemLoggingHTTPTransport("GitHub", client.Transport) + client.Transport = logging.NewLoggingHTTPTransport(client.Transport) client.Transport = newPreviewHeaderInjectorTransport(map[string]string{ // TODO: remove when Stone Crop preview is moved to general availability in the GraphQL API "Accept": "application/vnd.github.stone-crop-preview+json", diff --git a/github/provider_utils.go b/github/provider_utils.go index 386a4e4407..36e976f254 100644 --- a/github/provider_utils.go +++ b/github/provider_utils.go @@ -13,7 +13,7 @@ var ( isPaidPlan = os.Getenv("GITHUB_PAID_FEATURES") testEnterprise = os.Getenv("ENTERPRISE_SLUG") testOrganization = testOrganizationFunc() - testOwner = os.Getenv("GITHUB_OWNER") + testOwner = testOwnerFunc() testToken = os.Getenv("GITHUB_TOKEN") testBaseURLGHES = os.Getenv("GHES_BASE_URL") ) @@ -54,8 +54,8 @@ func skipUnlessMode(t *testing.T, providerMode string) { t.Log("GITHUB_TOKEN environment variable should be empty") } case enterprise: - if os.Getenv("GITHUB_TOKEN") == "" { - t.Log("GITHUB_TOKEN environment variable should be set") + if os.Getenv("GITHUB_TOKEN") == "" || os.Getenv("ENTERPRISE_ACCOUNT") != "true" || os.Getenv("ENTERPRISE_SLUG") == "" { + t.Log("GITHUB_TOKEN and ENTERPRISE_ACCOUNT and ENTERPRISE_SLUG environment variables should be set") } else { return } @@ -67,10 +67,10 @@ func skipUnlessMode(t *testing.T, providerMode string) { t.Log("GITHUB_TOKEN and GITHUB_OWNER environment variables should be set") } case organization: - if os.Getenv("GITHUB_TOKEN") != "" && os.Getenv("GITHUB_ORGANIZATION") != "" { + if os.Getenv("GITHUB_TOKEN") != "" && (os.Getenv("GITHUB_ORGANIZATION") != "" || os.Getenv("GITHUB_TEST_ORGANIZATION") != "") { return } else { - t.Log("GITHUB_TOKEN and GITHUB_ORGANIZATION environment variables should be set") + t.Log("GITHUB_TOKEN and GITHUB_ORGANIZATION or GITHUB_TEST_ORGANIZATION environment variables should be set") } } @@ -120,6 +120,7 @@ func testOrganizationFunc() string { organization := os.Getenv("GITHUB_ORGANIZATION") if organization == "" { organization = os.Getenv("GITHUB_TEST_ORGANIZATION") + os.Setenv("GITHUB_ORGANIZATION", organization) } return organization } @@ -128,6 +129,7 @@ func testOwnerFunc() string { owner := os.Getenv("GITHUB_OWNER") if owner == "" { owner = os.Getenv("GITHUB_TEST_OWNER") + os.Setenv("GITHUB_OWNER", owner) } return owner } diff --git a/github/repository_rules_validation_utils.go b/github/repository_rules_validation_utils.go new file mode 100644 index 0000000000..a475e4dd28 --- /dev/null +++ b/github/repository_rules_validation_utils.go @@ -0,0 +1,157 @@ +package github + +import ( + "context" + "fmt" + "slices" + + "github.com/hashicorp/terraform-plugin-log/tflog" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" +) + +// branchTagOnlyRules contains rules that are only valid for branch and tag targets. +// +// These rules apply to ref-based operations (branches and tags) and are not supported +// for push rulesets which operate on file content. +// +// To verify/maintain this list: +// 1. Check the GitHub API documentation for organization rulesets: +// https://docs.github.com/en/rest/orgs/rules?apiVersion=2022-11-28#create-an-organization-repository-ruleset +// 2. The API docs don't clearly separate push vs branch/tag rules. To verify, +// attempt to create a push ruleset via API or UI with each rule type. +// Push rulesets will reject branch/tag rules with "Invalid rule ''" error. +// 3. Generally, push rules deal with file content (paths, sizes, extensions), +// while branch/tag rules deal with ref lifecycle and merge requirements. +var branchTagOnlyRules = []string{ + "creation", + "update", + "deletion", + "required_linear_history", + "required_signatures", + "pull_request", + "required_status_checks", + "non_fast_forward", + "commit_message_pattern", + "commit_author_email_pattern", + "committer_email_pattern", + "branch_name_pattern", + "tag_name_pattern", + "required_workflows", + "required_code_scanning", + "required_deployments", + "merge_queue", +} + +// pushOnlyRules contains rules that are only valid for push targets. +// +// These rules apply to push operations and control what content can be pushed +// to repositories. They are not supported for branch or tag rulesets. +// +// To verify/maintain this list: +// 1. Check the GitHub API documentation for organization rulesets: +// https://docs.github.com/en/rest/orgs/rules?apiVersion=2022-11-28#create-an-organization-repository-ruleset +// 2. The API docs don't clearly separate push vs branch/tag rules. To verify, +// attempt to create a branch ruleset via API or UI with each rule type. +// Branch rulesets will reject push-only rules with an error. +// 3. Push rules control file content: paths, sizes, extensions, path lengths. +var pushOnlyRules = []string{ + "file_path_restriction", + "max_file_path_length", + "file_extension_restriction", + "max_file_size", +} + +func validateRulesForTarget(ctx context.Context, d *schema.ResourceDiff) error { + target := d.Get("target").(string) + tflog.Debug(ctx, "Validating rules for target", map[string]any{"target": target}) + + switch target { + case "push": + return validateRulesForPushTarget(ctx, d) + case "branch", "tag": + return validateRulesForBranchTagTarget(ctx, d) + } + + tflog.Debug(ctx, "Rules validation passed", map[string]any{"target": target}) + return nil +} + +func validateRulesForPushTarget(ctx context.Context, d *schema.ResourceDiff) error { + return validateRules(ctx, d, pushOnlyRules) +} + +func validateRulesForBranchTagTarget(ctx context.Context, d *schema.ResourceDiff) error { + return validateRules(ctx, d, branchTagOnlyRules) +} + +func validateRules(ctx context.Context, d *schema.ResourceDiff, allowedRules []string) error { + target := d.Get("target").(string) + rules := d.Get("rules").([]any)[0].(map[string]any) + for ruleName := range rules { + ruleValue, exists := d.GetOk(fmt.Sprintf("rules.0.%s", ruleName)) + if !exists { + continue + } + switch ruleValue := ruleValue.(type) { + case []any: + if len(ruleValue) == 0 { + continue + } + case map[string]any: + if len(ruleValue) == 0 { + continue + } + case any: + if ruleValue == nil { + continue + } + } + if slices.Contains(allowedRules, ruleName) { + continue + } else { + tflog.Debug(ctx, fmt.Sprintf("Invalid rule for %s target", target), map[string]any{"rule": ruleName, "value": ruleValue}) + return fmt.Errorf("rule %q is not valid for %[2]s target; %[2]s targets only support: %v", ruleName, target, allowedRules) + } + } + tflog.Debug(ctx, fmt.Sprintf("Rules validation passed for %s target", target)) + return nil +} + +func validateRepositoryRulesetConditionsFieldForBranchAndTagTargets(ctx context.Context, target string, conditions map[string]any) error { + tflog.Debug(ctx, fmt.Sprintf("Validating conditions field for %s target", target), map[string]any{"target": target, "conditions": conditions}) + + if conditions["ref_name"] == nil || len(conditions["ref_name"].([]any)) == 0 { + tflog.Debug(ctx, fmt.Sprintf("Missing ref_name for %s target", target), map[string]any{"target": target}) + return fmt.Errorf("ref_name must be set for %s target", target) + } + + tflog.Debug(ctx, fmt.Sprintf("Conditions validation passed for %s target", target)) + return nil +} + +func validateConditionsFieldForBranchAndTagTargets(ctx context.Context, target string, conditions map[string]any) error { + tflog.Debug(ctx, fmt.Sprintf("Validating conditions field for %s target", target), map[string]any{"target": target, "conditions": conditions}) + + if conditions["ref_name"] == nil || len(conditions["ref_name"].([]any)) == 0 { + tflog.Debug(ctx, fmt.Sprintf("Missing ref_name for %s target", target), map[string]any{"target": target}) + return fmt.Errorf("ref_name must be set for %s target", target) + } + + if (conditions["repository_name"] == nil || len(conditions["repository_name"].([]any)) == 0) && (conditions["repository_id"] == nil || len(conditions["repository_id"].([]any)) == 0) { + tflog.Debug(ctx, fmt.Sprintf("Missing repository_name or repository_id for %s target", target), map[string]any{"target": target}) + return fmt.Errorf("either repository_name or repository_id must be set for %s target", target) + } + tflog.Debug(ctx, fmt.Sprintf("Conditions validation passed for %s target", target)) + return nil +} + +func validateConditionsFieldForPushTarget(ctx context.Context, conditions map[string]any) error { + tflog.Debug(ctx, "Validating conditions field for push target", map[string]any{"target": "push", "conditions": conditions}) + + if conditions["ref_name"] != nil && len(conditions["ref_name"].([]any)) > 0 { + tflog.Debug(ctx, "Invalid ref_name for push target", map[string]any{"ref_name": conditions["ref_name"]}) + return fmt.Errorf("ref_name must not be set for push target") + } + tflog.Debug(ctx, "Conditions validation passed for push target") + return nil +} diff --git a/github/repository_rules_validation_utils_test.go b/github/repository_rules_validation_utils_test.go new file mode 100644 index 0000000000..91915561fb --- /dev/null +++ b/github/repository_rules_validation_utils_test.go @@ -0,0 +1,225 @@ +package github + +import ( + "testing" +) + +func TestValidateConditionsFieldForPushTarget(t *testing.T) { + tests := []struct { + name string + conditions map[string]any + expectError bool + errorMsg string + }{ + { + name: "valid push target without ref_name", + conditions: map[string]any{ + "repository_name": []any{map[string]any{"include": []any{"~ALL"}, "exclude": []any{}}}, + }, + expectError: false, + }, + { + name: "valid push target with nil ref_name", + conditions: map[string]any{"ref_name": nil}, + expectError: false, + }, + { + name: "valid push target with empty ref_name slice", + conditions: map[string]any{"ref_name": []any{}}, + expectError: false, + }, + { + name: "invalid push target with ref_name set", + conditions: map[string]any{ + "ref_name": []any{map[string]any{"include": []any{"~ALL"}, "exclude": []any{}}}, + }, + expectError: true, + errorMsg: "ref_name must not be set for push target", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateConditionsFieldForPushTarget(t.Context(), tt.conditions) + if tt.expectError { + if err == nil { + t.Errorf("expected error but got nil") + } else if err.Error() != tt.errorMsg { + t.Errorf("expected error %q, got %q", tt.errorMsg, err.Error()) + } + } else { + if err != nil { + t.Errorf("expected no error but got: %v", err) + } + } + }) + } +} + +func TestValidateRepositoryRulesetConditionsFieldForBranchAndTagTargets(t *testing.T) { + tests := []struct { + name string + target string + conditions map[string]any + expectError bool + errorMsg string + }{ + { + name: "valid branch target with ref_name", + target: "branch", + conditions: map[string]any{ + "ref_name": []any{map[string]any{"include": []any{"~DEFAULT_BRANCH"}, "exclude": []any{}}}, + }, + expectError: false, + }, + { + name: "valid tag target with ref_name", + target: "tag", + conditions: map[string]any{ + "ref_name": []any{map[string]any{"include": []any{"v*"}, "exclude": []any{}}}, + }, + expectError: false, + }, + { + name: "invalid branch target without ref_name", + target: "branch", + conditions: map[string]any{}, + expectError: true, + errorMsg: "ref_name must be set for branch target", + }, + { + name: "invalid tag target without ref_name", + target: "tag", + conditions: map[string]any{}, + expectError: true, + errorMsg: "ref_name must be set for tag target", + }, + { + name: "invalid branch target with nil ref_name", + target: "branch", + conditions: map[string]any{"ref_name": nil}, + expectError: true, + errorMsg: "ref_name must be set for branch target", + }, + { + name: "invalid tag target with empty ref_name slice", + target: "tag", + conditions: map[string]any{"ref_name": []any{}}, + expectError: true, + errorMsg: "ref_name must be set for tag target", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateRepositoryRulesetConditionsFieldForBranchAndTagTargets(t.Context(), tt.target, tt.conditions) + if tt.expectError { + if err == nil { + t.Errorf("expected error but got nil") + } else if err.Error() != tt.errorMsg { + t.Errorf("expected error %q, got %q", tt.errorMsg, err.Error()) + } + } else { + if err != nil { + t.Errorf("expected no error but got: %v", err) + } + } + }) + } +} + +func TestValidateConditionsFieldForBranchAndTagTargets(t *testing.T) { + tests := []struct { + name string + target string + conditions map[string]any + expectError bool + errorMsg string + }{ + { + name: "valid branch target with ref_name and repository_name", + target: "branch", + conditions: map[string]any{ + "ref_name": []any{map[string]any{"include": []any{"~DEFAULT_BRANCH"}, "exclude": []any{}}}, + "repository_name": []any{map[string]any{"include": []any{"~ALL"}, "exclude": []any{}}}, + }, + expectError: false, + }, + { + name: "valid tag target with ref_name and repository_id", + target: "tag", + conditions: map[string]any{ + "ref_name": []any{map[string]any{"include": []any{"v*"}, "exclude": []any{}}}, + "repository_id": []any{123, 456}, + }, + expectError: false, + }, + { + name: "invalid branch target without ref_name", + target: "branch", + conditions: map[string]any{ + "repository_name": []any{map[string]any{"include": []any{"~ALL"}, "exclude": []any{}}}, + }, + expectError: true, + errorMsg: "ref_name must be set for branch target", + }, + { + name: "invalid branch target without repository_name or repository_id", + target: "branch", + conditions: map[string]any{ + "ref_name": []any{map[string]any{"include": []any{"~DEFAULT_BRANCH"}, "exclude": []any{}}}, + }, + expectError: true, + errorMsg: "either repository_name or repository_id must be set for branch target", + }, + { + name: "invalid tag target with nil repository_name and repository_id", + target: "tag", + conditions: map[string]any{ + "ref_name": []any{map[string]any{"include": []any{"v*"}, "exclude": []any{}}}, + "repository_name": nil, + "repository_id": nil, + }, + expectError: true, + errorMsg: "either repository_name or repository_id must be set for tag target", + }, + { + name: "invalid branch target with empty repository_name and repository_id slices", + target: "branch", + conditions: map[string]any{ + "ref_name": []any{map[string]any{"include": []any{"~DEFAULT_BRANCH"}, "exclude": []any{}}}, + "repository_name": []any{}, + "repository_id": []any{}, + }, + expectError: true, + errorMsg: "either repository_name or repository_id must be set for branch target", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := validateConditionsFieldForBranchAndTagTargets(t.Context(), tt.target, tt.conditions) + if tt.expectError { + if err == nil { + t.Errorf("expected error but got nil") + } else if err.Error() != tt.errorMsg { + t.Errorf("expected error %q, got %q", tt.errorMsg, err.Error()) + } + } else { + if err != nil { + t.Errorf("expected no error but got: %v", err) + } + } + }) + } +} + +func TestRuleListsDoNotOverlap(t *testing.T) { + for _, pushRule := range pushOnlyRules { + for _, branchTagRule := range branchTagOnlyRules { + if pushRule == branchTagRule { + t.Errorf("rule %q appears in both pushOnlyRules and branchTagOnlyRules", pushRule) + } + } + } +} diff --git a/github/resource_github_organization_ruleset.go b/github/resource_github_organization_ruleset.go index c0a027dc67..75ed710db4 100644 --- a/github/resource_github_organization_ruleset.go +++ b/github/resource_github_organization_ruleset.go @@ -6,25 +6,34 @@ import ( "fmt" "log" "net/http" + "regexp" "strconv" "github.com/google/go-github/v77/github" + "github.com/hashicorp/terraform-plugin-log/tflog" + "github.com/hashicorp/terraform-plugin-sdk/v2/diag" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" ) func resourceGithubOrganizationRuleset() *schema.Resource { return &schema.Resource{ - Create: resourceGithubOrganizationRulesetCreate, - Read: resourceGithubOrganizationRulesetRead, - Update: resourceGithubOrganizationRulesetUpdate, - Delete: resourceGithubOrganizationRulesetDelete, + CreateContext: resourceGithubOrganizationRulesetCreate, + ReadContext: resourceGithubOrganizationRulesetRead, + UpdateContext: resourceGithubOrganizationRulesetUpdate, + DeleteContext: resourceGithubOrganizationRulesetDelete, Importer: &schema.ResourceImporter{ - State: resourceGithubOrganizationRulesetImport, + StateContext: resourceGithubOrganizationRulesetImport, }, SchemaVersion: 1, + CustomizeDiff: customdiff.All( + validateConditionsFieldBasedOnTarget, + validateOrganizationRulesetRules, + ), + Schema: map[string]*schema.Schema{ "name": { Type: schema.TypeString, @@ -36,16 +45,16 @@ func resourceGithubOrganizationRuleset() *schema.Resource { Type: schema.TypeString, Required: true, ValidateFunc: validation.StringInSlice([]string{"branch", "tag", "push"}, false), - Description: "Possible values are `branch`, `tag` and `push`. Note: The `push` target is in beta and is subject to change.", + Description: "The target of the ruleset. Possible values are `branch`, `tag`, and `push`.", }, "enforcement": { Type: schema.TypeString, Required: true, ValidateFunc: validation.StringInSlice([]string{"disabled", "active", "evaluate"}, false), - Description: "Possible values for Enforcement are `disabled`, `active`, `evaluate`. Note: `evaluate` is currently only supported for owners of type `organization`.", + Description: "The enforcement level of the ruleset. `evaluate` allows admins to test rules before enforcing them. Possible values are `disabled`, `active`, and `evaluate`. Note: `evaluate` is only available for Enterprise plans.", }, "bypass_actors": { - Type: schema.TypeList, + Type: schema.TypeList, // TODO: These are returned from GH API sorted by actor_id, we might want to investigate if we want to include sorting Optional: true, DiffSuppressFunc: bypassActorsDiffSuppressFunc, Description: "The actors that can bypass the rules in this ruleset.", @@ -61,7 +70,7 @@ func resourceGithubOrganizationRuleset() *schema.Resource { Type: schema.TypeString, Required: true, ValidateFunc: validation.StringInSlice([]string{"Integration", "OrganizationAdmin", "RepositoryRole", "Team", "DeployKey"}, false), - Description: "The type of actor that can bypass a ruleset. See https://docs.github.com/en/rest/orgs/rules for more information", + Description: "The type of actor that can bypass a ruleset. Can be one of: `Integration`, `OrganizationAdmin`, `RepositoryRole`, `Team`, or `DeployKey`.", }, "bypass_mode": { Type: schema.TypeString, @@ -86,13 +95,14 @@ func resourceGithubOrganizationRuleset() *schema.Resource { Type: schema.TypeList, Optional: true, MaxItems: 1, - Description: "Parameters for an organization ruleset condition. `ref_name` is required alongside one of `repository_name` or `repository_id`.", + Description: "Parameters for an organization ruleset condition. `ref_name` is required for `branch` and `tag` targets, but must not be set for `push` targets. One of `repository_name` or `repository_id` is always required.", Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "ref_name": { - Type: schema.TypeList, - Required: true, - MaxItems: 1, + Type: schema.TypeList, + Optional: true, + MaxItems: 1, + Description: "Targets refs that match the specified patterns. Required for `branch` and `tag` targets.", Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "include": { @@ -118,6 +128,7 @@ func resourceGithubOrganizationRuleset() *schema.Resource { Type: schema.TypeList, Optional: true, MaxItems: 1, + Description: "Targets repositories that match the specified name patterns.", ExactlyOneOf: []string{"conditions.0.repository_id"}, AtLeastOneOf: []string{"conditions.0.repository_id"}, Elem: &schema.Resource{ @@ -197,6 +208,16 @@ func resourceGithubOrganizationRuleset() *schema.Resource { Description: "Require all commits be made to a non-target branch and submitted via a pull request before they can be merged.", Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ + "allowed_merge_methods": { + Type: schema.TypeList, + Required: true, + MinItems: 1, + Description: "Array of allowed merge methods. Allowed values include `merge`, `squash`, and `rebase`. At least one option must be enabled.", + Elem: &schema.Schema{ + Type: schema.TypeString, + ValidateDiagFunc: toDiagFunc(validation.StringInSlice([]string{"merge", "squash", "rebase"}, false), "allowed_merge_methods"), + }, + }, "dismiss_stale_reviews_on_push": { Type: schema.TypeBool, Optional: true, @@ -245,9 +266,10 @@ func resourceGithubOrganizationRuleset() *schema.Resource { Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ "context": { - Type: schema.TypeString, - Required: true, - Description: "The status check context name that must be present on the commit.", + Type: schema.TypeString, + Required: true, + ValidateDiagFunc: validation.ToDiagFunc(validation.StringIsNotEmpty), + Description: "The status check context name that must be present on the commit.", }, "integration_id": { Type: schema.TypeInt, @@ -275,7 +297,7 @@ func resourceGithubOrganizationRuleset() *schema.Resource { "non_fast_forward": { Type: schema.TypeBool, Optional: true, - Description: "Prevent users with push access from force pushing to branches.", + Description: "Prevent users with push access from force pushing to refs.", }, "commit_message_pattern": { Type: schema.TypeList, @@ -454,9 +476,10 @@ func resourceGithubOrganizationRuleset() *schema.Resource { Description: "The repository in which the workflow is defined.", }, "path": { - Type: schema.TypeString, - Required: true, - Description: "The path to the workflow YAML definition file.", + Type: schema.TypeString, + Required: true, + ValidateDiagFunc: toDiagFunc(validation.StringMatch(regexp.MustCompile(`^\.github\/workflows\/.*$`), "Path must be in the .github/workflows directory"), "path"), + Description: "The path to the workflow YAML definition file.", }, "ref": { Type: schema.TypeString, @@ -578,43 +601,43 @@ func resourceGithubOrganizationRuleset() *schema.Resource { }, }, "etag": { - Type: schema.TypeString, - Computed: true, + Type: schema.TypeString, + Computed: true, + Description: "An etag representing the ruleset for caching purposes.", }, }, } } -func resourceGithubOrganizationRulesetCreate(d *schema.ResourceData, meta any) error { +func resourceGithubOrganizationRulesetCreate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { client := meta.(*Owner).v3client rulesetReq := resourceGithubRulesetObject(d, meta.(*Owner).name) org := meta.(*Owner).name - ctx := context.Background() var ruleset *github.RepositoryRuleset var err error ruleset, _, err = client.Organizations.CreateRepositoryRuleset(ctx, org, *rulesetReq) if err != nil { - return err + return diag.FromErr(err) } d.SetId(strconv.FormatInt(*ruleset.ID, 10)) - return resourceGithubOrganizationRulesetRead(d, meta) + return resourceGithubOrganizationRulesetRead(ctx, d, meta) } -func resourceGithubOrganizationRulesetRead(d *schema.ResourceData, meta any) error { +func resourceGithubOrganizationRulesetRead(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { client := meta.(*Owner).v3client org := meta.(*Owner).name rulesetID, err := strconv.ParseInt(d.Id(), 10, 64) if err != nil { - return unconvertibleIdErr(d.Id(), err) + return diag.FromErr(unconvertibleIdErr(d.Id(), err)) } - ctx := context.WithValue(context.Background(), ctxId, rulesetID) + ctx = context.WithValue(ctx, ctxId, rulesetID) if !d.IsNewResource() { ctx = context.WithValue(ctx, ctxEtag, d.Get("etag").(string)) } @@ -636,7 +659,7 @@ func resourceGithubOrganizationRulesetRead(d *schema.ResourceData, meta any) err return nil } } - return err + return diag.FromErr(err) } if ruleset == nil { @@ -651,7 +674,7 @@ func resourceGithubOrganizationRulesetRead(d *schema.ResourceData, meta any) err _ = d.Set("target", ruleset.GetTarget()) _ = d.Set("enforcement", ruleset.Enforcement) _ = d.Set("bypass_actors", flattenBypassActors(ruleset.BypassActors)) - _ = d.Set("conditions", flattenConditions(ruleset.GetConditions(), true)) + _ = d.Set("conditions", flattenConditionsWithContext(ctx, ruleset.GetConditions(), true)) _ = d.Set("rules", flattenRules(ruleset.Rules, true)) _ = d.Set("node_id", ruleset.GetNodeID()) _ = d.Set("ruleset_id", ruleset.ID) @@ -659,7 +682,7 @@ func resourceGithubOrganizationRulesetRead(d *schema.ResourceData, meta any) err return nil } -func resourceGithubOrganizationRulesetUpdate(d *schema.ResourceData, meta any) error { +func resourceGithubOrganizationRulesetUpdate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { client := meta.(*Owner).v3client rulesetReq := resourceGithubRulesetObject(d, meta.(*Owner).name) @@ -667,38 +690,46 @@ func resourceGithubOrganizationRulesetUpdate(d *schema.ResourceData, meta any) e org := meta.(*Owner).name rulesetID, err := strconv.ParseInt(d.Id(), 10, 64) if err != nil { - return unconvertibleIdErr(d.Id(), err) + return diag.FromErr(unconvertibleIdErr(d.Id(), err)) } - ctx := context.WithValue(context.Background(), ctxId, rulesetID) + ctx = context.WithValue(ctx, ctxId, rulesetID) var ruleset *github.RepositoryRuleset + if d.HasChange("bypass_actors") && len(rulesetReq.BypassActors) == 0 { + // Clear bypass actors first, then update with new ruleset + _, err = client.Organizations.UpdateRepositoryRulesetClearBypassActor(ctx, org, rulesetID) + if err != nil { + return diag.FromErr(err) + } + } + ruleset, _, err = client.Organizations.UpdateRepositoryRuleset(ctx, org, rulesetID, *rulesetReq) if err != nil { - return err + return diag.FromErr(err) } d.SetId(strconv.FormatInt(*ruleset.ID, 10)) - return resourceGithubOrganizationRulesetRead(d, meta) + return resourceGithubOrganizationRulesetRead(ctx, d, meta) } -func resourceGithubOrganizationRulesetDelete(d *schema.ResourceData, meta any) error { +func resourceGithubOrganizationRulesetDelete(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { client := meta.(*Owner).v3client org := meta.(*Owner).name rulesetID, err := strconv.ParseInt(d.Id(), 10, 64) if err != nil { - return unconvertibleIdErr(d.Id(), err) + return diag.FromErr(unconvertibleIdErr(d.Id(), err)) } - ctx := context.WithValue(context.Background(), ctxId, rulesetID) + ctx = context.WithValue(ctx, ctxId, rulesetID) log.Printf("[DEBUG] Deleting organization ruleset: %s: %d", org, rulesetID) _, err = client.Organizations.DeleteRepositoryRuleset(ctx, org, rulesetID) - return err + return diag.FromErr(err) } -func resourceGithubOrganizationRulesetImport(d *schema.ResourceData, meta any) ([]*schema.ResourceData, error) { +func resourceGithubOrganizationRulesetImport(ctx context.Context, d *schema.ResourceData, meta any) ([]*schema.ResourceData, error) { rulesetID, err := strconv.ParseInt(d.Id(), 10, 64) if err != nil { return []*schema.ResourceData{d}, unconvertibleIdErr(d.Id(), err) @@ -710,7 +741,6 @@ func resourceGithubOrganizationRulesetImport(d *schema.ResourceData, meta any) ( client := meta.(*Owner).v3client org := meta.(*Owner).name - ctx := context.Background() ruleset, _, err := client.Organizations.GetRepositoryRuleset(ctx, org, rulesetID) if ruleset == nil || err != nil { @@ -720,3 +750,37 @@ func resourceGithubOrganizationRulesetImport(d *schema.ResourceData, meta any) ( return []*schema.ResourceData{d}, nil } + +func validateConditionsFieldBasedOnTarget(ctx context.Context, d *schema.ResourceDiff, meta any) error { + target := d.Get("target").(string) + tflog.Debug(ctx, "Validating conditions field based on target", map[string]any{"target": target}) + conditionsRaw := d.Get("conditions").([]any) + + if len(conditionsRaw) == 0 { + tflog.Debug(ctx, "An empty conditions block, skipping validation.", map[string]any{"target": target}) + return nil + } + + conditions := conditionsRaw[0].(map[string]any) + + switch target { + case "branch", "tag": + return validateConditionsFieldForBranchAndTagTargets(ctx, target, conditions) + case "push": + return validateConditionsFieldForPushTarget(ctx, conditions) + } + return nil +} + +func validateOrganizationRulesetRules(ctx context.Context, d *schema.ResourceDiff, _ any) error { + target := d.Get("target").(string) + tflog.Debug(ctx, "Validating organization ruleset rules based on target", map[string]any{"target": target}) + + rulesRaw := d.Get("rules").([]any) + if len(rulesRaw) == 0 { + tflog.Debug(ctx, "No rules block, skipping validation") + return nil + } + + return validateRulesForTarget(ctx, d) +} diff --git a/github/resource_github_organization_ruleset_test.go b/github/resource_github_organization_ruleset_test.go index 68e432e688..932bccb09a 100644 --- a/github/resource_github_organization_ruleset_test.go +++ b/github/resource_github_organization_ruleset_test.go @@ -2,6 +2,7 @@ package github import ( "fmt" + "regexp" "strings" "testing" @@ -11,20 +12,37 @@ import ( ) func TestGithubOrganizationRulesets(t *testing.T) { - if isEnterprise != "true" { - t.Skip("Skipping because `ENTERPRISE_ACCOUNT` is not set or set to false") - } - - if testEnterprise == "" { - t.Skip("Skipping because `ENTERPRISE_SLUG` is not set") - } - randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) t.Run("Creates and updates organization rulesets without errors", func(t *testing.T) { + resourceName := "test-create-and-update" config := fmt.Sprintf(` - resource "github_organization_ruleset" "test" { - name = "test-%s" + resource "github_repository" "%[1]s" { + name = "test-%[2]s" + visibility = "private" + auto_init = true + + ignore_vulnerability_alerts_during_read = true + + } + + resource "github_repository_file" "%[1]s" { + repository = github_repository.%[1]s.name + branch = "main" + file = ".github/workflows/echo.yaml" + content = "name: Echo Workflow\n\non: [pull_request]\n\njobs:\n echo:\n runs-on: linux\n steps:\n - run: echo \"Hello, world!\"\n" + commit_message = "Managed by Terraform" + commit_author = "Terraform User" + commit_email = "terraform@example.com" + } + + resource "github_actions_repository_access_level" "%[1]s" { + repository = github_repository.%[1]s.name + access_level = "organization" + } + + resource "github_organization_ruleset" "%[1]s" { + name = "test-%[2]s" target = "branch" enforcement = "active" @@ -33,6 +51,10 @@ func TestGithubOrganizationRulesets(t *testing.T) { include = ["~ALL"] exclude = [] } + repository_name { + include = ["~ALL"] + exclude = [] + } } rules { @@ -46,6 +68,7 @@ func TestGithubOrganizationRulesets(t *testing.T) { required_signatures = false pull_request { + allowed_merge_methods = ["merge", "squash", "rebase"] required_approving_review_count = 2 required_review_thread_resolution = true require_code_owner_review = true @@ -66,17 +89,18 @@ func TestGithubOrganizationRulesets(t *testing.T) { required_workflows { do_not_enforce_on_create = true required_workflow { - path = "path/to/workflow.yaml" - repository_id = 1234 + path = ".github/workflows/echo.yaml" + repository_id = github_repository.%[1]s.repo_id + ref = "main" # Default ref is master } } required_code_scanning { - required_code_scanning_tool { - alerts_threshold = "errors" - security_alerts_threshold = "high_or_higher" - tool = "CodeQL" - } + required_code_scanning_tool { + alerts_threshold = "errors" + security_alerts_threshold = "high_or_higher" + tool = "CodeQL" + } } branch_name_pattern { @@ -88,47 +112,43 @@ func TestGithubOrganizationRulesets(t *testing.T) { non_fast_forward = true } + depends_on = [github_repository_file.%[1]s] } - `, randomID) + `, resourceName, randomID) check := resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr( - "github_organization_ruleset.test", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "name", - "test", + fmt.Sprintf("test-%s", randomID), ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "enforcement", "active", ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "rules.0.required_workflows.0.do_not_enforce_on_create", "true", ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "rules.0.required_workflows.0.required_workflow.0.path", - "path/to/workflow.yaml", - ), - resource.TestCheckResourceAttr( - "github_organization_ruleset.test", - "rules.0.required_workflows.0.required_workflow.0.repository_id", - "1234", + ".github/workflows/echo.yaml", ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "rules.0.required_code_scanning.0.required_code_scanning_tool.0.alerts_threshold", "errors", ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "rules.0.required_code_scanning.0.required_code_scanning_tool.0.security_alerts_threshold", "high_or_higher", ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "rules.0.required_code_scanning.0.required_code_scanning_tool.0.tool", "CodeQL", ), @@ -210,8 +230,9 @@ func TestGithubOrganizationRulesets(t *testing.T) { }) t.Run("Imports rulesets without error", func(t *testing.T) { + resourceName := "test-import-rulesets" config := fmt.Sprintf(` - resource "github_organization_ruleset" "test" { + resource "github_organization_ruleset" "%s" { name = "test-%s" target = "branch" enforcement = "active" @@ -221,6 +242,10 @@ func TestGithubOrganizationRulesets(t *testing.T) { include = ["~ALL"] exclude = [] } + repository_name { + include = ["~ALL"] + exclude = [] + } } rules { @@ -234,6 +259,7 @@ func TestGithubOrganizationRulesets(t *testing.T) { required_signatures = false pull_request { + allowed_merge_methods = ["merge", "squash", "rebase"] required_approving_review_count = 2 required_review_thread_resolution = true require_code_owner_review = true @@ -261,10 +287,10 @@ func TestGithubOrganizationRulesets(t *testing.T) { non_fast_forward = true } } - `, randomID) + `, resourceName, randomID) check := resource.ComposeTestCheckFunc( - resource.TestCheckResourceAttrSet("github_organization_ruleset.test", "name"), + resource.TestCheckResourceAttrSet(fmt.Sprintf("github_organization_ruleset.%s", resourceName), "name"), ) testCase := func(t *testing.T, mode string) { @@ -277,7 +303,7 @@ func TestGithubOrganizationRulesets(t *testing.T) { Check: check, }, { - ResourceName: "github_organization_ruleset.test", + ResourceName: fmt.Sprintf("github_organization_ruleset.%s", resourceName), ImportState: true, ImportStateVerify: true, }, @@ -291,8 +317,9 @@ func TestGithubOrganizationRulesets(t *testing.T) { }) t.Run("Creates and updates organization using bypasses", func(t *testing.T) { + resourceName := "test-creates-and-updates-using-bypasses" config := fmt.Sprintf(` - resource "github_organization_ruleset" "test" { + resource "github_organization_ruleset" "%s" { name = "test-%s" target = "branch" enforcement = "active" @@ -319,6 +346,10 @@ func TestGithubOrganizationRulesets(t *testing.T) { include = ["~ALL"] exclude = [] } + repository_name { + include = ["~ALL"] + exclude = [] + } } rules { @@ -328,6 +359,7 @@ func TestGithubOrganizationRulesets(t *testing.T) { required_linear_history = true required_signatures = false pull_request { + allowed_merge_methods = ["merge", "squash", "rebase"] required_approving_review_count = 2 required_review_thread_resolution = true require_code_owner_review = true @@ -336,43 +368,43 @@ func TestGithubOrganizationRulesets(t *testing.T) { } } } - `, randomID) + `, resourceName, randomID) check := resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr( - "github_organization_ruleset.test", "bypass_actors.#", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "bypass_actors.#", "3", ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", "bypass_actors.0.actor_type", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "bypass_actors.0.actor_type", "DeployKey", ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", "bypass_actors.0.bypass_mode", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "bypass_actors.0.bypass_mode", "always", ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", "bypass_actors.1.actor_id", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "bypass_actors.2.actor_id", "5", ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", "bypass_actors.1.actor_type", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "bypass_actors.2.actor_type", "RepositoryRole", ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", "bypass_actors.1.bypass_mode", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "bypass_actors.2.bypass_mode", "always", ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", "bypass_actors.2.actor_id", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "bypass_actors.1.actor_id", "1", ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", "bypass_actors.2.actor_type", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "bypass_actors.1.actor_type", "OrganizationAdmin", ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", "bypass_actors.2.bypass_mode", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "bypass_actors.1.bypass_mode", "always", ), ) @@ -396,8 +428,9 @@ func TestGithubOrganizationRulesets(t *testing.T) { }) t.Run("Creates organization ruleset with all bypass_modes", func(t *testing.T) { + resourceName := "test-create-with-bypass-modes" config := fmt.Sprintf(` - resource "github_organization_ruleset" "test" { + resource "github_organization_ruleset" "%s" { name = "test-bypass-modes-%s" target = "branch" enforcement = "active" @@ -425,53 +458,57 @@ func TestGithubOrganizationRulesets(t *testing.T) { include = ["~ALL"] exclude = [] } + repository_name { + include = ["~ALL"] + exclude = [] + } } rules { creation = true } } - `, randomID) + `, resourceName, randomID) check := resource.ComposeTestCheckFunc( resource.TestCheckResourceAttr( - "github_organization_ruleset.test", "bypass_actors.#", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "bypass_actors.#", "3", ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", "bypass_actors.0.actor_id", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "bypass_actors.0.actor_id", "1", ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", "bypass_actors.0.actor_type", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "bypass_actors.0.actor_type", "OrganizationAdmin", ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", "bypass_actors.0.bypass_mode", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "bypass_actors.0.bypass_mode", "always", ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", "bypass_actors.1.actor_id", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "bypass_actors.2.actor_id", "5", ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", "bypass_actors.1.actor_type", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "bypass_actors.2.actor_type", "RepositoryRole", ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", "bypass_actors.1.bypass_mode", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "bypass_actors.2.bypass_mode", "pull_request", ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", "bypass_actors.2.actor_id", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "bypass_actors.1.actor_id", "2", ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", "bypass_actors.2.actor_type", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "bypass_actors.1.actor_type", "RepositoryRole", ), resource.TestCheckResourceAttr( - "github_organization_ruleset.test", "bypass_actors.2.bypass_mode", + fmt.Sprintf("github_organization_ruleset.%s", resourceName), "bypass_actors.1.bypass_mode", "exempt", ), ) @@ -512,6 +549,10 @@ func TestGithubOrganizationRulesets(t *testing.T) { include = ["~ALL"] exclude = [] } + repository_name { + include = ["~ALL"] + exclude = [] + } } rules { @@ -563,6 +604,405 @@ func TestGithubOrganizationRulesets(t *testing.T) { testCase(t, enterprise) }) }) + + t.Run("Validates branch target requires `ref_name` condition", func(t *testing.T) { + config := fmt.Sprintf(` + resource "github_organization_ruleset" "test" { + name = "test-validation-%s" + target = "branch" + enforcement = "active" + + conditions { + repository_name { + include = ["~ALL"] + exclude = [] + } + } + + rules { + creation = true + } + } + `, randomID) + + testCase := func(t *testing.T, mode string) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnlessMode(t, mode) }, + Providers: testAccProviders, + Steps: []resource.TestStep{ + { + Config: config, + ExpectError: regexp.MustCompile("ref_name must be set for branch target"), + }, + }, + }) + } + + t.Run("with an enterprise account", func(t *testing.T) { + testCase(t, enterprise) + }) + + t.Run("with an organization account", func(t *testing.T) { + t.Skip("organization account not supported for this operation, since it needs a paid Team plan.") + }) + }) + + t.Run("Validates tag target requires `ref_name` condition", func(t *testing.T) { + config := fmt.Sprintf(` + resource "github_organization_ruleset" "test" { + name = "test-tag-no-conditions-%s" + target = "tag" + enforcement = "active" + + conditions { + repository_name { + include = ["~ALL"] + exclude = [] + } + } + + rules { + creation = true + } + } + `, randomID) + + testCase := func(t *testing.T, mode string) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnlessMode(t, mode) }, + Providers: testAccProviders, + Steps: []resource.TestStep{ + { + Config: config, + ExpectError: regexp.MustCompile("ref_name must be set for tag target"), + }, + }, + }) + } + + t.Run("with an enterprise account", func(t *testing.T) { + testCase(t, enterprise) + }) + + t.Run("with an organization account", func(t *testing.T) { + t.Skip("organization account not supported for this operation, since it needs a paid Team plan.") + }) + }) + + t.Run("Validates push target rejects ref_name", func(t *testing.T) { + resourceName := "test-push-reject-ref-name" + config := fmt.Sprintf(` + resource "github_organization_ruleset" "%s" { + name = "test-push-with-ref-%s" + target = "push" + enforcement = "active" + + conditions { + ref_name { + include = ["~ALL"] + exclude = [] + } + repository_name { + include = ["~ALL"] + exclude = [] + } + } + + rules { + # Push rulesets only support push-specific rules + max_file_size { + max_file_size = 100 + } + } + } + `, resourceName, randomID) + + testCase := func(t *testing.T, mode string) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnlessMode(t, mode) }, + Providers: testAccProviders, + Steps: []resource.TestStep{ + { + Config: config, + ExpectError: regexp.MustCompile("ref_name must not be set for push target"), + }, + }, + }) + } + + t.Run("with an enterprise account", func(t *testing.T) { + testCase(t, enterprise) + }) + + t.Run("with an organization account", func(t *testing.T) { + t.Skip("organization account not supported for this operation, since it needs a paid Team plan.") + }) + }) + + t.Run("Validates push target rejects branch/tag rules", func(t *testing.T) { + resourceName := "test-push-reject-branch-rules" + config := fmt.Sprintf(` + resource "github_organization_ruleset" "%s" { + name = "test-push-branch-rule-%s" + target = "push" + enforcement = "active" + + conditions { + repository_name { + include = ["~ALL"] + exclude = [] + } + } + + rules { + # 'creation' is a branch/tag rule, not valid for push target + creation = true + } + } + `, resourceName, randomID) + + testCase := func(t *testing.T, mode string) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnlessMode(t, mode) }, + Providers: testAccProviders, + Steps: []resource.TestStep{ + { + Config: config, + ExpectError: regexp.MustCompile("rule .* is not valid for push target"), + }, + }, + }) + } + + t.Run("with an enterprise account", func(t *testing.T) { + testCase(t, enterprise) + }) + + t.Run("with an organization account", func(t *testing.T) { + t.Skip("organization account not supported for this operation, since it needs a paid Team plan.") + }) + }) + + t.Run("Validates branch target rejects push-only rules", func(t *testing.T) { + resourceName := "test-branch-reject-push-rules" + config := fmt.Sprintf(` + resource "github_organization_ruleset" "%s" { + name = "test-branch-push-rule-%s" + target = "branch" + enforcement = "active" + + conditions { + ref_name { + include = ["~ALL"] + exclude = [] + } + repository_name { + include = ["~ALL"] + exclude = [] + } + } + + rules { + # 'max_file_size' is a push-only rule, not valid for branch target + max_file_size { + max_file_size = 100 + } + } + } + `, resourceName, randomID) + + testCase := func(t *testing.T, mode string) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnlessMode(t, mode) }, + Providers: testAccProviders, + Steps: []resource.TestStep{ + { + Config: config, + ExpectError: regexp.MustCompile("rule .* is not valid for branch target"), + }, + }, + }) + } + + t.Run("with an enterprise account", func(t *testing.T) { + testCase(t, enterprise) + }) + + t.Run("with an organization account", func(t *testing.T) { + t.Skip("organization account not supported for this operation, since it needs a paid Team plan.") + }) + }) + + t.Run("Creates push ruleset with repository_name only", func(t *testing.T) { + resourceName := "test-push-repo-name-only" + config := fmt.Sprintf(` + resource "github_organization_ruleset" "%s" { + name = "test-push-%s" + target = "push" + enforcement = "active" + + conditions { + repository_name { + include = ["~ALL"] + exclude = [] + } + } + + rules { + # Push rulesets only support push-specific rules: + # file_path_restriction, max_file_path_length, file_extension_restriction, max_file_size + max_file_size { + max_file_size = 100 + } + } + } + `, resourceName, randomID) + + check := resource.ComposeTestCheckFunc( + resource.TestCheckResourceAttr( + fmt.Sprintf("github_organization_ruleset.%s", resourceName), + "name", + fmt.Sprintf("test-push-%s", randomID), + ), + resource.TestCheckResourceAttr( + fmt.Sprintf("github_organization_ruleset.%s", resourceName), + "target", + "push", + ), + resource.TestCheckResourceAttr( + fmt.Sprintf("github_organization_ruleset.%s", resourceName), + "enforcement", + "active", + ), + resource.TestCheckResourceAttr( + fmt.Sprintf("github_organization_ruleset.%s", resourceName), + "rules.0.max_file_size.0.max_file_size", + "100", + ), + ) + + testCase := func(t *testing.T, mode string) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnlessMode(t, mode) }, + Providers: testAccProviders, + Steps: []resource.TestStep{ + { + Config: config, + Check: check, + }, + }, + }) + } + + t.Run("with an enterprise account", func(t *testing.T) { + testCase(t, enterprise) + }) + + t.Run("with an organization account", func(t *testing.T) { + t.Skip("organization account not supported for this operation, since it needs a paid Team plan.") + }) + }) + + t.Run("Validates rules.required_status_checks", func(t *testing.T) { + t.Run("required_check.context should not be empty", func(t *testing.T) { + resourceName := "test-required-status-checks-context-is-not-empty" + config := fmt.Sprintf(` + resource "github_organization_ruleset" "%s" { + name = "test-context-is-not-empty-%s" + target = "branch" + enforcement = "active" + + conditions { + ref_name { + include = ["~ALL"] + exclude = [] + } + repository_name { + include = ["~ALL"] + exclude = [] + } + } + + rules { + required_status_checks { + required_check { + context = "" + } + } + } + } + `, resourceName, randomID) + + testCase := func(t *testing.T, mode string) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnlessMode(t, mode) }, + Providers: testAccProviders, + Steps: []resource.TestStep{ + { + Config: config, + ExpectError: regexp.MustCompile("expected \"context\" to not be an empty string"), + }, + }, + }) + } + + t.Run("with an enterprise account", func(t *testing.T) { + testCase(t, enterprise) + }) + + t.Run("with an organization account", func(t *testing.T) { + t.Skip("organization account not supported for this operation, since it needs a paid Team plan.") + }) + }) + t.Run("required_check should be required when strict_required_status_checks_policy is set", func(t *testing.T) { + resourceName := "test-required-check-is-required" + config := fmt.Sprintf(` + resource "github_organization_ruleset" "%s" { + name = "test-required-with-%s" + target = "branch" + enforcement = "active" + + conditions { + ref_name { + include = ["~ALL"] + exclude = [] + } + repository_name { + include = ["~ALL"] + exclude = [] + } + } + + rules { + required_status_checks { + strict_required_status_checks_policy = true + } + } + } + `, resourceName, randomID) + + testCase := func(t *testing.T, mode string) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnlessMode(t, mode) }, + Providers: testAccProviders, + Steps: []resource.TestStep{ + { + Config: config, + ExpectError: regexp.MustCompile("Insufficient required_check blocks"), + }, + }, + }) + } + + t.Run("with an enterprise account", func(t *testing.T) { + testCase(t, enterprise) + }) + + t.Run("with an organization account", func(t *testing.T) { + t.Skip("organization account not supported for this operation, since it needs a paid Team plan.") + }) + }) + }) } func TestOrganizationPushRulesetSupport(t *testing.T) { diff --git a/github/resource_github_repository.go b/github/resource_github_repository.go index 8673877aea..88c3725f28 100644 --- a/github/resource_github_repository.go +++ b/github/resource_github_repository.go @@ -10,18 +10,19 @@ import ( "strings" "github.com/google/go-github/v77/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 resourceGithubRepository() *schema.Resource { return &schema.Resource{ - Create: resourceGithubRepositoryCreate, - Read: resourceGithubRepositoryRead, - Update: resourceGithubRepositoryUpdate, - Delete: resourceGithubRepositoryDelete, + CreateContext: resourceGithubRepositoryCreate, + ReadContext: resourceGithubRepositoryRead, + UpdateContext: resourceGithubRepositoryUpdate, + DeleteContext: resourceGithubRepositoryDelete, Importer: &schema.ResourceImporter{ - State: func(d *schema.ResourceData, meta any) ([]*schema.ResourceData, error) { + StateContext: func(ctx context.Context, d *schema.ResourceData, meta any) ([]*schema.ResourceData, error) { if err := d.Set("auto_init", false); err != nil { return nil, err } @@ -595,18 +596,17 @@ func resourceGithubRepositoryObject(d *schema.ResourceData) *github.Repository { return repository } -func resourceGithubRepositoryCreate(d *schema.ResourceData, meta any) error { +func resourceGithubRepositoryCreate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { client := meta.(*Owner).v3client if branchName, hasDefaultBranch := d.GetOk("default_branch"); hasDefaultBranch && (branchName != "main") { - return fmt.Errorf("cannot set the default branch on a new repository to something other than 'main'") + return diag.FromErr(fmt.Errorf("cannot set the default branch on a new repository to something other than 'main'")) } repoReq := resourceGithubRepositoryObject(d) owner := meta.(*Owner).name repoName := repoReq.GetName() - ctx := context.Background() // determine if repository should be private. assume public to start isPrivate := false @@ -632,7 +632,7 @@ func resourceGithubRepositoryCreate(d *schema.ResourceData, meta any) error { for _, templateConfigBlock := range templateConfigBlocks { templateConfigMap, ok := templateConfigBlock.(map[string]any) if !ok { - return errors.New("failed to unpack template configuration block") + return diag.FromErr(errors.New("failed to unpack template configuration block")) } templateRepo := templateConfigMap["repository"].(string) @@ -653,7 +653,7 @@ func resourceGithubRepositoryCreate(d *schema.ResourceData, meta any) error { &templateRepoReq, ) if err != nil { - return err + return diag.FromErr(err) } d.SetId(*repo.Name) @@ -667,7 +667,7 @@ func resourceGithubRepositoryCreate(d *schema.ResourceData, meta any) error { log.Printf("[INFO] Creating fork of %s/%s in %s", sourceOwner, sourceRepo, owner) if sourceOwner == "" || sourceRepo == "" { - return fmt.Errorf("source_owner and source_repo must be provided when forking a repository") + return diag.FromErr(fmt.Errorf("source_owner and source_repo must be provided when forking a repository")) } // Create the fork using the GitHub client library @@ -688,18 +688,18 @@ func resourceGithubRepositoryCreate(d *schema.ResourceData, meta any) error { log.Printf("[INFO] Fork is being created asynchronously") // Despite the 202 status, the API should still return preliminary fork information if fork == nil { - return fmt.Errorf("fork information not available after accepted status") + return diag.FromErr(fmt.Errorf("fork information not available after accepted status")) } log.Printf("[DEBUG] Fork name: %s", fork.GetName()) } else { - return fmt.Errorf("failed to create fork: %w", err) + return diag.FromErr(fmt.Errorf("failed to create fork: %w", err)) } } else if resp != nil { log.Printf("[DEBUG] Fork response status: %d", resp.StatusCode) } if fork == nil { - return fmt.Errorf("fork creation failed - no repository returned") + return diag.FromErr(fmt.Errorf("fork creation failed - no repository returned")) } log.Printf("[INFO] Fork created with name: %s", fork.GetName()) @@ -723,7 +723,7 @@ func resourceGithubRepositoryCreate(d *schema.ResourceData, meta any) error { repo, _, err = client.Repositories.Create(ctx, "", repoReq) } if err != nil { - return err + return diag.FromErr(err) } d.SetId(repo.GetName()) } @@ -732,7 +732,7 @@ func resourceGithubRepositoryCreate(d *schema.ResourceData, meta any) error { if len(topics) > 0 { _, _, err := client.Repositories.ReplaceAllTopics(ctx, owner, repoName, topics) if err != nil { - return err + return diag.FromErr(err) } } @@ -740,19 +740,19 @@ func resourceGithubRepositoryCreate(d *schema.ResourceData, meta any) error { if pages != nil { _, _, err := client.Repositories.EnablePages(ctx, owner, repoName, pages) if err != nil { - return err + return diag.FromErr(err) } } err := updateVulnerabilityAlerts(d, client, ctx, owner, repoName) if err != nil { - return err + return diag.FromErr(err) } - return resourceGithubRepositoryUpdate(d, meta) + return resourceGithubRepositoryUpdate(ctx, d, meta) } -func resourceGithubRepositoryRead(d *schema.ResourceData, meta any) error { +func resourceGithubRepositoryRead(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { client := meta.(*Owner).v3client owner := meta.(*Owner).name @@ -764,7 +764,7 @@ func resourceGithubRepositoryRead(d *schema.ResourceData, meta any) error { owner = explicitOwner } - ctx := context.WithValue(context.Background(), ctxId, d.Id()) + ctx = context.WithValue(ctx, ctxId, d.Id()) if !d.IsNewResource() { ctx = context.WithValue(ctx, ctxEtag, d.Get("etag").(string)) } @@ -783,7 +783,7 @@ func resourceGithubRepositoryRead(d *schema.ResourceData, meta any) error { return nil } } - return err + return diag.FromErr(err) } _ = d.Set("etag", resp.Header.Get("ETag")) @@ -829,10 +829,10 @@ func resourceGithubRepositoryRead(d *schema.ResourceData, meta any) error { if repo.GetHasPages() { pages, _, err := client.Repositories.GetPagesInfo(ctx, owner, repoName) if err != nil { - return err + return diag.FromErr(err) } if err := d.Set("pages", flattenPages(pages)); err != nil { - return fmt.Errorf("error setting pages: %w", err) + return diag.FromErr(fmt.Errorf("error setting pages: %w", err)) } } @@ -858,34 +858,34 @@ func resourceGithubRepositoryRead(d *schema.ResourceData, meta any) error { "repository": repo.TemplateRepository.Name, }, }); err != nil { - return err + return diag.FromErr(err) } } else { if err = d.Set("template", []any{}); err != nil { - return err + return diag.FromErr(err) } } if !d.Get("ignore_vulnerability_alerts_during_read").(bool) { vulnerabilityAlerts, _, err := client.Repositories.GetVulnerabilityAlerts(ctx, owner, repoName) if err != nil { - return fmt.Errorf("error reading repository vulnerability alerts: %w", err) + return diag.FromErr(fmt.Errorf("error reading repository vulnerability alerts: %w", err)) } if err = d.Set("vulnerability_alerts", vulnerabilityAlerts); err != nil { - return err + return diag.FromErr(err) } } if err = d.Set("security_and_analysis", flattenSecurityAndAnalysis(repo.GetSecurityAndAnalysis())); err != nil { - return err + return diag.FromErr(err) } return nil } -func resourceGithubRepositoryUpdate(d *schema.ResourceData, meta any) error { +func resourceGithubRepositoryUpdate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { // Can only update a repository if it is not archived or the update is to - // archive the repository (unarchiving is not supported by the GitHub API) + // archive the repository if d.Get("archived").(bool) && !d.HasChange("archived") { log.Printf("[INFO] Skipping update of archived repository") return nil @@ -895,7 +895,7 @@ func resourceGithubRepositoryUpdate(d *schema.ResourceData, meta any) error { repoReq := resourceGithubRepositoryObject(d) - // handle visibility updates separately from other fields + // handle visibility updates separately from other fields // TODO: Review if this behaviour is still needed repoReq.Visibility = nil if !d.HasChange("security_and_analysis") { @@ -913,7 +913,7 @@ func resourceGithubRepositoryUpdate(d *schema.ResourceData, meta any) error { repoName := d.Id() owner := meta.(*Owner).name - ctx := context.WithValue(context.Background(), ctxId, d.Id()) + ctx = context.WithValue(ctx, ctxId, d.Id()) // When the organization has "Require sign off on web-based commits" enabled, // the API doesn't allow you to send `web_commit_signoff_required` in order to @@ -931,7 +931,7 @@ func resourceGithubRepositoryUpdate(d *schema.ResourceData, meta any) error { repo, _, err := client.Repositories.Edit(ctx, owner, repoName, repoReq) if err != nil { - return err + return diag.FromErr(err) } d.SetId(*repo.Name) @@ -940,7 +940,7 @@ func resourceGithubRepositoryUpdate(d *schema.ResourceData, meta any) error { if opts != nil { pages, res, err := client.Repositories.GetPagesInfo(ctx, owner, repoName) if res.StatusCode != http.StatusNotFound && err != nil { - return err + return diag.FromErr(err) } if pages == nil { @@ -949,12 +949,12 @@ func resourceGithubRepositoryUpdate(d *schema.ResourceData, meta any) error { _, err = client.Repositories.UpdatePages(ctx, owner, repoName, opts) } if err != nil { - return err + return diag.FromErr(err) } } else { _, err := client.Repositories.DisablePages(ctx, owner, repoName) if err != nil { - return err + return diag.FromErr(err) } } } @@ -963,7 +963,7 @@ func resourceGithubRepositoryUpdate(d *schema.ResourceData, meta any) error { topics := repoReq.Topics _, _, err = client.Repositories.ReplaceAllTopics(ctx, owner, *repo.Name, topics) if err != nil { - return err + return diag.FromErr(err) } d.SetId(*repo.Name) @@ -971,7 +971,7 @@ func resourceGithubRepositoryUpdate(d *schema.ResourceData, meta any) error { topics := repoReq.Topics _, _, err = client.Repositories.ReplaceAllTopics(ctx, owner, *repo.Name, topics) if err != nil { - return err + return diag.FromErr(err) } } } @@ -979,46 +979,46 @@ func resourceGithubRepositoryUpdate(d *schema.ResourceData, meta any) error { if d.HasChange("vulnerability_alerts") { err = updateVulnerabilityAlerts(d, client, ctx, owner, repoName) if err != nil { - return err + return diag.FromErr(err) } } - if d.HasChange("visibility") { + if d.HasChange("visibility") && !d.Get("archived").(bool) { o, n := d.GetChange("visibility") repoReq.Visibility = github.Ptr(n.(string)) log.Printf("[DEBUG] Updating repository visibility from %s to %s", o, n) _, resp, err := client.Repositories.Edit(ctx, owner, repoName, repoReq) if err != nil { if resp.StatusCode != 422 || !strings.Contains(err.Error(), fmt.Sprintf("Visibility is already %s", n.(string))) { - return err + return diag.FromErr(err) } } } else { log.Printf("[DEBUG] No visibility update required. visibility: %s", d.Get("visibility")) } - if d.HasChange("private") { + if d.HasChange("private") && !d.Get("archived").(bool) { o, n := d.GetChange("private") repoReq.Private = github.Ptr(n.(bool)) log.Printf("[DEBUG] Updating repository privacy from %v to %v", o, n) _, _, err = client.Repositories.Edit(ctx, owner, repoName, repoReq) if err != nil { if !strings.Contains(err.Error(), "422 Privacy is already set") { - return err + return diag.FromErr(err) } } } else { log.Printf("[DEBUG] No privacy update required. private: %v", d.Get("private")) } - return resourceGithubRepositoryRead(d, meta) + return resourceGithubRepositoryRead(ctx, d, meta) } -func resourceGithubRepositoryDelete(d *schema.ResourceData, meta any) error { +func resourceGithubRepositoryDelete(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { client := meta.(*Owner).v3client repoName := d.Id() owner := meta.(*Owner).name - ctx := context.WithValue(context.Background(), ctxId, d.Id()) + ctx = context.WithValue(ctx, ctxId, d.Id()) archiveOnDestroy := d.Get("archive_on_destroy").(bool) if archiveOnDestroy { @@ -1027,20 +1027,20 @@ func resourceGithubRepositoryDelete(d *schema.ResourceData, meta any) error { return nil } else { if err := d.Set("archived", true); err != nil { - return err + return diag.FromErr(err) } repoReq := resourceGithubRepositoryObject(d) // Always remove `web_commit_signoff_required` when archiving, to avoid 422 error repoReq.WebCommitSignoffRequired = nil log.Printf("[DEBUG] Archiving repository on delete: %s/%s", owner, repoName) _, _, err := client.Repositories.Edit(ctx, owner, repoName, repoReq) - return err + return diag.FromErr(err) } } log.Printf("[DEBUG] Deleting repository: %s/%s", owner, repoName) _, err := client.Repositories.Delete(ctx, owner, repoName) - return err + return diag.FromErr(err) } func expandPages(input []any) *github.Pages { diff --git a/github/resource_github_repository_ruleset.go b/github/resource_github_repository_ruleset.go index 52c23eba7f..e658b632c5 100644 --- a/github/resource_github_repository_ruleset.go +++ b/github/resource_github_repository_ruleset.go @@ -10,6 +10,8 @@ import ( "strconv" "github.com/google/go-github/v77/github" + "github.com/hashicorp/terraform-plugin-log/tflog" + "github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/validation" ) @@ -26,6 +28,11 @@ func resourceGithubRepositoryRuleset() *schema.Resource { SchemaVersion: 1, + CustomizeDiff: customdiff.All( + validateRepositoryRulesetConditions, + validateRepositoryRulesetRules, + ), + Schema: map[string]*schema.Schema{ "name": { Type: schema.TypeString, @@ -188,6 +195,16 @@ func resourceGithubRepositoryRuleset() *schema.Resource { Description: "Require all commits be made to a non-target branch and submitted via a pull request before they can be merged.", Elem: &schema.Resource{ Schema: map[string]*schema.Schema{ + "allowed_merge_methods": { + Type: schema.TypeList, + Required: true, + MinItems: 1, + Description: "Array of allowed merge methods. Allowed values include `merge`, `squash`, and `rebase`. At least one option must be enabled.", + Elem: &schema.Schema{ + Type: schema.TypeString, + ValidateDiagFunc: toDiagFunc(validation.StringInSlice([]string{"merge", "squash", "rebase"}, false), "allowed_merge_methods"), + }, + }, "dismiss_stale_reviews_on_push": { Type: schema.TypeBool, Optional: true, @@ -707,7 +724,7 @@ func resourceGithubRepositoryRulesetUpdate(d *schema.ResourceData, meta any) err var ruleset *github.RepositoryRuleset - if d.HasChange("bypass_actors") { + if d.HasChange("bypass_actors") && len(rulesetReq.BypassActors) == 0 { // Clear bypass actors first, then update with new ruleset _, err = client.Repositories.UpdateRulesetClearBypassActor(ctx, owner, repoName, rulesetID) if err != nil { @@ -773,3 +790,41 @@ func resourceGithubRepositoryRulesetImport(d *schema.ResourceData, meta any) ([] return []*schema.ResourceData{d}, nil } + +// validateRepositoryRulesetConditions validates conditions based on target type. +func validateRepositoryRulesetConditions(ctx context.Context, d *schema.ResourceDiff, _ any) error { + target := d.Get("target").(string) + tflog.Debug(ctx, "Validating repository ruleset conditions", map[string]any{"target": target}) + + conditionsRaw := d.Get("conditions").([]any) + if len(conditionsRaw) == 0 { + tflog.Debug(ctx, "No conditions block, skipping validation") + return nil + } + + conditions := conditionsRaw[0].(map[string]any) + + switch target { + case "branch", "tag": + return validateRepositoryRulesetConditionsFieldForBranchAndTagTargets(ctx, target, conditions) + case "push": + return validateConditionsFieldForPushTarget(ctx, conditions) + } + return nil +} + +func validateRepositoryRulesetRules(ctx context.Context, d *schema.ResourceDiff, _ any) error { + target := d.Get("target").(string) + tflog.Debug(ctx, "Validating repository ruleset rules based on target", map[string]any{"target": target}) + + rulesRaw := d.Get("rules").([]any) + if len(rulesRaw) == 0 { + tflog.Debug(ctx, "No rules block, skipping validation") + return nil + } + + value, exists := d.GetOk("rules.0.update_allows_fetch_and_merge") + tflog.Debug(ctx, "FOO", map[string]any{"foo": value, "exists": exists}) + + return validateRulesForTarget(ctx, d) +} diff --git a/github/resource_github_repository_ruleset_test.go b/github/resource_github_repository_ruleset_test.go index ee5d3c1c28..0da0426098 100644 --- a/github/resource_github_repository_ruleset_test.go +++ b/github/resource_github_repository_ruleset_test.go @@ -12,7 +12,7 @@ import ( "github.com/hashicorp/terraform-plugin-sdk/v2/terraform" ) -func TestGithubRepositoryRulesets(t *testing.T) { +func TestAccGithubRepositoryRulesets(t *testing.T) { randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) t.Run("Creates and updates repository rulesets without errors", func(t *testing.T) { @@ -21,7 +21,9 @@ func TestGithubRepositoryRulesets(t *testing.T) { name = "tf-acc-test-%s" auto_init = true default_branch = "main" - vulnerability_alerts = true + vulnerability_alerts = true + + visibility = "private" # Enables test even in GHEC EMU } resource "github_repository_environment" "example" { @@ -67,6 +69,7 @@ func TestGithubRepositoryRulesets(t *testing.T) { } pull_request { + allowed_merge_methods = ["merge", "squash", "rebase"] required_approving_review_count = 2 required_review_thread_resolution = true require_code_owner_review = true @@ -85,11 +88,11 @@ func TestGithubRepositoryRulesets(t *testing.T) { } required_code_scanning { - required_code_scanning_tool { - alerts_threshold = "errors" - security_alerts_threshold = "high_or_higher" - tool = "CodeQL" - } + required_code_scanning_tool { + alerts_threshold = "errors" + security_alerts_threshold = "high_or_higher" + tool = "CodeQL" + } } non_fast_forward = true @@ -165,6 +168,8 @@ func TestGithubRepositoryRulesets(t *testing.T) { name = "tf-acc-test-%s" auto_init = false vulnerability_alerts = true + + visibility = "private" # Enables test even in GHEC EMU } resource "github_repository_environment" "example" { @@ -232,9 +237,11 @@ func TestGithubRepositoryRulesets(t *testing.T) { config := fmt.Sprintf(` resource "github_repository" "test" { - name = "%[1]s" - description = "Terraform acceptance tests %[2]s" - vulnerability_alerts = true + name = "%[1]s" + description = "Terraform acceptance tests %[2]s" + vulnerability_alerts = true + + visibility = "private" # Enables test even in GHEC EMU } resource "github_repository_ruleset" "test" { @@ -301,11 +308,13 @@ func TestGithubRepositoryRulesets(t *testing.T) { t.Run("Imports rulesets without error", func(t *testing.T) { config := fmt.Sprintf(` resource "github_repository" "test" { - name = "tf-acc-test-import-%[1]s" - description = "Terraform acceptance tests %[1]s" - auto_init = true - default_branch = "main" - vulnerability_alerts = true + name = "tf-acc-test-import-%[1]s" + description = "Terraform acceptance tests %[1]s" + auto_init = true + default_branch = "main" + vulnerability_alerts = true + + visibility = "private" # Enables test even in GHEC EMU } resource "github_repository_environment" "example" { @@ -341,6 +350,7 @@ func TestGithubRepositoryRulesets(t *testing.T) { required_signatures = false pull_request { + allowed_merge_methods = ["merge", "squash", "rebase"] required_approving_review_count = 2 required_review_thread_resolution = true require_code_owner_review = true @@ -429,13 +439,13 @@ func TestGithubRepositoryRulesets(t *testing.T) { rules { file_path_restriction { - restricted_file_paths = ["test.txt"] + restricted_file_paths = ["test.txt"] } max_file_size { - max_file_size = 1048576 + max_file_size = 10 # Value is in megabytes (MB), valid range is 1-100 } file_extension_restriction { - restricted_file_extensions = ["*.zip"] + restricted_file_extensions = ["*.zip"] } } } @@ -456,7 +466,7 @@ func TestGithubRepositoryRulesets(t *testing.T) { ), resource.TestCheckResourceAttr( "github_repository_ruleset.test_push", "rules.0.max_file_size.0.max_file_size", - "1048576", + "10", ), resource.TestCheckResourceAttr( "github_repository_ruleset.test_push", "rules.0.file_extension_restriction.0.restricted_file_extensions.0", @@ -492,6 +502,8 @@ func TestGithubRepositoryRulesets(t *testing.T) { name = "tf-acc-test-merge-queue-%s" auto_init = true default_branch = "main" + + visibility = "private" # Enables test even in GHEC EMU } resource "github_repository_ruleset" "test" { @@ -564,11 +576,15 @@ func TestGithubRepositoryRulesets(t *testing.T) { name = "tf-acc-test-bypass-%s" description = "Terraform acceptance tests %[1]s" auto_init = true + + visibility = "private" # Enables test even in GHEC EMU } resource "github_team" "test" { name = "tf-acc-test-team-%[1]s" description = "Terraform acc test team" + + privacy = "closed" } resource "github_repository_ruleset" "test" { @@ -592,6 +608,7 @@ func TestGithubRepositoryRulesets(t *testing.T) { rules { pull_request { + allowed_merge_methods = ["merge", "squash", "rebase"] dismiss_stale_reviews_on_push = false require_code_owner_review = true require_last_push_approval = false @@ -670,6 +687,8 @@ func TestGithubRepositoryRulesets(t *testing.T) { name = "tf-acc-test-no-bypass-%s" description = "Terraform acceptance tests %[1]s" auto_init = true + + visibility = "private" # Enables test even in GHEC EMU } resource "github_repository_ruleset" "test" { @@ -757,21 +776,32 @@ func TestGithubRepositoryRulesets(t *testing.T) { name = "tf-acc-test-bypass-modes-%s" description = "Terraform acceptance tests %[1]s" auto_init = true + + visibility = "private" # Enables test even in GHEC EMU + + ignore_vulnerability_alerts_during_read = true + } resource "github_team" "test_always" { name = "tf-acc-test-team-always-%[1]s" description = "Terraform acc test team for always bypass" + + privacy = "closed" } resource "github_team" "test_pull_request" { name = "tf-acc-test-team-pr-%[1]s" description = "Terraform acc test team for pull_request bypass" + + privacy = "closed" } resource "github_team" "test_exempt" { name = "tf-acc-test-team-exempt-%[1]s" description = "Terraform acc test team for exempt bypass" + + privacy = "closed" } resource "github_repository_ruleset" "test" { @@ -819,10 +849,11 @@ func TestGithubRepositoryRulesets(t *testing.T) { resource.TestCheckResourceAttrSet( "github_repository_ruleset.test", "bypass_actors.0.actor_id", ), - resource.TestCheckResourceAttr( - "github_repository_ruleset.test", "bypass_actors.0.bypass_mode", - "always", - ), + // TODO: We need to figure out sorting of bypass_actors. Maybe it needs to be a TypeSet instead of a List. + // resource.TestCheckResourceAttr( + // "github_repository_ruleset.test", "bypass_actors.0.bypass_mode", + // "always", + // ), resource.TestCheckResourceAttr( "github_repository_ruleset.test", "bypass_actors.0.actor_type", "Team", @@ -830,10 +861,11 @@ func TestGithubRepositoryRulesets(t *testing.T) { resource.TestCheckResourceAttrSet( "github_repository_ruleset.test", "bypass_actors.1.actor_id", ), - resource.TestCheckResourceAttr( - "github_repository_ruleset.test", "bypass_actors.1.bypass_mode", - "pull_request", - ), + // TODO: We need to figure out sorting of bypass_actors. Maybe it needs to be a TypeSet instead of a List. + // resource.TestCheckResourceAttr( + // "github_repository_ruleset.test", "bypass_actors.1.bypass_mode", + // "pull_request", + // ), resource.TestCheckResourceAttr( "github_repository_ruleset.test", "bypass_actors.1.actor_type", "Team", @@ -841,10 +873,11 @@ func TestGithubRepositoryRulesets(t *testing.T) { resource.TestCheckResourceAttrSet( "github_repository_ruleset.test", "bypass_actors.2.actor_id", ), - resource.TestCheckResourceAttr( - "github_repository_ruleset.test", "bypass_actors.2.bypass_mode", - "exempt", - ), + // TODO: We need to figure out sorting of bypass_actors. Maybe it needs to be a TypeSet instead of a List. + // resource.TestCheckResourceAttr( + // "github_repository_ruleset.test", "bypass_actors.2.bypass_mode", + // "exempt", + // ), resource.TestCheckResourceAttr( "github_repository_ruleset.test", "bypass_actors.2.actor_type", "Team", @@ -883,11 +916,15 @@ func TestGithubRepositoryRulesets(t *testing.T) { name = "tf-acc-test-bypass-update-%s" description = "Terraform acceptance tests %[1]s" auto_init = true + + visibility = "private" # Enables test even in GHEC EMU } resource "github_team" "test" { name = "tf-acc-test-team-update-%[1]s" description = "Terraform acc test team" + + privacy = "closed" } resource "github_repository_ruleset" "test" { @@ -973,11 +1010,15 @@ func TestGithubRepositoryRulesets(t *testing.T) { name = "tf-acc-test-actor-types-%s" description = "Terraform acceptance tests %[1]s" auto_init = true + + visibility = "private" # Enables test even in GHEC EMU } resource "github_team" "test" { name = "tf-acc-test-team-actor-%[1]s" description = "Terraform acc test team" + + privacy = "closed" } resource "github_repository_ruleset" "test" { @@ -1017,21 +1058,22 @@ func TestGithubRepositoryRulesets(t *testing.T) { } `, randomID) - check := resource.ComposeTestCheckFunc( + check := resource.ComposeAggregateTestCheckFunc( resource.TestCheckResourceAttr( "github_repository_ruleset.test", "bypass_actors.#", "3", ), - resource.TestCheckResourceAttrSet( + resource.TestCheckResourceAttr( "github_repository_ruleset.test", "bypass_actors.0.actor_id", + "1", ), resource.TestCheckResourceAttr( "github_repository_ruleset.test", "bypass_actors.0.actor_type", - "Team", + "OrganizationAdmin", ), resource.TestCheckResourceAttr( "github_repository_ruleset.test", "bypass_actors.0.bypass_mode", - "always", + "exempt", ), resource.TestCheckResourceAttr( "github_repository_ruleset.test", "bypass_actors.1.actor_id", @@ -1045,17 +1087,16 @@ func TestGithubRepositoryRulesets(t *testing.T) { "github_repository_ruleset.test", "bypass_actors.1.bypass_mode", "pull_request", ), - resource.TestCheckResourceAttr( + resource.TestCheckResourceAttrSet( "github_repository_ruleset.test", "bypass_actors.2.actor_id", - "1", ), resource.TestCheckResourceAttr( "github_repository_ruleset.test", "bypass_actors.2.actor_type", - "OrganizationAdmin", + "Team", ), resource.TestCheckResourceAttr( "github_repository_ruleset.test", "bypass_actors.2.bypass_mode", - "exempt", + "always", ), ) @@ -1086,7 +1127,7 @@ func TestGithubRepositoryRulesets(t *testing.T) { }) } -func TestGithubRepositoryRulesetArchived(t *testing.T) { +func TestAccGithubRepositoryRulesetArchived(t *testing.T) { randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) t.Run("skips update and delete on archived repository", func(t *testing.T) { @@ -1095,6 +1136,8 @@ func TestGithubRepositoryRulesetArchived(t *testing.T) { name = "tf-acc-test-archive-%s" auto_init = true archived = false + + visibility = "private" # Enables test even in GHEC EMU } resource "github_repository_ruleset" "test" { name = "test" @@ -1122,6 +1165,8 @@ func TestGithubRepositoryRulesetArchived(t *testing.T) { name = "tf-acc-test-archive-create-%s" auto_init = true archived = true + + visibility = "private" # Enables test even in GHEC EMU } resource "github_repository_ruleset" "test" { name = "test" @@ -1142,6 +1187,238 @@ func TestGithubRepositoryRulesetArchived(t *testing.T) { }) } +func TestAccGithubRepositoryRulesetValidation(t *testing.T) { + randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum) + + t.Run("Validates push target rejects ref_name condition", func(t *testing.T) { + if isPaidPlan != "true" { + t.Skip("Skipping because `GITHUB_PAID_FEATURES` is not set to true") + } + + config := fmt.Sprintf(` + resource "github_repository" "test" { + name = "tf-acc-test-push-ref-%s" + auto_init = true + visibility = "internal" + vulnerability_alerts = true + } + + resource "github_repository_ruleset" "test" { + name = "test-push-with-ref" + repository = github_repository.test.id + target = "push" + enforcement = "active" + + conditions { + ref_name { + include = ["~ALL"] + exclude = [] + } + } + + rules { + max_file_size { + max_file_size = 100 + } + } + } + `, randomID) + + testCase := func(t *testing.T, mode string) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnlessMode(t, mode) }, + Providers: testAccProviders, + Steps: []resource.TestStep{ + { + Config: config, + ExpectError: regexp.MustCompile("ref_name must not be set for push target"), + }, + }, + }) + } + + t.Run("with an anonymous account", func(t *testing.T) { + t.Skip("anonymous account not supported for this operation") + }) + + t.Run("with an individual account", func(t *testing.T) { + t.Skip("individual account not supported for push rulesets") + }) + + t.Run("with an organization account", func(t *testing.T) { + testCase(t, organization) + }) + }) + + t.Run("Validates push target rejects branch/tag rules", func(t *testing.T) { + if isPaidPlan != "true" { + t.Skip("Skipping because `GITHUB_PAID_FEATURES` is not set to true") + } + + config := fmt.Sprintf(` + resource "github_repository" "test" { + name = "tf-acc-test-push-rules-%s" + auto_init = true + visibility = "internal" + vulnerability_alerts = true + } + + resource "github_repository_ruleset" "test" { + name = "test-push-branch-rule" + repository = github_repository.test.id + target = "push" + enforcement = "active" + + rules { + # 'creation' is a branch/tag rule, not valid for push target + creation = true + } + } + `, randomID) + + testCase := func(t *testing.T, mode string) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnlessMode(t, mode) }, + Providers: testAccProviders, + Steps: []resource.TestStep{ + { + Config: config, + ExpectError: regexp.MustCompile("rule .* is not valid for push target"), + }, + }, + }) + } + + t.Run("with an anonymous account", func(t *testing.T) { + t.Skip("anonymous account not supported for this operation") + }) + + t.Run("with an individual account", func(t *testing.T) { + t.Skip("individual account not supported for push rulesets") + }) + + t.Run("with an organization account", func(t *testing.T) { + testCase(t, organization) + }) + }) + + t.Run("Validates branch target rejects push-only rules", func(t *testing.T) { + config := fmt.Sprintf(` + resource "github_repository" "test" { + name = "tf-acc-test-branch-push-%s" + auto_init = true + vulnerability_alerts = true + + visibility = "private" + } + + resource "github_repository_ruleset" "test" { + name = "test-branch-push-rule" + repository = github_repository.test.id + target = "branch" + enforcement = "active" + + conditions { + ref_name { + include = ["~ALL"] + exclude = [] + } + } + + rules { + # 'max_file_size' is a push-only rule, not valid for branch target + max_file_size { + max_file_size = 100 + } + } + } + `, randomID) + + testCase := func(t *testing.T, mode string) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnlessMode(t, mode) }, + Providers: testAccProviders, + Steps: []resource.TestStep{ + { + Config: config, + ExpectError: regexp.MustCompile("rule .* is not valid for branch target"), + }, + }, + }) + } + + t.Run("with an anonymous account", func(t *testing.T) { + t.Skip("anonymous account not supported for this operation") + }) + + t.Run("with an individual account", func(t *testing.T) { + testCase(t, individual) + }) + + t.Run("with an organization account", func(t *testing.T) { + testCase(t, organization) + }) + }) + + t.Run("Validates tag target rejects push-only rules", func(t *testing.T) { + config := fmt.Sprintf(` + resource "github_repository" "test" { + name = "tf-acc-test-tag-push-%s" + auto_init = true + vulnerability_alerts = true + + visibility = "private" + } + + resource "github_repository_ruleset" "test" { + name = "test-tag-push-rule" + repository = github_repository.test.id + target = "tag" + enforcement = "active" + + conditions { + ref_name { + include = ["~ALL"] + exclude = [] + } + } + + rules { + # 'file_path_restriction' is a push-only rule, not valid for tag target + file_path_restriction { + restricted_file_paths = ["secrets/"] + } + } + } + `, randomID) + + testCase := func(t *testing.T, mode string) { + resource.Test(t, resource.TestCase{ + PreCheck: func() { skipUnlessMode(t, mode) }, + Providers: testAccProviders, + Steps: []resource.TestStep{ + { + Config: config, + ExpectError: regexp.MustCompile("rule .* is not valid for tag target"), + }, + }, + }) + } + + t.Run("with an anonymous account", func(t *testing.T) { + t.Skip("anonymous account not supported for this operation") + }) + + t.Run("with an individual account", func(t *testing.T) { + testCase(t, individual) + }) + + t.Run("with an organization account", func(t *testing.T) { + testCase(t, organization) + }) + }) +} + func importRepositoryRulesetByResourcePaths(repoLogicalName, rulesetLogicalName string) resource.ImportStateIdFunc { // test importing using an ID of the form : return func(s *terraform.State) (string, error) { diff --git a/github/resource_github_team.go b/github/resource_github_team.go index 70c7117a32..6a28fde089 100644 --- a/github/resource_github_team.go +++ b/github/resource_github_team.go @@ -8,6 +8,7 @@ import ( "strconv" "github.com/google/go-github/v77/github" + "github.com/hashicorp/terraform-plugin-sdk/v2/diag" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/customdiff" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" "github.com/shurcooL/githubv4" @@ -15,12 +16,12 @@ import ( func resourceGithubTeam() *schema.Resource { return &schema.Resource{ - Create: resourceGithubTeamCreate, - Read: resourceGithubTeamRead, - Update: resourceGithubTeamUpdate, - Delete: resourceGithubTeamDelete, + CreateContext: resourceGithubTeamCreate, + ReadContext: resourceGithubTeamRead, + UpdateContext: resourceGithubTeamUpdate, + DeleteContext: resourceGithubTeamDelete, Importer: &schema.ResourceImporter{ - State: resourceGithubTeamImport, + StateContext: resourceGithubTeamImport, }, CustomizeDiff: customdiff.Sequence( @@ -104,10 +105,10 @@ func resourceGithubTeam() *schema.Resource { } } -func resourceGithubTeamCreate(d *schema.ResourceData, meta any) error { +func resourceGithubTeamCreate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { err := checkOrganization(meta) if err != nil { - return err + return diag.FromErr(err) } client := meta.(*Owner).v3client @@ -128,16 +129,15 @@ func resourceGithubTeamCreate(d *schema.ResourceData, meta any) error { if parentTeamID, ok := d.GetOk("parent_team_id"); ok { teamID, err := getTeamID(parentTeamID.(string), meta) if err != nil { - return err + return diag.FromErr(err) } newTeam.ParentTeamID = &teamID } - ctx := context.Background() githubTeam, _, err := client.Teams.CreateTeam(ctx, ownerName, newTeam) if err != nil { - return err + return diag.FromErr(err) } /* @@ -161,7 +161,7 @@ func resourceGithubTeamCreate(d *schema.ResourceData, meta any) error { newTeam, false) if err != nil { - return err + return diag.FromErr(err) } } @@ -169,18 +169,18 @@ func resourceGithubTeamCreate(d *schema.ResourceData, meta any) error { if !create_default_maintainer { log.Printf("[DEBUG] Removing default maintainer from team: %s (%s)", name, ownerName) if err := removeDefaultMaintainer(*githubTeam.Slug, meta); err != nil { - return err + return diag.FromErr(err) } } d.SetId(strconv.FormatInt(githubTeam.GetID(), 10)) - return resourceGithubTeamRead(d, meta) + return resourceGithubTeamRead(ctx, d, meta) } -func resourceGithubTeamRead(d *schema.ResourceData, meta any) error { +func resourceGithubTeamRead(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { err := checkOrganization(meta) if err != nil { - return err + return diag.FromErr(err) } client := meta.(*Owner).v3client @@ -188,9 +188,9 @@ func resourceGithubTeamRead(d *schema.ResourceData, meta any) error { id, err := strconv.ParseInt(d.Id(), 10, 64) if err != nil { - return unconvertibleIdErr(d.Id(), err) + return diag.FromErr(unconvertibleIdErr(d.Id(), err)) } - ctx := context.WithValue(context.Background(), ctxId, d.Id()) + ctx = context.WithValue(ctx, ctxId, d.Id()) if !d.IsNewResource() { ctx = context.WithValue(ctx, ctxEtag, d.Get("etag").(string)) } @@ -210,62 +210,62 @@ func resourceGithubTeamRead(d *schema.ResourceData, meta any) error { return nil } } - return err + return diag.FromErr(err) } if err = d.Set("etag", resp.Header.Get("ETag")); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("description", team.GetDescription()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("name", team.GetName()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("privacy", team.GetPrivacy()); err != nil { - return err + return diag.FromErr(err) } if parent := team.Parent; parent != nil { if err = d.Set("parent_team_id", strconv.FormatInt(team.Parent.GetID(), 10)); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("parent_team_read_id", strconv.FormatInt(team.Parent.GetID(), 10)); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("parent_team_read_slug", parent.Slug); err != nil { - return err + return diag.FromErr(err) } } else { if err = d.Set("parent_team_id", ""); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("parent_team_read_id", ""); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("parent_team_read_slug", ""); err != nil { - return err + return diag.FromErr(err) } } if err = d.Set("ldap_dn", team.GetLDAPDN()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("slug", team.GetSlug()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("node_id", team.GetNodeID()); err != nil { - return err + return diag.FromErr(err) } if err = d.Set("members_count", team.GetMembersCount()); err != nil { - return err + return diag.FromErr(err) } return nil } -func resourceGithubTeamUpdate(d *schema.ResourceData, meta any) error { +func resourceGithubTeamUpdate(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { err := checkOrganization(meta) if err != nil { - return err + return diag.FromErr(err) } client := meta.(*Owner).v3client @@ -279,7 +279,7 @@ func resourceGithubTeamUpdate(d *schema.ResourceData, meta any) error { if parentTeamID, ok := d.GetOk("parent_team_id"); ok { teamID, err := getTeamID(parentTeamID.(string), meta) if err != nil { - return err + return diag.FromErr(err) } editedTeam.ParentTeamID = &teamID removeParentTeam = false @@ -289,15 +289,15 @@ func resourceGithubTeamUpdate(d *schema.ResourceData, meta any) error { teamID, err := strconv.ParseInt(d.Id(), 10, 64) if err != nil { - return unconvertibleIdErr(d.Id(), err) + return diag.FromErr(unconvertibleIdErr(d.Id(), err)) } - ctx := context.WithValue(context.Background(), ctxId, d.Id()) + ctx = context.WithValue(ctx, ctxId, d.Id()) //nolint:staticcheck // SA1019: EditTeamByID is deprecated but still needed for legacy compatibility team, _, err := client.Teams.EditTeamByID(ctx, meta.(*Owner).id, teamID, editedTeam, removeParentTeam) if err != nil { - return err + return diag.FromErr(err) } if d.HasChange("ldap_dn") { @@ -307,18 +307,18 @@ func resourceGithubTeamUpdate(d *schema.ResourceData, meta any) error { } _, _, err = client.Admin.UpdateTeamLDAPMapping(ctx, team.GetID(), mapping) if err != nil { - return err + return diag.FromErr(err) } } d.SetId(strconv.FormatInt(team.GetID(), 10)) - return resourceGithubTeamRead(d, meta) + return resourceGithubTeamRead(ctx, d, meta) } -func resourceGithubTeamDelete(d *schema.ResourceData, meta any) error { +func resourceGithubTeamDelete(ctx context.Context, d *schema.ResourceData, meta any) diag.Diagnostics { err := checkOrganization(meta) if err != nil { - return err + return diag.FromErr(err) } client := meta.(*Owner).v3client @@ -326,9 +326,9 @@ func resourceGithubTeamDelete(d *schema.ResourceData, meta any) error { id, err := strconv.ParseInt(d.Id(), 10, 64) if err != nil { - return unconvertibleIdErr(d.Id(), err) + return diag.FromErr(unconvertibleIdErr(d.Id(), err)) } - ctx := context.WithValue(context.Background(), ctxId, d.Id()) + ctx = context.WithValue(ctx, ctxId, d.Id()) //nolint:staticcheck // SA1019: DeleteTeamByID is deprecated but still needed for legacy compatibility _, err = client.Teams.DeleteTeamByID(ctx, orgID, id) @@ -356,10 +356,10 @@ func resourceGithubTeamDelete(d *schema.ResourceData, meta any) error { } } } - return err + return diag.FromErr(err) } -func resourceGithubTeamImport(d *schema.ResourceData, meta any) ([]*schema.ResourceData, error) { +func resourceGithubTeamImport(ctx context.Context, d *schema.ResourceData, meta any) ([]*schema.ResourceData, error) { teamID, err := getTeamID(d.Id(), meta) if err != nil { return nil, err diff --git a/github/respository_rules_utils.go b/github/respository_rules_utils.go index 4044293f52..57ffaea8a9 100644 --- a/github/respository_rules_utils.go +++ b/github/respository_rules_utils.go @@ -1,13 +1,18 @@ package github import ( + "context" + "log" "reflect" "sort" "github.com/google/go-github/v77/github" + "github.com/hashicorp/terraform-plugin-log/tflog" "github.com/hashicorp/terraform-plugin-sdk/v2/helper/schema" ) +var DEFAULT_PULL_REQUEST_MERGE_METHODS = []github.PullRequestMergeMethod{github.PullRequestMergeMethodMerge, github.PullRequestMergeMethodRebase, github.PullRequestMergeMethodSquash} + // Helper function to safely convert interface{} to int, handling both int and float64. func toInt(v any) int { switch val := v.(type) { @@ -36,6 +41,21 @@ func toInt64(v any) int64 { } } +func toPullRequestMergeMethods(input any) []github.PullRequestMergeMethod { + value, ok := input.([]any) + if !ok || value == nil || len(value) == 0 { + log.Printf("[DEBUG] No allowed merge methods provided, using default: %#v", input) + return DEFAULT_PULL_REQUEST_MERGE_METHODS + } + mergeMethods := make([]github.PullRequestMergeMethod, 0, len(value)) + for _, item := range value { + if method, ok := item.(string); ok { + mergeMethods = append(mergeMethods, github.PullRequestMergeMethod(method)) + } + } + return mergeMethods +} + func resourceGithubRulesetObject(d *schema.ResourceData, org string) *github.RepositoryRuleset { isOrgLevel := len(org) > 0 @@ -115,9 +135,15 @@ func flattenBypassActors(bypassActors []*github.BypassActor) []any { actorsSlice := make([]any, 0) for _, v := range bypassActors { actorMap := make(map[string]any) - - actorMap["actor_id"] = v.GetActorID() - actorMap["actor_type"] = v.GetActorType() + actorID := v.GetActorID() + actorType := v.GetActorType() + if *actorType == github.BypassActorTypeOrganizationAdmin && actorID == 0 { + // This is a workaround for the GitHub API bug where OrganizationAdmin actor_id is returned as `null` instead of `1` + log.Printf("[DEBUG] Setting OrganizationAdmin Actor ID to 1") + actorID = 1 + } + actorMap["actor_id"] = actorID + actorMap["actor_type"] = actorType actorMap["bypass_mode"] = v.GetBypassMode() actorsSlice = append(actorsSlice, actorMap) @@ -201,19 +227,26 @@ func expandConditions(input []any, org bool) *github.RepositoryRulesetConditions } func flattenConditions(conditions *github.RepositoryRulesetConditions, org bool) []any { - if conditions == nil || conditions.RefName == nil { + return flattenConditionsWithContext(context.TODO(), conditions, org) +} + +func flattenConditionsWithContext(ctx context.Context, conditions *github.RepositoryRulesetConditions, org bool) []any { + if conditions == nil || reflect.DeepEqual(conditions, &github.RepositoryRulesetConditions{}) { + tflog.Debug(ctx, "Conditions are empty, returning empty list") return []any{} } conditionsMap := make(map[string]any) refNameSlice := make([]map[string]any, 0) - refNameSlice = append(refNameSlice, map[string]any{ - "include": conditions.RefName.Include, - "exclude": conditions.RefName.Exclude, - }) + if conditions.RefName != nil { + refNameSlice = append(refNameSlice, map[string]any{ + "include": conditions.RefName.Include, + "exclude": conditions.RefName.Exclude, + }) - conditionsMap["ref_name"] = refNameSlice + conditionsMap["ref_name"] = refNameSlice + } // org-only fields if org { @@ -295,12 +328,15 @@ func expandRules(input []any, org bool) *github.RepositoryRulesetRules { // Pull request rule if v, ok := rulesMap["pull_request"].([]any); ok && len(v) != 0 { pullRequestMap := v[0].(map[string]any) + allowedMergeMethods := pullRequestMap["allowed_merge_methods"] + params := &github.PullRequestRuleParameters{ DismissStaleReviewsOnPush: pullRequestMap["dismiss_stale_reviews_on_push"].(bool), RequireCodeOwnerReview: pullRequestMap["require_code_owner_review"].(bool), RequireLastPushApproval: pullRequestMap["require_last_push_approval"].(bool), RequiredApprovingReviewCount: toInt(pullRequestMap["required_approving_review_count"]), RequiredReviewThreadResolution: pullRequestMap["required_review_thread_resolution"].(bool), + AllowedMergeMethods: toPullRequestMergeMethods(allowedMergeMethods), } rulesetRules.PullRequest = params } @@ -515,10 +551,14 @@ func flattenRules(rules *github.RepositoryRulesetRules, org bool) []any { // Update rule with parameters if rules.Update != nil { rulesMap["update"] = true - rulesMap["update_allows_fetch_and_merge"] = rules.Update.UpdateAllowsFetchAndMerge + if !org { + rulesMap["update_allows_fetch_and_merge"] = rules.Update.UpdateAllowsFetchAndMerge + } } else { rulesMap["update"] = false - rulesMap["update_allows_fetch_and_merge"] = false + if !org { + rulesMap["update_allows_fetch_and_merge"] = false + } } // Required deployments rule if rules.RequiredDeployments != nil { requiredDeploymentsSlice := make([]map[string]any, 0) @@ -537,7 +577,9 @@ func flattenRules(rules *github.RepositoryRulesetRules, org bool) []any { "require_last_push_approval": rules.PullRequest.RequireLastPushApproval, "required_approving_review_count": rules.PullRequest.RequiredApprovingReviewCount, "required_review_thread_resolution": rules.PullRequest.RequiredReviewThreadResolution, + "allowed_merge_methods": rules.PullRequest.AllowedMergeMethods, }) + log.Printf("[DEBUG] Flattened Pull Request rules slice: %#v", pullRequestSlice) rulesMap["pull_request"] = pullRequestSlice } diff --git a/github/respository_rules_utils_test.go b/github/respository_rules_utils_test.go index df3b042f24..164380b600 100644 --- a/github/respository_rules_utils_test.go +++ b/github/respository_rules_utils_test.go @@ -418,3 +418,155 @@ func TestCompletePushRulesetSupport(t *testing.T) { t.Errorf("Expected 3 restricted file extensions, got %d", len(restrictedExts)) } } + +func TestFlattenConditions_PushRuleset_WithRepositoryNameOnly(t *testing.T) { + // Push rulesets don't use ref_name - they only have repository_name or repository_id. + // flattenConditions should return the conditions even when RefName is nil. + conditions := &github.RepositoryRulesetConditions{ + RefName: nil, // Push rulesets don't have ref_name + RepositoryName: &github.RepositoryRulesetRepositoryNamesConditionParameters{ + Include: []string{"~ALL"}, + Exclude: []string{}, + }, + } + + result := flattenConditions(conditions, true) // org=true for organization rulesets + + if len(result) != 1 { + t.Fatalf("Expected 1 conditions block, got %d", len(result)) + } + + conditionsMap := result[0].(map[string]any) + + // ref_name should be empty for push rulesets + refNameSlice := conditionsMap["ref_name"] + if refNameSlice != nil { + t.Fatalf("Expected ref_name to be nil, got %T", conditionsMap["ref_name"]) + } + + // repository_name should be present + repoNameSlice, ok := conditionsMap["repository_name"].([]map[string]any) + if !ok { + t.Fatalf("Expected repository_name to be []map[string]any, got %T", conditionsMap["repository_name"]) + } + if len(repoNameSlice) != 1 { + t.Fatalf("Expected 1 repository_name block, got %d", len(repoNameSlice)) + } + + include, ok := repoNameSlice[0]["include"].([]string) + if !ok { + t.Fatalf("Expected include to be []string, got %T", repoNameSlice[0]["include"]) + } + if len(include) != 1 || include[0] != "~ALL" { + t.Errorf("Expected include to be [~ALL], got %v", include) + } +} + +func TestFlattenConditions_BranchRuleset_WithRefNameAndRepositoryName(t *testing.T) { + // Branch/tag rulesets have both ref_name and repository_name. + // This test ensures we didn't break the existing behavior. + conditions := &github.RepositoryRulesetConditions{ + RefName: &github.RepositoryRulesetRefConditionParameters{ + Include: []string{"~DEFAULT_BRANCH", "refs/heads/main"}, + Exclude: []string{"refs/heads/experimental-*"}, + }, + RepositoryName: &github.RepositoryRulesetRepositoryNamesConditionParameters{ + Include: []string{"~ALL"}, + Exclude: []string{"test-*"}, + }, + } + + result := flattenConditions(conditions, true) // org=true for organization rulesets + + if len(result) != 1 { + t.Fatalf("Expected 1 conditions block, got %d", len(result)) + } + + conditionsMap := result[0].(map[string]any) + + // ref_name should be present for branch/tag rulesets + refNameSlice, ok := conditionsMap["ref_name"].([]map[string]any) + if !ok { + t.Fatalf("Expected ref_name to be []map[string]any, got %T", conditionsMap["ref_name"]) + } + if len(refNameSlice) != 1 { + t.Fatalf("Expected 1 ref_name block, got %d", len(refNameSlice)) + } + + refInclude, ok := refNameSlice[0]["include"].([]string) + if !ok { + t.Fatalf("Expected ref_name include to be []string, got %T", refNameSlice[0]["include"]) + } + if len(refInclude) != 2 { + t.Errorf("Expected 2 ref_name includes, got %d", len(refInclude)) + } + + refExclude, ok := refNameSlice[0]["exclude"].([]string) + if !ok { + t.Fatalf("Expected ref_name exclude to be []string, got %T", refNameSlice[0]["exclude"]) + } + if len(refExclude) != 1 { + t.Errorf("Expected 1 ref_name exclude, got %d", len(refExclude)) + } + + // repository_name should also be present + repoNameSlice, ok := conditionsMap["repository_name"].([]map[string]any) + if !ok { + t.Fatalf("Expected repository_name to be []map[string]any, got %T", conditionsMap["repository_name"]) + } + if len(repoNameSlice) != 1 { + t.Fatalf("Expected 1 repository_name block, got %d", len(repoNameSlice)) + } + + repoInclude, ok := repoNameSlice[0]["include"].([]string) + if !ok { + t.Fatalf("Expected repository_name include to be []string, got %T", repoNameSlice[0]["include"]) + } + if len(repoInclude) != 1 || repoInclude[0] != "~ALL" { + t.Errorf("Expected repository_name include to be [~ALL], got %v", repoInclude) + } + + repoExclude, ok := repoNameSlice[0]["exclude"].([]string) + if !ok { + t.Fatalf("Expected repository_name exclude to be []string, got %T", repoNameSlice[0]["exclude"]) + } + if len(repoExclude) != 1 || repoExclude[0] != "test-*" { + t.Errorf("Expected repository_name exclude to be [test-*], got %v", repoExclude) + } +} + +func TestFlattenConditions_PushRuleset_WithRepositoryIdOnly(t *testing.T) { + // Push rulesets can also use repository_id instead of repository_name. + conditions := &github.RepositoryRulesetConditions{ + RefName: nil, // Push rulesets don't have ref_name + RepositoryID: &github.RepositoryRulesetRepositoryIDsConditionParameters{ + RepositoryIDs: []int64{12345, 67890}, + }, + } + + result := flattenConditions(conditions, true) // org=true for organization rulesets + + if len(result) != 1 { + t.Fatalf("Expected 1 conditions block, got %d", len(result)) + } + + conditionsMap := result[0].(map[string]any) + + // ref_name should be nil for push rulesets + refNameSlice := conditionsMap["ref_name"] + if refNameSlice != nil { + t.Fatalf("Expected ref_name to be nil, got %T", conditionsMap["ref_name"]) + } + + // repository_id should be present + repoIDs, ok := conditionsMap["repository_id"].([]int64) + if !ok { + t.Fatalf("Expected repository_id to be []int64, got %T", conditionsMap["repository_id"]) + } + if len(repoIDs) != 2 { + t.Fatalf("Expected 2 repository IDs, got %d", len(repoIDs)) + } + if repoIDs[0] != 12345 || repoIDs[1] != 67890 { + t.Errorf("Expected repository IDs [12345, 67890], got %v", repoIDs) + } +} diff --git a/go.mod b/go.mod index eacd1663f7..eca003b564 100644 --- a/go.mod +++ b/go.mod @@ -9,6 +9,7 @@ require ( github.com/google/go-github/v77 v77.0.0 github.com/google/uuid v1.6.0 github.com/hashicorp/go-cty v1.5.0 + github.com/hashicorp/terraform-plugin-log v0.9.0 github.com/hashicorp/terraform-plugin-sdk/v2 v2.38.1 github.com/shurcooL/githubv4 v0.0.0-20221126192849-0b5c4c7994eb github.com/stretchr/testify v1.11.1 @@ -129,7 +130,6 @@ require ( github.com/hashicorp/terraform-exec v0.23.1 // indirect github.com/hashicorp/terraform-json v0.27.1 // indirect github.com/hashicorp/terraform-plugin-go v0.29.0 // indirect - github.com/hashicorp/terraform-plugin-log v0.9.0 // indirect github.com/hashicorp/terraform-registry-address v0.4.0 // indirect github.com/hashicorp/terraform-svchost v0.1.1 // indirect github.com/hashicorp/yamux v0.1.2 // indirect diff --git a/website/docs/r/organization_ruleset.html.markdown b/website/docs/r/organization_ruleset.html.markdown index e04118f678..12c228ee7e 100644 --- a/website/docs/r/organization_ruleset.html.markdown +++ b/website/docs/r/organization_ruleset.html.markdown @@ -65,24 +65,23 @@ resource "github_organization_ruleset" "example" { } } -# Example with push ruleset +# Example with push ruleset +# Note: Push targets must NOT have ref_name in conditions, only repository_name or repository_id resource "github_organization_ruleset" "example_push" { name = "example_push" target = "push" enforcement = "active" conditions { - ref_name { - include = ["~ALL"] - exclude = [] - } repository_name { - include = ["~ALL"] + include = ["~ALL"] exclude = [] } } rules { + # Push targets only support these rules: + # file_path_restriction, max_file_size, max_file_path_length, file_extension_restriction file_path_restriction { restricted_file_paths = [".github/workflows/*", "*.env"] } @@ -114,12 +113,14 @@ resource "github_organization_ruleset" "example_push" { * `bypass_actors` - (Optional) (Block List) The actors that can bypass the rules in this ruleset. (see [below for nested schema](#bypass_actors)) -* `conditions` - (Optional) (Block List, Max: 1) Parameters for an organization ruleset condition. `ref_name` is required alongside one of `repository_name` or `repository_id`. (see [below for nested schema](#conditions)) +* `conditions` - (Optional) (Block List, Max: 1) Parameters for an organization ruleset condition. For `branch` and `tag` targets, `ref_name` is required alongside one of `repository_name` or `repository_id`. For `push` targets, `ref_name` must NOT be set - only `repository_name` or `repository_id` should be used. (see [below for nested schema](#conditions)) #### Rules #### The `rules` block supports the following: +~> **Note:** Rules are target-specific. `branch` and `tag` targets support rules like `creation`, `deletion`, `pull_request`, `required_status_checks`, etc. `push` targets only support `file_path_restriction`, `max_file_size`, `max_file_path_length`, and `file_extension_restriction`. Using the wrong rules for a target will result in a validation error. + * `branch_name_pattern` - (Optional) (Block List, Max: 1) Parameters to be used for the branch_name_pattern rule. This rule only applies to repositories within an enterprise, it cannot be applied to repositories owned by individuals or regular organizations. Conflicts with `tag_name_pattern` as it only applies to rulesets with target `branch`. (see [below for nested schema](#rules.branch_name_pattern)) * `commit_author_email_pattern` - (Optional) (Block List, Max: 1) Parameters to be used for the commit_author_email_pattern rule. This rule only applies to repositories within an enterprise, it cannot be applied to repositories owned by individuals or regular organizations. (see [below for nested schema](#rules.commit_author_email_pattern)) @@ -296,12 +297,14 @@ The `rules` block supports the following: #### conditions #### -* `ref_name` - (Required) (Block List, Min: 1, Max: 1) (see [below for nested schema](#conditions.ref_name)) +* `ref_name` - (Optional) (Block List, Max: 1) Required for `branch` and `tag` targets. Must NOT be set for `push` targets. (see [below for nested schema](#conditions.ref_name)) * `repository_id` (Optional) (List of Number) The repository IDs that the ruleset applies to. One of these IDs must match for the condition to pass. Conflicts with `repository_name`. * `repository_name` (Optional) (Block List, Max: 1) Conflicts with `repository_id`. (see [below for nested schema](#conditions.repository_name)) One of `repository_id` and `repository_name` must be set for the rule to target any repositories. +~> **Note:** For `push` targets, do not include `ref_name` in conditions. Push rulesets operate on file content, not on refs. + #### conditions.ref_name #### * `exclude` - (Required) (List of String) Array of ref names or patterns to exclude. The condition will not pass if any of these patterns match. diff --git a/website/docs/r/repository_ruleset.html.markdown b/website/docs/r/repository_ruleset.html.markdown index 12fbc88b45..961a2b42d2 100644 --- a/website/docs/r/repository_ruleset.html.markdown +++ b/website/docs/r/repository_ruleset.html.markdown @@ -98,7 +98,7 @@ resource "github_repository_ruleset" "example_push" { * `bypass_actors` - (Optional) (Block List) The actors that can bypass the rules in this ruleset. (see [below for nested schema](#bypass_actors)) -* `conditions` - (Optional) (Block List, Max: 1) Parameters for a repository ruleset ref name condition. (see [below for nested schema](#conditions)) +* `conditions` - (Optional) (Block List, Max: 1) Parameters for a repository ruleset condition. For `branch` and `tag` targets, `ref_name` is required. For `push` targets, `ref_name` must NOT be set - conditions are optional for push targets. (see [below for nested schema](#conditions)) * `repository` - (Required) (String) Name of the repository to apply ruleset to. @@ -106,6 +106,8 @@ resource "github_repository_ruleset" "example_push" { The `rules` block supports the following: +~> **Note:** Rules are target-specific. `branch` and `tag` targets support rules like `creation`, `deletion`, `pull_request`, `required_status_checks`, etc. `push` targets only support `file_path_restriction`, `max_file_size`, `max_file_path_length`, and `file_extension_restriction`. Using the wrong rules for a target will result in a validation error. + * `branch_name_pattern` - (Optional) (Block List, Max: 1) Parameters to be used for the branch_name_pattern rule. This rule only applies to repositories within an enterprise, it cannot be applied to repositories owned by individuals or regular organizations. Conflicts with `tag_name_pattern` as it only applied to rulesets with target `branch`. (see [below for nested schema](#rulesbranch_name_pattern)) * `commit_author_email_pattern` - (Optional) (Block List, Max: 1) Parameters to be used for the commit_author_email_pattern rule. This rule only applies to repositories within an enterprise, it cannot be applied to repositories owned by individuals or regular organizations. (see [below for nested schema](#rulescommit_author_email_pattern)) @@ -136,9 +138,9 @@ The `rules` block supports the following: * `required_code_scanning` - (Optional) (Block List, Max: 1) Define which tools must provide code scanning results before the reference is updated. When configured, code scanning must be enabled and have results for both the commit and the reference being updated. Multiple code scanning tools can be specified. (see [below for nested schema](#rulesrequired_code_scanning)) -* `file_path_restriction` - (Optional) (Block List, Max 1) Parameters to be used for the file_path_restriction rule. When enabled restricts access to files within the repository. (See [below for nested schema](#rules.file_path_restriction)) +* `file_path_restriction` - (Optional) (Block List, Max 1) Parameters to be used for the file_path_restriction rule. This rule only applies to rulesets with target `push`. (See [below for nested schema](#rules.file_path_restriction)) -* `max_file_size` - (Optional) (Block List, Max 1) Parameters to be used for the max_file_size rule. When enabled restricts the maximum size of a file that can be pushed to the repository. (See [below for nested schema](#rules.max_file_size)) +* `max_file_size` - (Optional) (Block List, Max 1) Parameters to be used for the max_file_size rule. This rule only applies to rulesets with target `push`. (See [below for nested schema](#rules.max_file_size)) * `max_file_path_length` - (Optional) (Block List, Max: 1) Prevent commits that include file paths that exceed a specified character limit from being pushed to the commit graph. This rule only applies to rulesets with target `push`. (see [below for nested schema](#rules.max_file_path_length)) @@ -289,7 +291,9 @@ The `rules` block supports the following: #### conditions #### -* `ref_name` - (Required) (Block List, Min: 1, Max: 1) (see [below for nested schema](#conditions.ref_name)) +* `ref_name` - (Optional) (Block List, Max: 1) Required for `branch` and `tag` targets. Must NOT be set for `push` targets. (see [below for nested schema](#conditions.ref_name)) + +~> **Note:** For `push` targets, do not include `ref_name` in conditions. Push rulesets operate on file content, not on refs. The `conditions` block is optional for push targets. #### conditions.ref_name ####