Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
✨ branch protection: requiring PRs gives partial credit (ossf#3499)
* feat(branch-protection): consider if project requires PRs prior to make changes As discussed at the issue ossf#2727, we're adding the "require PRs prior to make changes" as another requirement to tier 2. In addition to that, we're changing the weight of the tier 2 requirements so that "requiring 1 reviewer" has weight 2, while the other tier 2 requirements have weight 1 Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * test(branch-protection): increment and adapt testing 1. Adapt previous test cases to consider that now we'll have an aditional Info log telling that the project requires PRs to make changes. 2. Add more cases to test relevant use cases on the tier 2 level of branch protection Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * docs(branch-protection-check): adapt check description to consider requirement of require PRs to make changes It adds the new tier 2 requirement, but also specify that the "require at least 1 reviewer" will have doubled weight. Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * refactor(branch-protection-check): avoid duplicate funcions and enhance readability Made some nice-to-have improvements on project readability, making it easier easier to understand how the branch-protection score is computed. Also unified 8 different functions that were doing basically the same thing. Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * feat(branch-protection): standardize values received on evaluation Previously, at the evaluation part of branch protetion, the values nil and false or zero were sort of interchangeble. This commit changes the code to set as nil only the data that could not be retrieved from github -- all the others would have values as false, zero, true, etc Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * test(github-client): adapt and add tests to check if nil values are coherent 1. Add new test to evaluate how we're interpreting a rule with all checkboxes unchecked (most shouldn't be nil) 2. Adapt existent tests to expect non-nil values for unchecked checkboxes Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * feat(client-github): avoid reusing bool pointers Changes some pieces of code to prefer using pointers of bool instantiated independently. If reusing bool pointers, at some piece of code the value of the bool could inadvertently changed and it would change the value of all other fields reusing that pointer. Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * feat(branch-protection): enhance evaluation if scorecard was run by admin At the evaluation step we were using some non untrusted fieldds of the resposte to evaluate if Scorecard was run as admin or not. Now we're using a field provided directly from the client file. Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * test(branch-protection): adapt testings to say if they have admin info or not After last commit, the client will tell the evaluation files if Scorecard was run by administrator or not (i.e., if we have all the infos). This commit adapts the testings to also provide this info. Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * test(e2e-branch-protection): adapt number of logs after changes - 2 warns (for 'last push approval' and 'codeowners review' disabled) were added because now those informations come as 'not-nil' at the evaluation part. - 1 info was added to say that PRs are required to make changes - 1 debug was removed because it said that we couldn't retrieve 'last push approval' information, but we actually can. It was just incorrectly set as nil Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * Revert the 2 commits with changes around how Scorecard detects admin run Reverts commit 64c3521 and commit e2662b7. Both had chances around using clients/branch.go scructur to store the information of whether Scorecard was being run by admin or not. We decided to not change this structure for this purpose. Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * refactor(branch-protection): change data structure to use pointer instead of value At clients.BranchProtectionRule struct, changing RequiredPullRequestReviews to be a pointer instead of a struct value. This will allow the usage of the nil value of this structure to mean that we can't say if the repository requires reviews or not. Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * feat(branch-protection): use nil pointer on reviewers struct to mean we don't know if they require PRs The nil value of the struct RequiredPullRequestReviews will now mean that we can't tell whether the project requires PRs to make changes or not. When we get this case, we're printing a debug informing that we don't have this data, but also printing a warn saying that they don't require reviews, because that will be true at this case. Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * test(branch-protection): if we're setting the reviewers struct to nil when needed Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * doc(branch-protection): add code comment explaining different weight on tier 2 scores Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * refactor(branch-protection): avoid duplicate if branches on reviewers num comparation Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * docs(branch-protection): clarify commentings around data structure Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * refactor: clean code on parsing GitHub BP data Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * feat(branch-protection): ressignify the nil PullRequestReviewRule to mean PR not required Adapt translation of data from GitHub API, now for our internal data modeling, having a nil PullRequestReviewRule structure will mean that PRs are not required on the repo (can also mean we don't have data to ensure that). It also changes the order of the calls of copyNonAdminSettings and copyAdminSettings to make the first one be called first. This eases the code because the PullRequestReviewRule can be always instantiated at this function. Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * test(branch-protection): ensure we translate GitHub BP data as expected Ensure we're correctly translating GitHub data from the old Branch Protection config. Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * feat(branch-protection): adapt score evaluation after 2efeee6 Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * test(branch-protection): adapt testings to changes of last commits Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * docs(branch-protection): add TODO comments pointing refactor opportunities Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * fix: avoid penalyzing non-admin for dismissStaleReview Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * fix(branch-protection): prevent false value from API field to become nil When translating the API results, if the specific field `DismissesStaleReviews` had a false value, it was not being initiated in our data model and was remaining nil. Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * refactor: clarify different weight on first reviewer Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * refactor: enhance clarity of loggings and comments Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * test(branch-protection): new test to cover different rules affecting same branch Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * docs(branch-protection): change requirements ordering to keep admin ones together Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * refactor(branch-protection): simplify auxiliary function Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * refactor(branch-protection): fix code format to linter requirements Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * refactor(branch-protection): avoid unnecessary initializations and rename function Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * test(branch-protection): adapt test that was forgotten on commit 6858790 Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * refactor(branch-protection): use enums to represent tiers Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * refactor(branch-protection): remove nil fields of struct initialization when they dont contribute for clarification Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * refactor(branch-protection): simplify functions by using generics Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * docs(branch-protection): update docs after generate-docs run Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * fix(branch-protection): fix duplicated line on code Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * fix(branch-protection): stop exporting Tier enum Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * refactor(branch-protection): changing unchanged var to const Signed-off-by: Diogo Teles Sant'Anna <[email protected]> * test(branch-protection): Rename test and adapt it to be consistent with 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]> --------- Signed-off-by: Diogo Teles Sant'Anna <[email protected]>
- Loading branch information
1 parent
30ef6b1
commit db7b6e7