fix!: Refactor the repository ruleset code #3430

stevehipwell · 2025-01-13T15:23:36Z

This PR refactors the repository ruleset functionality to make is consistent, the following items are the big picture changes.

Consistently refer to repository rulesets as such
Use the same RepositoryRuleset type in both the REST API and events
Don't return an opaque json.RawMessage field when the actual type is known
Added RepositoryRulesetRules to represent the ruleset rules in a strongly typed way (this could be swapped to []RepositoryRulesetRule with the type data still present behind an any)
Added BranchRules to represent the rules for a branch in a strongly typed way (this could be swapped to []BranchRule with the type data still present behind an any)
Fixed event repository ruleset rules

BREAKING CHANGES: The following types have been renamed:

Ruleset -> RepositoryRuleset
RulesetLink -> RepositoryRulesetLink
RulesetLinks -> RepositoryRulesetLinks
RulesetRefConditionParameters -> RepositoryRulesetRefConditionParameters
RulesetRepositoryNamesConditionParameters -> RepositoryRulesetRepositoryNamesConditionParameters
RulesetRepositoryIDsConditionParameters -> RepositoryRulesetRepositoryIDsConditionParameters
RulesetRepositoryPropertyTargetParameters -> Repository
RulesetRepositoryPropertyConditionParameters -> RepositoryRulesetRepositoryPropertyConditionParameters
RulesetOrganizationNamesConditionParameters -> RepositoryRulesetOrganizationNamesConditionParameters
RulesetOrganizationIDsConditionParameters -> RepositoryRulesetOrganizationIDsConditionParameters
RulesetConditions -> RepositoryRulesetConditions
RepositoryRulesetEditedChanges -> RepositoryRulesetChanges
RepositoryRulesetEditedSource -> RepositoryRulesetChangeSource
RepositoryRulesetEditedSources -> RepositoryRulesetChangeSources
RepositoryRulesetEditedConditions -> RepositoryRulesetUpdatedConditions
RepositoryRulesetUpdatedConditionsEdited -> RepositoryRulesetUpdatedCondition
RepositoryRulesetEditedRules -> RepositoryRulesetChangedRules
RepositoryRulesetUpdatedRules -> RepositoryRulesetUpdatedRules
RepositoryRulesetEditedRuleChanges -> RepositoryRulesetChangedRule

Fixes: #3429.

@gmlewis this PR adds the the breaking changes in #3417. The event changes have been tested with real world data. I'm happy to discuss any of the changes.

codecov · 2025-01-13T15:32:14Z

Codecov Report

Attention: Patch coverage is 63.43907% with 219 lines in your changes missing coverage. Please review.

Project coverage is 90.98%. Comparing base (1875fe0) to head (7241f1a).
Report is 2 commits behind head on master.

Files with missing lines	Patch %	Lines
github/rules.go	61.51%	146 Missing and 73 partials ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##           master    #3430      +/-   ##
==========================================
- Coverage   92.26%   90.98%   -1.29%     
==========================================
  Files         174      175       +1     
  Lines       15023    15282     +259     
==========================================
+ Hits        13861    13904      +43     
- Misses       1068     1211     +143     
- Partials       94      167      +73

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

gmlewis · 2025-01-13T15:33:34Z

Thank you, @stevehipwell !
Please run ./script/generate.sh and push the changes to this PR.

stevehipwell · 2025-01-13T15:51:50Z

@gmlewis sorry I thought I had run it, but I was making change while reviewing the PR diff so I guess I missed running it for one of the changes. Should be right now.

gmlewis

Thank you for doing this, @stevehipwell - this cleanup is looking really nice!
Please remember to not force-push, as that will save me a huge amount of time after you address my comments... thanks again!

github/event_types.go

github/rules.go

github/repos_rules.go

github/rules.go

stevehipwell · 2025-01-13T20:02:17Z

@gmlewis I've pushed the changes you requested, and remembered to not force push! 🥳

gmlewis

Thank you, @stevehipwell !
Just a few more minor concerns, please, if you don't mind.

github/rules.go

stevehipwell · 2025-01-14T11:20:26Z

@gmlewis is there a reason why we're not making the update functions (e.g. UpdateRuleset) do an actual update with all fields rather than supporting patching as the default behaviour? I know the APIs can function like this, but we don't have to use them that way. When I see UpdateRuleset I expect the ruleset to look like the payload after a successful apply. Then if patch functionality is required, we could add PatchRuleset as additional functionality.

gmlewis · 2025-01-14T13:58:58Z

@gmlewis is there a reason why we're not making the update functions (e.g. UpdateRuleset) do an actual update with all fields rather than supporting patching as the default behaviour? I know the APIs can function like this, but we don't have to use them that way. When I see UpdateRuleset I expect the ruleset to look like the payload after a successful apply. Then if patch functionality is required, we could add PatchRuleset as additional functionality.

I'm betting that the original endpoints were poorly named and I didn't catch it.

Since this is a fairly significant breaking change, I would say that now is the time in this PR to make the method names actually describe what they are doing.

So feel free to make them consistent in your renaming.

stevehipwell · 2025-01-14T14:27:53Z

@gmlewis is there a reason why we're not making the update functions (e.g. UpdateRuleset) do an actual update with all fields rather than supporting patching as the default behaviour? I know the APIs can function like this, but we don't have to use them that way. When I see UpdateRuleset I expect the ruleset to look like the payload after a successful apply. Then if patch functionality is required, we could add PatchRuleset as additional functionality.

I'm betting that the original endpoints were poorly named and I didn't catch it.

Since this is a fairly significant breaking change, I would say that now is the time in this PR to make the method names actually describe what they are doing.

So feel free to make them consistent in your renaming.

Would this be better as a separate but linked PR? I expect to change the behaviour of the update functions and add patch functions. The standard struct would lose the omitempty annotations and a new Patch* struct would be introduced for the patch method with the omitempty annotations. We could then remove the custom functions to unset specific fields.

gmlewis · 2025-01-14T15:08:02Z

Would this be better as a separate but linked PR? I expect to change the behaviour of the update functions and add patch functions. The standard struct would lose the omitempty annotations and a new Patch* struct would be introduced for the patch method with the omitempty annotations. We could then remove the custom functions to unset specific fields.

Absolutely, if you don't mind... that sounds great. Thank you, @stevehipwell !

github/rules.go

gmlewis

Thank you, @stevehipwell and thanks also for your patience with my requests for changes... I really appreciate it!
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

stevehipwell · 2025-01-20T11:40:45Z

@gmlewis and suggestions for who to ask to give this a second review?

gmlewis · 2025-01-20T12:09:27Z

@gmlewis and suggestions for who to ask to give this a second review?

I currently don't have any suggestions for volunteers and I try not to ping people unless it is really important.
Feel free to reach out to any Go devs you know to come do a code review for you so we can move this along.
We welcome contributions from anyone wanting to help out. Thanks.

stevehipwell · 2025-01-20T13:00:27Z

@gmlewis unfortunately I don't have any access to Go devs. I'll wait until I review a PR and ask for the author to return the favour.

alexandear · 2025-01-20T13:57:45Z

example/verifyartifact/main.go

@@ -109,7 +109,6 @@ func main() {
 		}

 		err := runVerification(sev, pb, b)
-


Could you please be so kind as to revert this unrelated change? It makes the review process more difficult.

You can use these commands:

git restore --source=master example/verifyartifact/main.go git commit -a -m "Revert unrelated changes"

Thank you.

alexandear · 2025-01-20T13:59:18Z

github/code_scanning.go

@@ -391,7 +391,7 @@ func (s *CodeScanningService) UploadSarif(ctx context.Context, owner, repo strin
 		return nil, nil, err
 	}

-	// This will always return an error without unmarshalling the data
+	// This will always return an error without unmarshaling the data


Please revert unrelated change. It's better to create a separate PR that fixes all unmarshalling words altogether.

This change was explicitly requested by @gmlewis.

And AFAIK it does fix all of the related typos.

I created a PR to address this #3441
Would you mind a review?

I approved your PR.

alexandear · 2025-01-20T14:11:14Z

github/enterprise_rules.go

@@ -76,14 +76,14 @@ func (s *EnterpriseService) UpdateEnterpriseRuleset(ctx context.Context, enterpr
 	return rs, resp, nil
 }

-// UpdateEnterpriseRulesetClearBypassActor clears the ruleset bypass actors for a ruleset for the specified repository.
+// UpdateRepositoryRulesetClearBypassActor clears the bypass actors for a repository ruleset for the specified enterprise.
 //
 // This function is necessary as the UpdateEnterpriseRuleset function does not marshal ByPassActor if passed as an empty array.


Perhaps we should change the comment after renaming:

Suggested change

// This function is necessary as the UpdateEnterpriseRuleset function does not marshal ByPassActor if passed as an empty array.

// This function is necessary as the UpdateRepositoryRuleset function does not marshal ByPassActor if passed as an empty array.

Good spot, fixed.

alexandear · 2025-01-20T14:25:42Z

github/event_types_test.go

+		event *RepositoryRulesetEvent
+	}{
+		{"empty", `{}`, &RepositoryRulesetEvent{}},
+		{"created", fmt.Sprintf(`{"action":"created","repository_ruleset":{"id":1,"name":"r","target":"branch","source_type":"Repository","source":"o/r","enforcement":"active","conditions":{"ref_name":{"exclude":[],"include":["~ALL"]}},"rules":[{"type":"deletion"},{"type":"creation"},{"type":"update"},{"type":"required_linear_history"},{"type":"pull_request","parameters":{"required_approving_review_count":2,"dismiss_stale_reviews_on_push":false,"require_code_owner_review":false,"require_last_push_approval":false,"required_review_thread_resolution":false,"automatic_copilot_code_review_enabled":false,"allowed_merge_methods":["squash","rebase","merge"]}},{"type":"code_scanning","parameters":{"code_scanning_tools":[{"tool":"CodeQL","security_alerts_threshold":"high_or_higher","alerts_threshold":"errors"}]}}],"node_id":"n","created_at":%[1]s,"updated_at":%[1]s,"_links":{"self":{"href":"a"},"html":{"href":"a"}}},"repository":{"id":1,"node_id":"n","name":"r","full_name":"o/r"},"organization":{"id":1,"node_id":"n","name":"o"},"enterprise":{"id":1,"node_id":"n","slug":"e","name":"e"},"installation":{"id":1,"node_id":"n","app_id":1,"app_slug":"a"},"sender":{"id":1,"node_id":"n","login":"l"}}`, referenceTimeStr), &RepositoryRulesetEvent{Action: Ptr("created"), RepositoryRuleset: &RepositoryRuleset{ID: Ptr(int64(1)), Name: "r", Target: Ptr(RulesetTargetBranch), SourceType: Ptr(RulesetSourceTypeRepository), Source: "o/r", Enforcement: RulesetEnforcementActive, Conditions: &RepositoryRulesetConditions{RefName: &RepositoryRulesetRefConditionParameters{Include: []string{"~ALL"}, Exclude: []string{}}}, Rules: &RepositoryRulesetRules{Creation: &EmptyRuleParameters{}, Update: &UpdateRuleParameters{}, Deletion: &EmptyRuleParameters{}, RequiredLinearHistory: &EmptyRuleParameters{}, PullRequest: &PullRequestRuleParameters{AllowedMergeMethods: []MergeMethod{MergeMethodSquash, MergeMethodRebase, MergeMethodMerge}, DismissStaleReviewsOnPush: false, RequireCodeOwnerReview: false, RequireLastPushApproval: false, RequiredApprovingReviewCount: 2, RequiredReviewThreadResolution: false}, CodeScanning: &CodeScanningRuleParameters{CodeScanningTools: []*RuleCodeScanningTool{{AlertsThreshold: CodeScanningAlertsThresholdErrors, SecurityAlertsThreshold: CodeScanningSecurityAlertsThresholdHighOrHigher, Tool: "CodeQL"}}}}, NodeID: Ptr("n"), CreatedAt: &Timestamp{referenceTime}, UpdatedAt: &Timestamp{referenceTime}, Links: &RepositoryRulesetLinks{Self: &RepositoryRulesetLink{HRef: Ptr("a")}, HTML: &RepositoryRulesetLink{HRef: Ptr("a")}}}, Repository: repository, Organization: organization, Enterprise: enterprise, Installation: installation, Sender: sender}},


This line contains 2668 characters, which may make it difficult to edit in the future as fields change. Could you please format the JSON as it is done in the other tests in this file?

@alexandear I'm going to have to disagree with you on this. As the tests should be tightly coupled with the JSON string it makes the most sense to make the JSON format as clear and unambiguous as possible. Therefore the compact representation is the obvious choice. If you want to edit the JSON it's going to be much easier to do so by copying the string and working on it as actual JSON (jq -c '.' will then format the JSON to be pasted back).

Please note that this test doesn't use testJSONMarshal like the other tests; that helper function can (and does) mask incorrect JSON by depending on the very marshaling code that's under test to format the JSON string.

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

alexandear · 2025-01-20T16:35:35Z

github/rules_test.go

+		{"empty", &RepositoryRulesetRules{}, `[]`},
+		{"single_rule_with_empty_params", &RepositoryRulesetRules{Creation: &EmptyRuleParameters{}}, `[{"type":"creation"}]`},
+		{"single_rule_with_required_params", &RepositoryRulesetRules{RequiredDeployments: &RequiredDeploymentsRuleParameters{RequiredDeploymentEnvironments: []string{"test"}}}, `[{"type":"required_deployments","parameters":{"required_deployment_environments":["test"]}}]`},
+		{"all_rules_with_required_params", &RepositoryRulesetRules{Creation: &EmptyRuleParameters{}, Update: &UpdateRuleParameters{}, Deletion: &EmptyRuleParameters{}, RequiredLinearHistory: &EmptyRuleParameters{}, MergeQueue: &MergeQueueRuleParameters{CheckResponseTimeoutMinutes: 5, GroupingStrategy: MergeGroupingStrategyAllGreen, MaxEntriesToBuild: 10, MaxEntriesToMerge: 20, MergeMethod: MergeMethodSquash, MinEntriesToMerge: 1, MinEntriesToMergeWaitMinutes: 15}, RequiredDeployments: &RequiredDeploymentsRuleParameters{RequiredDeploymentEnvironments: []string{"test1", "test2"}}, RequiredSignatures: &EmptyRuleParameters{}, PullRequest: &PullRequestRuleParameters{AllowedMergeMethods: []MergeMethod{MergeMethodSquash, MergeMethodRebase}, DismissStaleReviewsOnPush: true, RequireCodeOwnerReview: true, RequireLastPushApproval: true, RequiredApprovingReviewCount: 2, RequiredReviewThreadResolution: true}, RequiredStatusChecks: &RequiredStatusChecksRuleParameters{RequiredStatusChecks: []*RuleStatusCheck{{Context: "test1"}, {Context: "test2"}}, StrictRequiredStatusChecksPolicy: true}, NonFastForward: &EmptyRuleParameters{}, CommitMessagePattern: &PatternRuleParameters{Operator: PatternRuleOperatorStartsWith, Pattern: "test"}, CommitAuthorEmailPattern: &PatternRuleParameters{Operator: PatternRuleOperatorStartsWith, Pattern: "test"}, CommitterEmailPattern: &PatternRuleParameters{Operator: PatternRuleOperatorStartsWith, Pattern: "test"}, BranchNamePattern: &PatternRuleParameters{Operator: PatternRuleOperatorStartsWith, Pattern: "test"}, TagNamePattern: &PatternRuleParameters{Operator: PatternRuleOperatorStartsWith, Pattern: "test"}, FilePathRestriction: &FilePathRestrictionRuleParameters{RestrictedFilePaths: []string{"test1", "test2"}}, MaxFilePathLength: &MaxFilePathLengthRuleParameters{MaxFilePathLength: 512}, FileExtensionRestriction: &FileExtensionRestrictionRuleParameters{RestrictedFileExtensions: []string{".exe", ".pkg"}}, MaxFileSize: &MaxFileSizeRuleParameters{MaxFileSize: 1024}, Workflows: &WorkflowsRuleParameters{Workflows: []*RuleWorkflow{{Path: ".github/workflows/test1.yaml"}, {Path: ".github/workflows/test2.yaml"}}}, CodeScanning: &CodeScanningRuleParameters{CodeScanningTools: []*RuleCodeScanningTool{{AlertsThreshold: CodeScanningAlertsThresholdAll, SecurityAlertsThreshold: CodeScanningSecurityAlertsThresholdAll, Tool: "test"}, {AlertsThreshold: CodeScanningAlertsThresholdNone, SecurityAlertsThreshold: CodeScanningSecurityAlertsThresholdNone, Tool: "test"}}}}, `[{"type":"creation"},{"type":"update"},{"type":"deletion"},{"type":"required_linear_history"},{"type":"merge_queue","parameters":{"check_response_timeout_minutes":5,"grouping_strategy":"ALLGREEN","max_entries_to_build":10,"max_entries_to_merge":20,"merge_method":"squash","min_entries_to_merge":1,"min_entries_to_merge_wait_minutes":15}},{"type":"required_deployments","parameters":{"required_deployment_environments":["test1","test2"]}},{"type":"required_signatures"},{"type":"pull_request","parameters":{"allowed_merge_methods":["squash","rebase"],"dismiss_stale_reviews_on_push":true,"require_code_owner_review":true,"require_last_push_approval":true,"required_approving_review_count":2,"required_review_thread_resolution":true}},{"type":"required_status_checks","parameters":{"required_status_checks":[{"context":"test1"},{"context":"test2"}],"strict_required_status_checks_policy":true}},{"type":"non_fast_forward"},{"type":"commit_message_pattern","parameters":{"operator":"starts_with","pattern":"test"}},{"type":"commit_author_email_pattern","parameters":{"operator":"starts_with","pattern":"test"}},{"type":"committer_email_pattern","parameters":{"operator":"starts_with","pattern":"test"}},{"type":"branch_name_pattern","parameters":{"operator":"starts_with","pattern":"test"}},{"type":"tag_name_pattern","parameters":{"operator":"starts_with","pattern":"test"}},{"type":"file_path_restriction","parameters":{"restricted_file_paths":["test1","test2"]}},{"type":"max_file_path_length","parameters":{"max_file_path_length":512}},{"type":"file_extension_restriction","parameters":{"restricted_file_extensions":[".exe",".pkg"]}},{"type":"max_file_size","parameters":{"max_file_size":1024}},{"type":"workflows","parameters":{"workflows":[{"path":".github/workflows/test1.yaml"},{"path":".github/workflows/test2.yaml"}]}},{"type":"code_scanning","parameters":{"code_scanning_tools":[{"alerts_threshold":"all","security_alerts_threshold":"all","tool":"test"},{"alerts_threshold":"none","security_alerts_threshold":"none","tool":"test"}]}}]`},


I'm not sure it's a good idea to put everything into one line.

Compare your variant:

{"all_rules_with_all_params", &RepositoryRulesetRules{Creation: &EmptyRuleParameters{}, Update: &UpdateRuleParameters{UpdateAllowsFetchAndMerge: true}, Deletion: &EmptyRuleParameters{}, RequiredLinearHistory: &EmptyRuleParameters{}, MergeQueue: &MergeQueueRuleParameters{CheckResponseTimeoutMinutes: 5, GroupingStrategy: MergeGroupingStrategyAllGreen, MaxEntriesToBuild: 10, MaxEntriesToMerge: 20, MergeMethod: MergeMethodSquash, MinEntriesToMerge: 1, MinEntriesToMergeWaitMinutes: 15}, RequiredDeployments: &RequiredDeploymentsRuleParameters{RequiredDeploymentEnvironments: []string{"test1", "test2"}}, RequiredSignatures: &EmptyRuleParameters{}, PullRequest: &PullRequestRuleParameters{AllowedMergeMethods: []MergeMethod{MergeMethodSquash, MergeMethodRebase}, DismissStaleReviewsOnPush: true, RequireCodeOwnerReview: true, RequireLastPushApproval: true, RequiredApprovingReviewCount: 2, RequiredReviewThreadResolution: true}, RequiredStatusChecks: &RequiredStatusChecksRuleParameters{DoNotEnforceOnCreate: Ptr(true), RequiredStatusChecks: []*RuleStatusCheck{{Context: "test1", IntegrationID: Ptr(int64(1))}, {Context: "test2", IntegrationID: Ptr(int64(2))}}, StrictRequiredStatusChecksPolicy: true}, NonFastForward: &EmptyRuleParameters{}, CommitMessagePattern: &PatternRuleParameters{Name: Ptr("cmp"), Negate: Ptr(false), Operator: PatternRuleOperatorStartsWith, Pattern: "test"}, CommitAuthorEmailPattern: &PatternRuleParameters{Name: Ptr("caep"), Negate: Ptr(false), Operator: PatternRuleOperatorStartsWith, Pattern: "test"}, CommitterEmailPattern: &PatternRuleParameters{Name: Ptr("cep"), Negate: Ptr(false), Operator: PatternRuleOperatorStartsWith, Pattern: "test"}, BranchNamePattern: &PatternRuleParameters{Name: Ptr("bp"), Negate: Ptr(false), Operator: PatternRuleOperatorStartsWith, Pattern: "test"}, TagNamePattern: &PatternRuleParameters{Name: Ptr("tp"), Negate: Ptr(false), Operator: PatternRuleOperatorStartsWith, Pattern: "test"}, FilePathRestriction: &FilePathRestrictionRuleParameters{RestrictedFilePaths: []string{"test1", "test2"}}, MaxFilePathLength: &MaxFilePathLengthRuleParameters{MaxFilePathLength: 512}, FileExtensionRestriction: &FileExtensionRestrictionRuleParameters{RestrictedFileExtensions: []string{".exe", ".pkg"}}, MaxFileSize: &MaxFileSizeRuleParameters{MaxFileSize: 1024}, Workflows: &WorkflowsRuleParameters{DoNotEnforceOnCreate: Ptr(true), Workflows: []*RuleWorkflow{{Path: ".github/workflows/test1.yaml", Ref: Ptr("main"), RepositoryID: Ptr(int64(1)), SHA: Ptr("aaaa")}, {Path: ".github/workflows/test2.yaml", Ref: Ptr("main"), RepositoryID: Ptr(int64(2)), SHA: Ptr("bbbb")}}}, CodeScanning: &CodeScanningRuleParameters{CodeScanningTools: []*RuleCodeScanningTool{{AlertsThreshold: CodeScanningAlertsThresholdAll, SecurityAlertsThreshold: CodeScanningSecurityAlertsThresholdAll, Tool: "test"}, {AlertsThreshold: CodeScanningAlertsThresholdNone, SecurityAlertsThreshold: CodeScanningSecurityAlertsThresholdNone, Tool: "test"}}}}, `[{"type":"creation"},{"type":"update","parameters":{"update_allows_fetch_and_merge":true}},{"type":"deletion"},{"type":"required_linear_history"},{"type":"merge_queue","parameters":{"check_response_timeout_minutes":5,"grouping_strategy":"ALLGREEN","max_entries_to_build":10,"max_entries_to_merge":20,"merge_method":"squash","min_entries_to_merge":1,"min_entries_to_merge_wait_minutes":15}},{"type":"required_deployments","parameters":{"required_deployment_environments":["test1","test2"]}},{"type":"required_signatures"},{"type":"pull_request","parameters":{"allowed_merge_methods":["squash","rebase"],"dismiss_stale_reviews_on_push":true,"require_code_owner_review":true,"require_last_push_approval":true,"required_approving_review_count":2,"required_review_thread_resolution":true}},{"type":"required_status_checks","parameters":{"do_not_enforce_on_create":true,"required_status_checks":[{"context":"test1","integration_id":1},{"context":"test2","integration_id":2}],"strict_required_status_checks_policy":true}},{"type":"non_fast_forward"},{"type":"commit_message_pattern","parameters":{"name":"cmp","negate":false,"operator":"starts_with","pattern":"test"}},{"type":"commit_author_email_pattern","parameters":{"name":"caep","negate":false,"operator":"starts_with","pattern":"test"}},{"type":"committer_email_pattern","parameters":{"name":"cep","negate":false,"operator":"starts_with","pattern":"test"}},{"type":"branch_name_pattern","parameters":{"name":"bp","negate":false,"operator":"starts_with","pattern":"test"}},{"type":"tag_name_pattern","parameters":{"name":"tp","negate":false,"operator":"starts_with","pattern":"test"}},{"type":"file_path_restriction","parameters":{"restricted_file_paths":["test1","test2"]}},{"type":"max_file_path_length","parameters":{"max_file_path_length":512}},{"type":"file_extension_restriction","parameters":{"restricted_file_extensions":[".exe",".pkg"]}},{"type":"max_file_size","parameters":{"max_file_size":1024}},{"type":"workflows","parameters":{"do_not_enforce_on_create":true,"workflows":[{"path":".github/workflows/test1.yaml","ref":"main","repository_id":1,"sha":"aaaa"},{"path":".github/workflows/test2.yaml","ref":"main","repository_id":2,"sha":"bbbb"}]}},{"type":"code_scanning","parameters":{"code_scanning_tools":[{"alerts_threshold":"all","security_alerts_threshold":"all","tool":"test"},{"alerts_threshold":"none","security_alerts_threshold":"none","tool":"test"}]}}]`},

with mine:

{ "all_rules_with_all_params", &RepositoryRulesetRules{ Creation: &EmptyRuleParameters{}, Update: &UpdateRuleParameters{UpdateAllowsFetchAndMerge: true}, Deletion: &EmptyRuleParameters{}, RequiredLinearHistory: &EmptyRuleParameters{}, MergeQueue: &MergeQueueRuleParameters{ CheckResponseTimeoutMinutes: 5, GroupingStrategy: MergeGroupingStrategyAllGreen, MaxEntriesToBuild: 10, MaxEntriesToMerge: 20, MergeMethod: MergeMethodSquash, MinEntriesToMerge: 1, MinEntriesToMergeWaitMinutes: 15, }, RequiredDeployments: &RequiredDeploymentsRuleParameters{ RequiredDeploymentEnvironments: []string{"test1", "test2"}, }, RequiredSignatures: &EmptyRuleParameters{}, PullRequest: &PullRequestRuleParameters{ AllowedMergeMethods: []MergeMethod{ MergeMethodSquash, MergeMethodRebase, }, DismissStaleReviewsOnPush: true, RequireCodeOwnerReview: true, RequireLastPushApproval: true, RequiredApprovingReviewCount: 2, RequiredReviewThreadResolution: true, }, RequiredStatusChecks: &RequiredStatusChecksRuleParameters{ DoNotEnforceOnCreate: Ptr(true), RequiredStatusChecks: []*RuleStatusCheck{ {Context: "test1", IntegrationID: Ptr(int64(1))}, {Context: "test2", IntegrationID: Ptr(int64(2))}, }, StrictRequiredStatusChecksPolicy: true, }, NonFastForward: &EmptyRuleParameters{}, CommitMessagePattern: &PatternRuleParameters{ Name: Ptr("cmp"), Negate: Ptr(false), Operator: PatternRuleOperatorStartsWith, Pattern: "test", }, CommitAuthorEmailPattern: &PatternRuleParameters{ Name: Ptr("caep"), Negate: Ptr(false), Operator: PatternRuleOperatorStartsWith, Pattern: "test", }, CommitterEmailPattern: &PatternRuleParameters{ Name: Ptr("cep"), Negate: Ptr(false), Operator: PatternRuleOperatorStartsWith, Pattern: "test", }, BranchNamePattern: &PatternRuleParameters{ Name: Ptr("bp"), Negate: Ptr(false), Operator: PatternRuleOperatorStartsWith, Pattern: "test", }, TagNamePattern: &PatternRuleParameters{ Name: Ptr("tp"), Negate: Ptr(false), Operator: PatternRuleOperatorStartsWith, Pattern: "test", }, FilePathRestriction: &FilePathRestrictionRuleParameters{ RestrictedFilePaths: []string{"test1", "test2"}, }, MaxFilePathLength: &MaxFilePathLengthRuleParameters{MaxFilePathLength: 512}, FileExtensionRestriction: &FileExtensionRestrictionRuleParameters{ RestrictedFileExtensions: []string{".exe", ".pkg"}, }, MaxFileSize: &MaxFileSizeRuleParameters{MaxFileSize: 1024}, Workflows: &WorkflowsRuleParameters{ DoNotEnforceOnCreate: Ptr(true), Workflows: []*RuleWorkflow{ { Path: ".github/workflows/test1.yaml", Ref: Ptr("main"), RepositoryID: Ptr(int64(1)), SHA: Ptr("aaaa"), }, { Path: ".github/workflows/test2.yaml", Ref: Ptr("main"), RepositoryID: Ptr(int64(2)), SHA: Ptr("bbbb"), }, }, }, CodeScanning: &CodeScanningRuleParameters{ CodeScanningTools: []*RuleCodeScanningTool{ { AlertsThreshold: CodeScanningAlertsThresholdAll, SecurityAlertsThreshold: CodeScanningSecurityAlertsThresholdAll, Tool: "test", }, { AlertsThreshold: CodeScanningAlertsThresholdNone, SecurityAlertsThreshold: CodeScanningSecurityAlertsThresholdNone, Tool: "test", }, }, }, }, `[{"type":"creation"},{"type":"update","parameters":{"update_allows_fetch_and_merge":true}},{"type":"deletion"},{"type":"required_linear_history"},{"type":"merge_queue","parameters":{"check_response_timeout_minutes":5,"grouping_strategy":"ALLGREEN","max_entries_to_build":10,"max_entries_to_merge":20,"merge_method":"squash","min_entries_to_merge":1,"min_entries_to_merge_wait_minutes":15}},{"type":"required_deployments","parameters":{"required_deployment_environments":["test1","test2"]}},{"type":"required_signatures"},{"type":"pull_request","parameters":{"allowed_merge_methods":["squash","rebase"],"dismiss_stale_reviews_on_push":true,"require_code_owner_review":true,"require_last_push_approval":true,"required_approving_review_count":2,"required_review_thread_resolution":true}},{"type":"required_status_checks","parameters":{"do_not_enforce_on_create":true,"required_status_checks":[{"context":"test1","integration_id":1},{"context":"test2","integration_id":2}],"strict_required_status_checks_policy":true}},{"type":"non_fast_forward"},{"type":"commit_message_pattern","parameters":{"name":"cmp","negate":false,"operator":"starts_with","pattern":"test"}},{"type":"commit_author_email_pattern","parameters":{"name":"caep","negate":false,"operator":"starts_with","pattern":"test"}},{"type":"committer_email_pattern","parameters":{"name":"cep","negate":false,"operator":"starts_with","pattern":"test"}},{"type":"branch_name_pattern","parameters":{"name":"bp","negate":false,"operator":"starts_with","pattern":"test"}},{"type":"tag_name_pattern","parameters":{"name":"tp","negate":false,"operator":"starts_with","pattern":"test"}},{"type":"file_path_restriction","parameters":{"restricted_file_paths":["test1","test2"]}},{"type":"max_file_path_length","parameters":{"max_file_path_length":512}},{"type":"file_extension_restriction","parameters":{"restricted_file_extensions":[".exe",".pkg"]}},{"type":"max_file_size","parameters":{"max_file_size":1024}},{"type":"workflows","parameters":{"do_not_enforce_on_create":true,"workflows":[{"path":".github/workflows/test1.yaml","ref":"main","repository_id":1,"sha":"aaaa"},{"path":".github/workflows/test2.yaml","ref":"main","repository_id":2,"sha":"bbbb"}]}},{"type":"code_scanning","parameters":{"code_scanning_tools":[{"alerts_threshold":"all","security_alerts_threshold":"all","tool":"test"},{"alerts_threshold":"none","security_alerts_threshold":"none","tool":"test"}]}}]`, },

You can use golines to automatically format long lines.

For the objects I agree with you, I'll make these multi line.

Done, thanks for the pointer to golines.

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

gmlewis

Thank you, @stevehipwell!
LGTM.

Awaiting second LGTM+Approval from any other contributor to this repo before merging.

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

stevehipwell · 2025-01-21T11:16:42Z

@gmlewis I've added another test to try and improve the code coverage numbers.

alexandear · 2025-01-20T16:42:14Z

github/rules.go

+	// If new rules are added to RulesetRules the capacity needs increasing
+	arr := make([]repositoryRulesetRuleWrapper, 0, 21)


I propose to simplify:

Suggested change

// If new rules are added to RulesetRules the capacity needs increasing

arr := make([]repositoryRulesetRuleWrapper, 0, 21)

var arr []repositoryRulesetRuleWrapper

I'm not sure if we get a significant performance improvement by preallocating the slice here.

The similar for RepositoryRulesetRules.MarshalJSON.

Maybe we can rename arr to wrappers.

arr suggests an array, but []repositoryRulesetRuleWrapper is a slice.

I'm happy to rename arr to wrappers, and I will. However I can't see a benefit for removing the make(), call but I can see a number of downsides including both performance and clarity.

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

stevehipwell · 2025-01-21T12:40:50Z

@gmlewis what are your thoughts on the coverage here? The new untested lines are basically the error block for the JSON marshaling code and adding coverage for these would be very verbose but not impossible.

gmlewis · 2025-01-21T13:11:59Z

@gmlewis what are your thoughts on the coverage here? The new untested lines are basically the error block for the JSON marshaling code and adding coverage for these would be very verbose but not impossible.

Exactly right - that part of the code is marshaling and unmarshaling the JSON so the coverage tests would need to attempt to provide input to the functions with either bad structs (challenging) or bad JSON (e.g. {[) which I don't think is terribly beneficial in the big scheme of things. So I'm perfectly happy to ignore the CodeCov results in this PR. Thanks for asking.

gmlewis · 2025-01-21T13:19:44Z

github/rules.go

@@ -480,177 +480,177 @@ type repositoryRulesetRuleWrapper struct {
 // MarshalJSON is a custom JSON marshaler for RulesetRules.
 func (r *RepositoryRulesetRules) MarshalJSON() ([]byte, error) {
 	// If new rules are added to RulesetRules the capacity needs increasing
-	arr := make([]json.RawMessage, 0, 21)
+	rawRules := make([]json.RawMessage, 0, 21)


Can you please add a comment here as to why 21 was chosen for the capacity, for the benefit of the maintainers and developers?

If it is completely arbitrary, then please remove it entirely, as that is not idiomatic Go.
The Go team has gone to extensive lengths with benchmarking to make the defaults very reasonable, and if you don't have a good reason for the capacity, then it is not only distracting, but makes the code harder to read and understand, which also violates idiomatic Go, as one of the whole points of Go's design is that it should be super-easy to read and understand.

Therefore, if it is arbitrary, it is idiomatic Go to always take advantage of the zero-value of a type when its initial value is not the focus of the code. Therefore, this would simply be:

var rawRules []json.RawMessage

Short, sweet, and does not distract the reader into thinking there is something special going on here.
In fact, the comment on 482 is also distracting and reader-time-consuming, in my opinion, if there is nothing special going on here.

However, once again, if the 21 IS important (it is not obvious to me how it possibly could be), then change the comment on 482 to explain why 21 is important. Otherwise, this is just a magic number:
https://en.wikipedia.org/wiki/Magic_number_(programming)

I'll add a comment, but basically there will always be between 1 and 21 items in the slice due to the GitHub data model. My understanding was that if you had prior knowledge on the size of a Go array/slice that you should explicitly set it? If I'm incorrectly informed in this case I'll happily remove the make() call.

OK, fine, then please change the comment. Thanks.

I've removed 2 of the 3 cases as they were incorrect and improved the comment for the case where I think it's worth setting the capacity.

gmlewis

Requesting change to remove magic number.

gmlewis · 2025-01-21T13:23:10Z

Thank you, @alexandear for your review!

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

stevehipwell · 2025-01-21T15:58:25Z

@gmlewis hopefully this is ready for final review now.

gmlewis

Why aren't your wrappers all slices of pointers to structs?
Unless you have a good reason, they should be.

gmlewis · 2025-01-21T16:31:48Z

github/rules.go

+
+// UnmarshalJSON is a custom JSON unmarshaler for BranchRules.
+func (r *BranchRules) UnmarshalJSON(data []byte) error {
+	var wrappers []branchRuleWrapper


Suggested change

var wrappers []branchRuleWrapper

var wrappers []*branchRuleWrapper

gmlewis · 2025-01-21T16:32:11Z

github/rules.go

+
+// UnmarshalJSON is a custom JSON unmarshaler for RulesetRules.
+func (r *RepositoryRulesetRules) UnmarshalJSON(data []byte) error {
+	var wrappers []repositoryRulesetRuleWrapper


Suggested change

var wrappers []repositoryRulesetRuleWrapper

var wrappers []*repositoryRulesetRuleWrapper

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

gmlewis

Thank you, @stevehipwell and @alexandear!
LGTM
Merging.

gmlewis added NeedsReview PR is awaiting a review before merging. Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). labels Jan 13, 2025

gmlewis changed the title ~~fix!: Refactored the repository ruleset code~~ fix!: Refactor the repository ruleset code Jan 13, 2025

gmlewis requested changes Jan 13, 2025

View reviewed changes

github/rules.go Outdated Show resolved Hide resolved

github/rules.go Outdated Show resolved Hide resolved

github/rules.go Show resolved Hide resolved

github/rules.go Outdated Show resolved Hide resolved

gmlewis reviewed Jan 14, 2025

View reviewed changes

github/rules.go Show resolved Hide resolved

gmlewis approved these changes Jan 14, 2025

View reviewed changes

stevehipwell mentioned this pull request Jan 15, 2025

[MAINT]: actor_id for OrganizationAdmin seems to have changed from 1 to 0 integrations/terraform-provider-github#2536

Open

1 task

alexandear suggested changes Jan 20, 2025

View reviewed changes

stevehipwell mentioned this pull request Jan 20, 2025

Codecov action failing to upload for master due to a missing token #3439

Closed

stevehipwell added 7 commits January 20, 2025 15:36

fix!: Refactored the repository ruleset code

9c5fc32

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

fixup! fix!: Refactored the repository ruleset code

a38ea41

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

fixup! fix!: Refactored the repository ruleset code

152169a

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

fixup! fix!: Refactored the repository ruleset code

959df1e

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

fixup! fix!: Refactored the repository ruleset code

a149d40

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

fixup! fix!: Refactored the repository ruleset code

b9041af

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

fixup! fix!: Refactored the repository ruleset code

a24903b

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

stevehipwell added 2 commits January 20, 2025 15:37

fixup! fix!: Refactored the repository ruleset code

b12b08c

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

fixup! fix!: Refactored the repository ruleset code

6971bef

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

stevehipwell force-pushed the refactor-rules branch from efa2b7d to 6971bef Compare January 20, 2025 15:38

alexandear reviewed Jan 20, 2025

View reviewed changes

stevehipwell and others added 2 commits January 20, 2025 16:45

fixup! fix!: Refactored the repository ruleset code

1f00cfe

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

Merge branch 'master' into refactor-rules

528ce0e

gmlewis approved these changes Jan 20, 2025

View reviewed changes

fixup! fix!: Refactored the repository ruleset code

dffa330

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

alexandear reviewed Jan 21, 2025

View reviewed changes

fixup! fix!: Refactored the repository ruleset code

81b59ee

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

alexandear approved these changes Jan 21, 2025

View reviewed changes

gmlewis reviewed Jan 21, 2025

View reviewed changes

gmlewis requested changes Jan 21, 2025

View reviewed changes

gmlewis removed the NeedsReview PR is awaiting a review before merging. label Jan 21, 2025

stevehipwell added 2 commits January 21, 2025 14:47

fixup! fix!: Refactored the repository ruleset code

49fbce8

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

Merge remote-tracking branch 'upstream/master' into refactor-rules

47d0c77

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

gmlewis requested changes Jan 21, 2025

View reviewed changes

fixup! fix!: Refactored the repository ruleset code

7241f1a

Signed-off-by: Steve Hipwell <steve.hipwell@gmail.com>

gmlewis approved these changes Jan 21, 2025

View reviewed changes

gmlewis merged commit 27bd175 into google:master Jan 21, 2025
5 of 7 checks passed

stevehipwell deleted the refactor-rules branch January 21, 2025 18:25

		@@ -109,7 +109,6 @@ func main() {
		}

		err := runVerification(sev, pb, b)

	// This function is necessary as the UpdateEnterpriseRuleset function does not marshal ByPassActor if passed as an empty array.
	// This function is necessary as the UpdateRepositoryRuleset function does not marshal ByPassActor if passed as an empty array.

		// If new rules are added to RulesetRules the capacity needs increasing
		arr := make([]repositoryRulesetRuleWrapper, 0, 21)

	// If new rules are added to RulesetRules the capacity needs increasing
	arr := make([]repositoryRulesetRuleWrapper, 0, 21)
	var arr []repositoryRulesetRuleWrapper

	var wrappers []branchRuleWrapper
	var wrappers []*branchRuleWrapper

	var wrappers []repositoryRulesetRuleWrapper
	var wrappers []*repositoryRulesetRuleWrapper

fix!: Refactor the repository ruleset code #3430

fix!: Refactor the repository ruleset code #3430

Conversation

stevehipwell commented Jan 13, 2025 • edited by gmlewis Loading

codecov bot commented Jan 13, 2025 • edited Loading

Codecov Report

gmlewis commented Jan 13, 2025

stevehipwell commented Jan 13, 2025

gmlewis left a comment

Choose a reason for hiding this comment

stevehipwell commented Jan 13, 2025

gmlewis left a comment

Choose a reason for hiding this comment

stevehipwell commented Jan 14, 2025

gmlewis commented Jan 14, 2025 • edited Loading

stevehipwell commented Jan 14, 2025

gmlewis commented Jan 14, 2025

gmlewis left a comment

Choose a reason for hiding this comment

stevehipwell commented Jan 20, 2025

gmlewis commented Jan 20, 2025

stevehipwell commented Jan 20, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmlewis left a comment

Choose a reason for hiding this comment

stevehipwell commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stevehipwell commented Jan 21, 2025

gmlewis commented Jan 21, 2025

gmlewis Jan 21, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmlewis left a comment

Choose a reason for hiding this comment

gmlewis commented Jan 21, 2025

stevehipwell commented Jan 21, 2025

gmlewis left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gmlewis left a comment

Choose a reason for hiding this comment

stevehipwell commented Jan 13, 2025 •

edited by gmlewis

Loading

codecov bot commented Jan 13, 2025 •

edited

Loading

gmlewis commented Jan 14, 2025 •

edited

Loading

gmlewis Jan 21, 2025 •

edited

Loading