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/resource_github_organization_ruleset.go b/github/resource_github_organization_ruleset.go index c0a027dc67..e6884b295f 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, + validateRulesFieldBasedOnTarget, + ), + 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,38 @@ 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 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 +733,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 +742,167 @@ func resourceGithubOrganizationRulesetImport(d *schema.ResourceData, meta any) ( return []*schema.ResourceData{d}, nil } + +func validateConditionsFieldForBranchAndTagTargets(ctx context.Context, d *schema.ResourceDiff, _ any) error { + target := d.Get("target").(string) + conditions := d.Get("conditions").([]any)[0].(map[string]any) + tflog.Debug(ctx, "Validating conditions field for branch and tag targets", map[string]any{"target": target, "conditions": conditions}) + if conditions["ref_name"] == nil || len(conditions["ref_name"].([]any)) == 0 { + 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) { + return fmt.Errorf("either repository_name or repository_id must be set for %s target", target) + } + return nil +} + +func validateConditionsFieldForPushTarget(ctx context.Context, d *schema.ResourceDiff, _ any) error { + target := d.Get("target").(string) + conditions := d.Get("conditions").([]any)[0].(map[string]any) + tflog.Debug(ctx, "Validating conditions field for push target", map[string]any{"target": target, "conditions": conditions}) + + if conditions["ref_name"] != nil && len(conditions["ref_name"].([]any)) > 0 { + return fmt.Errorf("ref_name must not be set for %s target", target) + } + return 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 + } + + switch target { + case "branch", "tag": + return validateConditionsFieldForBranchAndTagTargets(ctx, d, meta) + case "push": + return validateConditionsFieldForPushTarget(ctx, d, meta) + } + return nil +} + +// 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", +} + +// 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 validateRulesForPushTarget(ctx context.Context, d *schema.ResourceDiff, _ any) error { + tflog.Debug(ctx, "Validating rules for push target") + rulesRaw := d.Get("rules").([]any) + if len(rulesRaw) == 0 { + tflog.Debug(ctx, "No rules block, skipping validation") + return nil + } + + rules := rulesRaw[0].(map[string]any) + + for _, ruleName := range branchTagOnlyRules { + ruleValue := rules[ruleName] + if ruleValue == nil { + continue + } + switch v := ruleValue.(type) { + case bool: + if v { + tflog.Debug(ctx, "Invalid rule for push target", map[string]any{"rule": ruleName, "value": v}) + return fmt.Errorf("rule %q is not valid for push target; push targets only support: %v", ruleName, pushOnlyRules) + } + case []any: + if len(v) > 0 { + tflog.Debug(ctx, "Invalid rule for push target", map[string]any{"rule": ruleName, "value": v}) + return fmt.Errorf("rule %q is not valid for push target; push targets only support: %v", ruleName, pushOnlyRules) + } + } + } + tflog.Debug(ctx, "Rules validation passed for push target") + return nil +} + +func validateRulesForBranchAndTagTargets(ctx context.Context, d *schema.ResourceDiff, _ any) error { + target := d.Get("target").(string) + tflog.Debug(ctx, "Validating rules for branch/tag 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 + } + + rules := rulesRaw[0].(map[string]any) + + for _, ruleName := range pushOnlyRules { + ruleValue := rules[ruleName] + if ruleValue == nil { + continue + } + if ruleList, ok := ruleValue.([]any); ok && len(ruleList) > 0 { + tflog.Debug(ctx, "Invalid rule for branch/tag target", map[string]any{"rule": ruleName, "target": target}) + return fmt.Errorf("rule %q is only valid for push target, not for %s target", ruleName, target) + } + } + tflog.Debug(ctx, "Rules validation passed for branch/tag target", map[string]any{"target": target}) + return nil +} + +func validateRulesFieldBasedOnTarget(ctx context.Context, d *schema.ResourceDiff, meta any) error { + target := d.Get("target").(string) + tflog.Debug(ctx, "Validating rules field based on target", map[string]any{"target": target}) + + switch target { + case "branch", "tag": + return validateRulesForBranchAndTagTargets(ctx, d, meta) + case "push": + return validateRulesForPushTarget(ctx, d, meta) + } + + return nil +} diff --git a/github/resource_github_organization_ruleset_test.go b/github/resource_github_organization_ruleset_test.go index 68e432e688..6af31e8072 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 only 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("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..b0791f1217 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,32 +858,32 @@ 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) if d.Get("archived").(bool) && !d.HasChange("archived") { @@ -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,7 +979,7 @@ 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) } } @@ -990,7 +990,7 @@ func resourceGithubRepositoryUpdate(d *schema.ResourceData, meta any) error { _, 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 { @@ -1004,21 +1004,21 @@ func resourceGithubRepositoryUpdate(d *schema.ResourceData, meta any) error { _, _, 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..298c4e7362 100644 --- a/github/resource_github_repository_ruleset.go +++ b/github/resource_github_repository_ruleset.go @@ -188,6 +188,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, 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