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

✨ branch protection: requiring PRs gives partial credit #3499

Conversation

diogoteles08
Copy link
Contributor

@diogoteles08 diogoteles08 commented Sep 20, 2023

What kind of change does this PR introduce?

It's a feature, described on the corresponding issue

Which issue(s) this PR fixes

Fixes #2727

Notes for the reviewers

I tried to split the PR on independent commits, so it might be easier to review commit by commit.

The last commit were just refactors on the previously existent code of the file I worked on. Let me know if you prefer that I make the refactors as another PR.

Does this PR introduce a user-facing change?

Branch Protection check now also evaluates if the project requires PRs prior to make changes to the branch. This won't change anything for the users that already require reviews, but will enable score enhancement for those who can't require reviewers.

@diogoteles08 diogoteles08 force-pushed the feat/branch-protection-recognize-rule-changes-only-through-pr branch from 4b17d60 to 5f53749 Compare September 20, 2023 16:40
@diogoteles08 diogoteles08 marked this pull request as draft September 20, 2023 16:47
@diogoteles08 diogoteles08 force-pushed the feat/branch-protection-recognize-rule-changes-only-through-pr branch from 5f53749 to a7ca2b5 Compare September 20, 2023 23:06
@diogoteles08 diogoteles08 force-pushed the feat/branch-protection-recognize-rule-changes-only-through-pr branch from a7ca2b5 to 657fc4f Compare September 20, 2023 23:18
@diogoteles08 diogoteles08 temporarily deployed to gitlab September 20, 2023 23:18 — with GitHub Actions Inactive
@diogoteles08 diogoteles08 temporarily deployed to integration-test September 20, 2023 23:18 — with GitHub Actions Inactive
@codecov
Copy link

codecov bot commented Sep 20, 2023

Codecov Report

Merging #3499 (f78b2f0) into main (30ef6b1) will decrease coverage by 5.26%.
The diff coverage is 79.16%.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #3499      +/-   ##
==========================================
- Coverage   76.24%   70.99%   -5.26%     
==========================================
  Files         214      214              
  Lines       14625    14628       +3     
==========================================
- Hits        11151    10385     -766     
- Misses       2807     3623     +816     
+ Partials      667      620      -47     

@diogoteles08 diogoteles08 force-pushed the feat/branch-protection-recognize-rule-changes-only-through-pr branch from 657fc4f to b6778c2 Compare September 20, 2023 23:32
@diogoteles08 diogoteles08 temporarily deployed to gitlab September 20, 2023 23:33 — with GitHub Actions Inactive
@diogoteles08 diogoteles08 marked this pull request as ready for review September 20, 2023 23:33
@diogoteles08 diogoteles08 temporarily deployed to integration-test September 20, 2023 23:33 — with GitHub Actions Inactive
@spencerschrock
Copy link
Member

Can you rebase/merge in the changes from main before review? #3502 introduced some scoring changes, so you'll need to resolve the merge conflict.

@diogoteles08 diogoteles08 force-pushed the feat/branch-protection-recognize-rule-changes-only-through-pr branch from b6778c2 to dae8813 Compare September 27, 2023 18:11
@diogoteles08 diogoteles08 temporarily deployed to gitlab September 27, 2023 18:11 — with GitHub Actions Inactive
@diogoteles08 diogoteles08 temporarily deployed to integration-test September 27, 2023 18:12 — with GitHub Actions Inactive
@diogoteles08 diogoteles08 force-pushed the feat/branch-protection-recognize-rule-changes-only-through-pr branch from dae8813 to d170f83 Compare September 27, 2023 18:50
@diogoteles08 diogoteles08 temporarily deployed to gitlab September 27, 2023 18:50 — with GitHub Actions Inactive
@diogoteles08 diogoteles08 temporarily deployed to integration-test September 27, 2023 18:50 — with GitHub Actions Inactive
clients/githubrepo/branches.go Outdated Show resolved Hide resolved
clients/githubrepo/branches.go Outdated Show resolved Hide resolved
clients/githubrepo/branches.go Outdated Show resolved Hide resolved
Copy link
Member

@spencerschrock spencerschrock left a comment

Choose a reason for hiding this comment

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

overall I think this is good. There's 1-2 bugs that should be fixed before merge.

checks/evaluation/branch_protection.go Outdated Show resolved Hide resolved
checks/evaluation/branch_protection.go Outdated Show resolved Hide resolved
checks/evaluation/branch_protection.go Show resolved Hide resolved
checks/evaluation/branch_protection.go Show resolved Hide resolved
checks/evaluation/branch_protection_test.go Outdated Show resolved Hide resolved
checks/evaluation/branch_protection_test.go Outdated Show resolved Hide resolved
clients/githubrepo/branches.go Show resolved Hide resolved
e2e/branch_protection_test.go Show resolved Hide resolved
…th its purpose

I also changed the test to not require PRs, as it's how it is when a new GitHub
Branch Protection config is created. The changes on the loggings numbers are due
to:
1. A warning for not having DismissStaleReviews became a debug
2. Removed the warning we had for not requiring CodeOwners
3. Have a new warning for not requiring PRe

Signed-off-by: Diogo Teles Sant'Anna <[email protected]>
@spencerschrock spencerschrock changed the title ✨ Feat/branch protection recognize rule changes only through pr ✨ branch protection: requiring PRs gives partial credit Dec 12, 2023
@spencerschrock spencerschrock merged commit db7b6e7 into ossf:main Dec 12, 2023
41 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Suggestion: Change Branch-Protection check to consider rule of "change only through PRs"