Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix!: Refactor the repository ruleset code #3430

Open
wants to merge 7 commits into
base: master
Choose a base branch
from

Conversation

stevehipwell
Copy link
Contributor

@stevehipwell stevehipwell commented Jan 13, 2025

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.

@gmlewis 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 gmlewis changed the title fix!: Refactored the repository ruleset code fix!: Refactor the repository ruleset code Jan 13, 2025
Copy link

codecov bot commented Jan 13, 2025

Codecov Report

Attention: Patch coverage is 50.83056% with 296 lines in your changes missing coverage. Please review.

Project coverage is 90.49%. Comparing base (2b8c7fa) to head (90f33ab).
Report is 221 commits behind head on master.

Files with missing lines Patch % Lines
github/rules.go 48.24% 236 Missing and 59 partials ⚠️
github/repos_contents.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #3430      +/-   ##
==========================================
- Coverage   97.72%   90.49%   -7.24%     
==========================================
  Files         153      175      +22     
  Lines       13390    15282    +1892     
==========================================
+ Hits        13085    13829     +744     
- Misses        215     1300    +1085     
- Partials       90      153      +63     

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

@gmlewis
Copy link
Collaborator

gmlewis commented Jan 13, 2025

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

@stevehipwell
Copy link
Contributor Author

@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.

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 Outdated Show resolved Hide resolved
github/event_types.go Outdated Show resolved Hide resolved
github/rules.go Outdated Show resolved Hide resolved
github/repos_rules.go Outdated Show resolved Hide resolved
github/rules.go Outdated Show resolved Hide resolved
github/rules.go Outdated Show resolved Hide resolved
github/rules.go Outdated Show resolved Hide resolved
github/rules.go Outdated Show resolved Hide resolved
github/rules.go Outdated Show resolved Hide resolved
github/rules.go Outdated Show resolved Hide resolved
@stevehipwell
Copy link
Contributor Author

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

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

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
@stevehipwell
Copy link
Contributor Author

@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
Copy link
Collaborator

gmlewis commented Jan 14, 2025

@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
Copy link
Contributor Author

@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
Copy link
Collaborator

gmlewis commented Jan 14, 2025

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 !

Copy link
Collaborator

@gmlewis gmlewis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Breaking API Change PR will require a bump to the major version num in next release. Look here to see the change(s). NeedsReview PR is awaiting a review before merging.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Repository ruleset event doesn't match schema
2 participants