Skip to content

Commit 7fb3bfe

Browse files
Ravio1iRavio1i
committed
refactor(ruleset): simplify validation logic and enhance conditions requirements for repository rulesets
1 parent 056f9e8 commit 7fb3bfe

5 files changed

Lines changed: 23 additions & 67 deletions

‎github/resource_github_enterprise_ruleset.go‎

Lines changed: 14 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -162,11 +162,11 @@ func resourceGithubEnterpriseRuleset() *schema.Resource {
162162
},
163163
},
164164
"repository_name": {
165-
Type: schema.TypeList,
166-
Optional: true,
167-
MaxItems: 1,
168-
Description: "Conditions for repository names that the ruleset targets. Conflicts with `repository_id` and `repository_property`.",
169-
ConflictsWith: []string{"conditions.0.repository_id", "conditions.0.repository_property"},
165+
Type: schema.TypeList,
166+
Optional: true,
167+
MaxItems: 1,
168+
Description: "Conditions for repository names that the ruleset targets. Exactly one of `repository_name`, `repository_id`, or `repository_property` must be set.",
169+
ExactlyOneOf: []string{"conditions.0.repository_name", "conditions.0.repository_id", "conditions.0.repository_property"},
170170
Elem: &schema.Resource{
171171
Schema: map[string]*schema.Schema{
172172
"include": {
@@ -195,20 +195,20 @@ func resourceGithubEnterpriseRuleset() *schema.Resource {
195195
},
196196
},
197197
"repository_id": {
198-
Type: schema.TypeList,
199-
Optional: true,
200-
ConflictsWith: []string{"conditions.0.repository_name", "conditions.0.repository_property"},
201-
Description: "The repository IDs that the ruleset applies to. One of these IDs must match for the condition to pass. Conflicts with `repository_name` and `repository_property`.",
198+
Type: schema.TypeList,
199+
Optional: true,
200+
ExactlyOneOf: []string{"conditions.0.repository_name", "conditions.0.repository_id", "conditions.0.repository_property"},
201+
Description: "The repository IDs that the ruleset applies to. One of these IDs must match for the condition to pass. Exactly one of `repository_name`, `repository_id`, or `repository_property` must be set.",
202202
Elem: &schema.Schema{
203203
Type: schema.TypeInt,
204204
},
205205
},
206206
"repository_property": {
207-
Type: schema.TypeList,
208-
Optional: true,
209-
MaxItems: 1,
210-
Description: "Conditions based on repository properties. Conflicts with `repository_name` and `repository_id`.",
211-
ConflictsWith: []string{"conditions.0.repository_name", "conditions.0.repository_id"},
207+
Type: schema.TypeList,
208+
Optional: true,
209+
MaxItems: 1,
210+
Description: "Conditions based on repository properties. Exactly one of `repository_name`, `repository_id`, or `repository_property` must be set.",
211+
ExactlyOneOf: []string{"conditions.0.repository_name", "conditions.0.repository_id", "conditions.0.repository_property"},
212212
Elem: &schema.Resource{
213213
Schema: map[string]*schema.Schema{
214214
"include": {
@@ -878,9 +878,6 @@ func resourceGithubEnterpriseRulesetRead(ctx context.Context, d *schema.Resource
878878
return diag.FromErr(err)
879879
}
880880

881-
if err := d.Set("ruleset_id", ruleset.ID); err != nil {
882-
return diag.FromErr(err)
883-
}
884881
if err := d.Set("name", ruleset.Name); err != nil {
885882
return diag.FromErr(err)
886883
}

‎github/resource_github_organization_ruleset.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -952,7 +952,7 @@ func resourceGithubOrganizationRulesetImport(ctx context.Context, d *schema.Reso
952952
}
953953

954954
func resourceGithubOrganizationRulesetDiff(ctx context.Context, d *schema.ResourceDiff, _ any) error {
955-
err := validateRulesetConditions(ctx, d, true)
955+
err := validateRulesetConditions(ctx, d)
956956
if err != nil {
957957
return err
958958
}

‎github/resource_github_repository_ruleset.go‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -884,7 +884,7 @@ func resourceGithubRepositoryRulesetImport(ctx context.Context, d *schema.Resour
884884
}
885885

886886
func resourceGithubRepositoryRulesetDiff(ctx context.Context, d *schema.ResourceDiff, meta any) error {
887-
err := validateRulesetConditions(ctx, d, false)
887+
err := validateRulesetConditions(ctx, d)
888888
if err != nil {
889889
return err
890890
}

‎github/util_ruleset_validation.go‎

Lines changed: 4 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -129,7 +129,7 @@ func validateRules(ctx context.Context, d *schema.ResourceDiff, allowedRules []g
129129
return nil
130130
}
131131

132-
func validateRulesetConditions(ctx context.Context, d *schema.ResourceDiff, isOrg bool) error {
132+
func validateRulesetConditions(ctx context.Context, d *schema.ResourceDiff) error {
133133
target := github.RulesetTarget(d.Get("target").(string))
134134
tflog.Debug(ctx, "Validating conditions field based on target", map[string]any{"target": target})
135135
conditionsRaw := d.Get("conditions").([]any)
@@ -143,7 +143,7 @@ func validateRulesetConditions(ctx context.Context, d *schema.ResourceDiff, isOr
143143

144144
switch target {
145145
case github.RulesetTargetBranch, github.RulesetTargetTag:
146-
return validateConditionsFieldForBranchAndTagTargets(ctx, target, conditions, isOrg)
146+
return validateConditionsFieldForBranchAndTagTargets(ctx, target, conditions)
147147
case github.RulesetTargetPush:
148148
return validateConditionsFieldForPushTarget(ctx, conditions)
149149
}
@@ -163,24 +163,14 @@ func validateRulesetRules(ctx context.Context, d *schema.ResourceDiff) error {
163163
return validateRulesForTarget(ctx, d)
164164
}
165165

166-
func validateConditionsFieldForBranchAndTagTargets(ctx context.Context, target github.RulesetTarget, conditions map[string]any, isOrg bool) error {
167-
tflog.Debug(ctx, fmt.Sprintf("Validating conditions field for %s target", target), map[string]any{"target": target, "conditions": conditions, "isOrg": isOrg})
166+
func validateConditionsFieldForBranchAndTagTargets(ctx context.Context, target github.RulesetTarget, conditions map[string]any) error {
167+
tflog.Debug(ctx, fmt.Sprintf("Validating conditions field for %s target", target), map[string]any{"target": target, "conditions": conditions})
168168

169169
if conditions["ref_name"] == nil || len(conditions["ref_name"].([]any)) == 0 {
170170
tflog.Debug(ctx, fmt.Sprintf("Missing ref_name for %s target", target), map[string]any{"target": target})
171171
return fmt.Errorf("ref_name must be set for %s target", target)
172172
}
173173

174-
// Repository rulesets don't have repository_name or repository_id, 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-
if !hasRepoName && !hasRepoID && !hasRepoProp {
180-
tflog.Debug(ctx, fmt.Sprintf("Missing repository_name, repository_id, or repository_property for %s target", target), map[string]any{"target": target})
181-
return fmt.Errorf("either repository_name, repository_id, or repository_property must be set for %s target", target)
182-
}
183-
}
184174
tflog.Debug(ctx, fmt.Sprintf("Conditions validation passed for %s target", target))
185175
return nil
186176
}

‎github/util_ruleset_validation_test.go‎

Lines changed: 3 additions & 34 deletions
Original file line numberDiff line numberDiff line change
@@ -114,7 +114,7 @@ func Test_validateRepositoryRulesetConditionsFieldForBranchAndTagTargets(t *test
114114

115115
for _, tt := range tests {
116116
t.Run(tt.name, func(t *testing.T) {
117-
err := validateConditionsFieldForBranchAndTagTargets(t.Context(), tt.target, tt.conditions, false)
117+
err := validateConditionsFieldForBranchAndTagTargets(t.Context(), tt.target, tt.conditions)
118118
if tt.expectError {
119119
if err == nil {
120120
t.Errorf("expected error but got nil")
@@ -130,7 +130,7 @@ func Test_validateRepositoryRulesetConditionsFieldForBranchAndTagTargets(t *test
130130
}
131131
}
132132

133-
func Test_validateConditionsFieldForBranchAndTagTargets(t *testing.T) {
133+
func Test_validateConditionsFieldForBranchAndTagTargets_OrgLevel(t *testing.T) {
134134
tests := []struct {
135135
name string
136136
target github.RulesetTarget
@@ -165,42 +165,11 @@ func Test_validateConditionsFieldForBranchAndTagTargets(t *testing.T) {
165165
expectError: true,
166166
errorMsg: "ref_name must be set for branch target",
167167
},
168-
{
169-
name: "invalid branch target without repository_name or repository_id",
170-
target: github.RulesetTargetBranch,
171-
conditions: map[string]any{
172-
"ref_name": []any{map[string]any{"include": []any{"~DEFAULT_BRANCH"}, "exclude": []any{}}},
173-
},
174-
expectError: true,
175-
errorMsg: "either repository_name or repository_id must be set for branch target",
176-
},
177-
{
178-
name: "invalid tag target with nil repository_name and repository_id",
179-
target: github.RulesetTargetTag,
180-
conditions: map[string]any{
181-
"ref_name": []any{map[string]any{"include": []any{"v*"}, "exclude": []any{}}},
182-
"repository_name": nil,
183-
"repository_id": nil,
184-
},
185-
expectError: true,
186-
errorMsg: "either repository_name or repository_id must be set for tag target",
187-
},
188-
{
189-
name: "invalid branch target with empty repository_name and repository_id slices",
190-
target: github.RulesetTargetBranch,
191-
conditions: map[string]any{
192-
"ref_name": []any{map[string]any{"include": []any{"~DEFAULT_BRANCH"}, "exclude": []any{}}},
193-
"repository_name": []any{},
194-
"repository_id": []any{},
195-
},
196-
expectError: true,
197-
errorMsg: "either repository_name or repository_id must be set for branch target",
198-
},
199168
}
200169

201170
for _, tt := range tests {
202171
t.Run(tt.name, func(t *testing.T) {
203-
err := validateConditionsFieldForBranchAndTagTargets(t.Context(), tt.target, tt.conditions, true)
172+
err := validateConditionsFieldForBranchAndTagTargets(t.Context(), tt.target, tt.conditions)
204173
if tt.expectError {
205174
if err == nil {
206175
t.Errorf("expected error but got nil")

0 commit comments

Comments
 (0)