Skip to content

Commit 8330d82

Browse files
Moser-ssMoser-ss
committed
Replace custom validation with built-in ExactlyOneOf for repo targeting
Add ExactlyOneOf/AtLeastOneOf to repository_property, repository_name,repository_id fields. Remove manual validation counting in util_ruleset_validation.Add 3 validation tests for single/multiple/missing repo targeting options. Addresses PR #2356 review feedback - simplifies validation using schema constraints.
1 parent 3ca9fe1 commit 8330d82

3 files changed

Lines changed: 135 additions & 34 deletions

File tree

‎github/resource_github_organization_ruleset.go‎

Lines changed: 19 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -124,10 +124,12 @@ func resourceGithubOrganizationRuleset() *schema.Resource {
124124
},
125125
},
126126
"repository_property": {
127-
Type: schema.TypeList,
128-
Optional: true,
129-
MaxItems: 1,
130-
Description: "Conditions to target repositories by custom or system properties.",
127+
Type: schema.TypeList,
128+
Optional: true,
129+
MaxItems: 1,
130+
ExactlyOneOf: []string{"conditions.0.repository_property", "conditions.0.repository_name", "conditions.0.repository_id"},
131+
AtLeastOneOf: []string{"conditions.0.repository_property", "conditions.0.repository_name", "conditions.0.repository_id"},
132+
Description: "Conditions to target repositories by custom or system properties.",
131133
Elem: &schema.Resource{
132134
Schema: map[string]*schema.Schema{
133135
"include": {
@@ -154,7 +156,7 @@ func resourceGithubOrganizationRuleset() *schema.Resource {
154156
Optional: true,
155157
Description: "The source of the repository property. Defaults to 'custom' if not specified. Can be one of: custom, system",
156158
Default: "custom",
157-
ValidateFunc: validation.StringInSlice([]string{"custom", "system"}, false),
159+
ValidateDiagFunc: validation.ToDiagFunc(validation.StringInSlice([]string{"custom", "system"}, false)),
158160
},
159161
},
160162
},
@@ -183,7 +185,7 @@ func resourceGithubOrganizationRuleset() *schema.Resource {
183185
Optional: true,
184186
Description: "The source of the repository property. Defaults to 'custom' if not specified. Can be one of: custom, system",
185187
Default: "custom",
186-
ValidateFunc: validation.StringInSlice([]string{"custom", "system"}, false),
188+
ValidateDiagFunc: validation.ToDiagFunc(validation.StringInSlice([]string{"custom", "system"}, false)),
187189
},
188190
},
189191
},
@@ -192,10 +194,12 @@ func resourceGithubOrganizationRuleset() *schema.Resource {
192194
},
193195
},
194196
"repository_name": {
195-
Type: schema.TypeList,
196-
Optional: true,
197-
MaxItems: 1,
198-
Description: "Targets repositories that match the specified name patterns.",
197+
Type: schema.TypeList,
198+
Optional: true,
199+
MaxItems: 1,
200+
ExactlyOneOf: []string{"conditions.0.repository_property", "conditions.0.repository_name", "conditions.0.repository_id"},
201+
AtLeastOneOf: []string{"conditions.0.repository_property", "conditions.0.repository_name", "conditions.0.repository_id"},
202+
Description: "Targets repositories that match the specified name patterns.",
199203
Elem: &schema.Resource{
200204
Schema: map[string]*schema.Schema{
201205
"include": {
@@ -224,9 +228,11 @@ func resourceGithubOrganizationRuleset() *schema.Resource {
224228
},
225229
},
226230
"repository_id": {
227-
Type: schema.TypeList,
228-
Optional: true,
229-
Description: "The repository IDs that the ruleset applies to. One of these IDs must match for the condition to pass.",
231+
Type: schema.TypeList,
232+
Optional: true,
233+
ExactlyOneOf: []string{"conditions.0.repository_property", "conditions.0.repository_name", "conditions.0.repository_id"},
234+
AtLeastOneOf: []string{"conditions.0.repository_property", "conditions.0.repository_name", "conditions.0.repository_id"},
235+
Description: "The repository IDs that the ruleset applies to. One of these IDs must match for the condition to pass.",
230236
Elem: &schema.Schema{
231237
Type: schema.TypeInt,
232238
},

‎github/resource_github_organization_ruleset_test.go‎

Lines changed: 114 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -738,6 +738,120 @@ resource "github_organization_ruleset" "test" {
738738
})
739739
})
740740

741+
t.Run("validates_conditions_require_exactly_one_repository_targeting", func(t *testing.T) {
742+
randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
743+
resourceName := "test-multiple-repo-targeting"
744+
config := fmt.Sprintf(`
745+
resource "github_organization_ruleset" "%s" {
746+
name = "test-multiple-targeting-%s"
747+
target = "branch"
748+
enforcement = "active"
749+
750+
conditions {
751+
ref_name {
752+
include = ["~ALL"]
753+
exclude = []
754+
}
755+
repository_name {
756+
include = ["~ALL"]
757+
exclude = []
758+
}
759+
repository_id = [123]
760+
}
761+
762+
rules {
763+
creation = true
764+
}
765+
}
766+
`, resourceName, randomID)
767+
768+
resource.Test(t, resource.TestCase{
769+
PreCheck: func() { skipUnlessHasPaidOrgs(t) },
770+
ProviderFactories: providerFactories,
771+
Steps: []resource.TestStep{
772+
{
773+
Config: config,
774+
ExpectError: regexp.MustCompile("only one of `conditions.0.repository_id,conditions.0.repository_name,conditions.0.repository_property` can be specified"),
775+
},
776+
},
777+
})
778+
})
779+
780+
t.Run("validates_conditions_require_at_least_one_repository_targeting", func(t *testing.T) {
781+
randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
782+
resourceName := "test-no-repo-targeting"
783+
config := fmt.Sprintf(`
784+
resource "github_organization_ruleset" "%s" {
785+
name = "test-no-targeting-%s"
786+
target = "branch"
787+
enforcement = "active"
788+
789+
conditions {
790+
ref_name {
791+
include = ["~ALL"]
792+
exclude = []
793+
}
794+
}
795+
796+
rules {
797+
creation = true
798+
}
799+
}
800+
`, resourceName, randomID)
801+
802+
resource.Test(t, resource.TestCase{
803+
PreCheck: func() { skipUnlessHasPaidOrgs(t) },
804+
ProviderFactories: providerFactories,
805+
Steps: []resource.TestStep{
806+
{
807+
Config: config,
808+
ExpectError: regexp.MustCompile("one of `conditions.0.repository_id,conditions.0.repository_name,conditions.0.repository_property` must be specified"),
809+
},
810+
},
811+
})
812+
})
813+
814+
t.Run("validates_repository_property_works_as_single_targeting_option", func(t *testing.T) {
815+
randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
816+
resourceName := "test-repo-property-targeting"
817+
config := fmt.Sprintf(`
818+
resource "github_organization_ruleset" "%s" {
819+
name = "test-property-targeting-%s"
820+
target = "branch"
821+
enforcement = "active"
822+
823+
conditions {
824+
ref_name {
825+
include = ["~ALL"]
826+
exclude = []
827+
}
828+
repository_property {
829+
include {
830+
name = "environment"
831+
property_values = ["production"]
832+
source = "custom"
833+
}
834+
}
835+
}
836+
837+
rules {
838+
creation = true
839+
}
840+
}
841+
`, resourceName, randomID)
842+
843+
resource.Test(t, resource.TestCase{
844+
PreCheck: func() { skipUnlessHasPaidOrgs(t) },
845+
ProviderFactories: providerFactories,
846+
Steps: []resource.TestStep{
847+
{
848+
Config: config,
849+
ExpectError: nil, // This should succeed
850+
},
851+
},
852+
})
853+
})
854+
741855
t.Run("creates_push_ruleset", func(t *testing.T) {
742856
randomID := acctest.RandStringFromCharSet(5, acctest.CharSetAlphaNum)
743857
rulesetName := fmt.Sprintf("%stest-push-%s", testResourcePrefix, randomID)

‎github/util_ruleset_validation.go‎

Lines changed: 2 additions & 21 deletions
Original file line numberDiff line numberDiff line change
@@ -172,27 +172,8 @@ func validateConditionsFieldForBranchAndTagTargets(ctx context.Context, target g
172172
}
173173

174174
// Repository rulesets don't have repository_name, repository_id, or repository_property - only org rulesets do.
175-
if isOrg {
176-
hasRepoName := conditions["repository_name"] != nil && len(conditions["repository_name"].([]any)) > 0
177-
hasRepoId := conditions["repository_id"] != nil && len(conditions["repository_id"].([]any)) > 0
178-
hasRepoProp := conditions["repository_property"] != nil && len(conditions["repository_property"].([]any)) > 0
179-
180-
repoTargetingCount := 0
181-
if hasRepoName {
182-
repoTargetingCount++
183-
}
184-
if hasRepoId {
185-
repoTargetingCount++
186-
}
187-
if hasRepoProp {
188-
repoTargetingCount++
189-
}
190-
191-
if repoTargetingCount != 1 {
192-
tflog.Debug(ctx, fmt.Sprintf("Invalid repository targeting for %s target", target), map[string]any{"target": target})
193-
return fmt.Errorf("exactly one of repository_name, repository_id, or repository_property must be set for %s target", target)
194-
}
195-
}
175+
// Note: The built-in ExactlyOneOf/AtLeastOneOf schema validation already ensures exactly one
176+
// repository targeting option is set, so no additional validation is needed here.
196177
tflog.Debug(ctx, fmt.Sprintf("Conditions validation passed for %s target", target))
197178
return nil
198179
}

0 commit comments

Comments
 (0)