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

⚠️ refactor: rename fields on Branch Protection Pull Request rules #3879

Conversation

diogoteles08
Copy link
Contributor

@diogoteles08 diogoteles08 commented Feb 16, 2024

This change was briefly discussed at #3728 (comment)

This PR solely renames part of the structure that represents Branch Protection rules.
Inside the structure clients.BranchRef.BranchProtectionRule, I'm renaming RequiredPullRequestReviews to PullRequestRule.

The previous struct declaration is the following:

type RequiredPullRequestReviews struct {
	Required                     *bool // are PRs required
	RequiredApprovingReviewCount *int32
	DismissStaleReviews          *bool
	RequireCodeOwnerReviews      *bool
}

The reason is that the field doesn't refer only to Required reviews of pull requests, it refers to different Branch protection rules around Pull Requests. More specifically, it has the field Required, that states if PRs are either required or not to make changes to that branch. The path RequiredPullRequestReviews.Required could get confusion, while PullRequestRule.Required is clearer. Additionally, all the other fields inside that struct already have the word "Review" on it, so it's redundant to have it also on the struct name.

What kind of change does this PR introduce?

Refactor

This PR would probably be a breaking change because the changed struct is shared externally

Copy link

codecov bot commented Feb 16, 2024

Codecov Report

Merging #3879 (ec9fa04) into main (e780e08) will decrease coverage by 4.49%.
The diff coverage is 84.37%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3879      +/-   ##
==========================================
- Coverage   75.01%   70.52%   -4.49%     
==========================================
  Files         227      227              
  Lines       16263    16263              
==========================================
- Hits        12199    11469     -730     
- Misses       3290     4067     +777     
+ Partials      774      727      -47     

@spencerschrock
Copy link
Member

This would probably be a breaking (:warning:) change, given it's changing a struct in one of our main interfaces. With V5 on the horizon, it's something we can do. But would be good to properly categorize

@diogoteles08 diogoteles08 marked this pull request as ready for review February 16, 2024 22:23
@diogoteles08 diogoteles08 requested a review from a team as a code owner February 16, 2024 22:23
@diogoteles08 diogoteles08 requested review from naveensrinivasan and laurentsimon and removed request for a team February 16, 2024 22:23
@diogoteles08 diogoteles08 changed the title 🌱 refactor: rename fields on Branch Protection Pull Request rules ⚠️ refactor: rename fields on Branch Protection Pull Request rules Feb 16, 2024
@diogoteles08
Copy link
Contributor Author

@spencerschrock Sorry, I forgot about this detail. I updated the PR title and the end of its description, PTAL

Copy link

This pull request is stale because it has been open for 10 days with no activity

@github-actions github-actions bot added the Stale label Feb 27, 2024
@spencerschrock
Copy link
Member

@spencerschrock Sorry, I forgot about this detail. I updated the PR title and the end of its description, PTAL

Now that #3759 is merged, this would need rebased (recreating + force pushing might just be easier). Overall, renaming SGTM.

@github-actions github-actions bot removed the Stale label Mar 1, 2024
Copy link

This pull request has been marked stale because it has been open for 10 days with no activity

@github-actions github-actions bot added the Stale label Mar 12, 2024
@github-actions github-actions bot removed the Stale label Mar 21, 2024
Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>
@diogoteles08 diogoteles08 force-pushed the refactor/renaming-structure-on-PRs-branch-protection branch from 6c957f5 to fd0231b Compare March 21, 2024 13:29
@diogoteles08
Copy link
Contributor Author

@spencerschrock rebased as requested =)

@spencerschrock
Copy link
Member

/scdiff generate Branch-Protection

Copy link

@spencerschrock spencerschrock merged commit 376ee1f into ossf:main Mar 25, 2024
38 checks passed
fhoeborn pushed a commit to fhoeborn/scorecard that referenced this pull request Apr 1, 2024
Signed-off-by: Diogo Teles Sant'Anna <diogoteles@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants