✨ add release branch protection check #554

asraa · 2021-06-08T19:20:49Z

Signed-off-by: Asra Ali [email protected]

Please check if the PR fulfills these requirements

Tests for the changes have been added (for bug fixes / features)
PR title follows the guidelines defined in https://github.com/ossf/scorecard/blob/main/CONTRIBUTING.md#pr-process

What kind of change does this PR introduce? (Bug fix, feature, docs update, ...)
What is the current behavior? (You can also link to an open issue here)
What is the new behavior (if this is a feature change)?
Does this PR introduce a breaking change? (What changes might users need to make in their application due to this PR?)
Other information:
Fixes Feature: Branch-Protection: add support for release branches #445
(although it does not do detection of activity on default branch)

Signed-off-by: Asra Ali <[email protected]>

asraa · 2021-06-08T19:24:57Z

Probably an e2e test is best for this, have to check in on how to enable that. The unit tests probably suffice for the check code.

Signed-off-by: Asra Ali <[email protected]>

asraa · 2021-06-10T18:59:57Z

Added unit tests with a nice suggestion from @jeffmendoza. Ready for review!

Signed-off-by: Asra Ali <[email protected]>

checks/branch_protected.go

Signed-off-by: Asra Ali <[email protected]>

laurentsimon · 2021-06-11T14:45:31Z

checks/branch_protected.go

+	return checker.MultiCheckResultAnd(checks...)
+}
+
+func getProtectionAndCheck(ctx context.Context, r repositories, l logger, ownerStr, repoStr,


do you know if this will work when the commitish is a commit hash and not a branch name?
For envoy, for example, one of the releases has commitish "target_commitish": "8fb3cb86082b17144a80402f5367ae65f06083bd" (see #445)

Nice catch! You get a "404 Branch not found" (nil response and an error) as opposed to if there's no protection, which gives "404 Branch not protected" (resp status not found and an error).

The existing code probably assumed the default branch was always there. Updating so this skips when branches are not found.

so the check will fail if a branch was deleted/does not exist?

In addition, we you could check if a commitish is a hash. If it is, retrieve the name of the branch the commits belongs to, and use the branch name to call getProtectionAndCheck().

I refactored to check whether the branch exists with ListBranches. If it doesn't exist/is deleted, it skips.

I'm not sure I can check which branch the commit belongs to. I found references to the github search api, which seems like it's in preview mode.

I refactored to check whether the branch exists with ListBranches. If it doesn't exist/is deleted, it skips.

shall we fail when the commitish points to a non-existing branch, since it provides no information about the protection of the branch that was used? Wdut?

I'm not sure I can check which branch the commit belongs to. I found references to the github search api, which seems like it's in preview mode.

you're right, does not seem to work. An alternative would be to clone the repo and use git branch --all --contains <commit>. I think it's better to wait for the API above to be more broadly available. Let's check if the commitish looks like a hash and ignore this case for now then. Let's create a github issue to track this for future improvement.

Any better ideas?

I refactored to check whether the branch exists with ListBranches. If it doesn't exist/is deleted, it skips.

shall we fail when the commitish points to a non-existing branch, since it provides no information about the protection of the branch that was used? Wdut?

while looking at envoy, I realized you renamed the master branch to main (the new naming that GitHub recommends). With what I suggested above, we would fail the test since the master branch no longer exists. If you go with the solution I suggested, you'll have to treat main and master as being the same branch.

I'm not sure I can check which branch the commit belongs to. I found references to the github search api, which seems like it's in preview mode.

you're right, does not seem to work. An alternative would be to clone the repo and use git branch --all --contains <commit>. I think it's better to wait for the API above to be more broadly available. Let's check if the commitish looks like a hash and ignore this case for now then. Let's create a github issue to track this for future improvement.

Any better ideas?

Sorry for the delay, was doing a bunch of local testing and ran into some rate limits. To answer the questions:

Agreed, let's ignore the hash commitish for now (I added a comment TODO to go back later for improvement)

About the branch rename. Ugh! the Github REST API gives you a 301 moved permanently when you try to get a moved branch (like master). BUT! Go-github does NOT from my testing. It gives you a 404 Not Found, giving me no way to distinguish between a moved and a deleted/non-existing branch. It feels wrong to fail on that. Still trying to see if I can get moved information from go-github.

Sorry for the delay, was doing a bunch of local testing and ran into some rate limits. To answer the questions:

Agreed, let's ignore the hash commitish for now (I added a comment TODO to go back later for improvement)

+1

About the branch rename. Ugh! the Github REST API gives you a 301 moved permanently when you try to get a moved branch (like master). BUT! Go-github does NOT from my testing. It gives you a 404 Not Found, giving me no way to distinguish between a moved and a deleted/non-existing branch. It feels wrong to fail on that. Still trying to see if I can get moved information from go-github.

nice finding. I did not know about that! While you investigate, we could file an github issue for go-github to ask about the behavior and whether it'd be a breaking change for them to fix it.
Last resort would be for us to make the http request ourselves :/ Kinda of sad to have to do this, though

Yep, just did! google/go-github#1895

For now just to keep this PR going I'll manually handle the master->main change, and otherwise we'll just fail when the commitish returns a non-existing branch.

done... as much as can do.

laurentsimon · 2021-06-11T15:10:15Z

checks/branch_protected_test.go

@@ -15,7 +15,10 @@
 package checks


you can also add e2e tests in the e2e/ folder. You can launch make e2e with an GITHUB_AUTH_TOKEN set for scorecard repo. But you should be able to locally test only the branch protection e2e test (I forgot the exact go command :-))

want to do this in this PR or in a follow-up one? Are there blockers for this? (I know sometimes it's hard to find a repo that actually passes all the tests)

checks/branch_protected.go

azeemshaikh38

Nice, thanks!

checker/check_runner.go

checks/branch_protected_test.go

Signed-off-by: Asra Ali <[email protected]>

asraa · 2021-06-15T16:28:52Z

Tried this on my own silly repo, that included a master -> main rename

$ ./scorecard --show-details --checks Branch-Protection --repo=github.com/asraa/asraa.github.io
Starting [Branch-Protection]
Finished [Branch-Protection]

RESULTS
-------
Repo: github.com/asraa/asraa.github.io
Branch-Protection: Fail 10
!! branch protection not enabled for branch branch1
!! branch protection - EnforceAdmins should be enabled on main
!! branch protection - Linear history should be enabled on main
!! branch protection - Status checks for merging should be enabled on main
!! branch protection - Stale review dismissal should be enabled on main
!! branch protection - Owner review should be enabled on main

check release branch protection

631d0a3

Signed-off-by: Asra Ali <[email protected]>

asraa added 3 commits June 10, 2021 12:53

add documentation

c885d8f

Signed-off-by: Asra Ali <[email protected]>

Merge remote-tracking branch 'upstream/main' into branch

306a23b

add tests

9927578

Signed-off-by: Asra Ali <[email protected]>

asraa marked this pull request as ready for review June 10, 2021 18:59

inferno-chromium requested review from laurentsimon and azeemshaikh38 June 10, 2021 19:29

asraa added 3 commits June 10, 2021 16:12

fix test parallelization

8b45c38

Signed-off-by: Asra Ali <[email protected]>

lint

77b58ef

Signed-off-by: Asra Ali <[email protected]>

Merge remote-tracking branch 'upstream/main' into branch

d3602ac

laurentsimon approved these changes Jun 10, 2021

View reviewed changes

checks/branch_protected.go Outdated Show resolved Hide resolved

checks/branch_protected.go Outdated Show resolved Hide resolved

comments

6d280c7

Signed-off-by: Asra Ali <[email protected]>

laurentsimon reviewed Jun 11, 2021

View reviewed changes

checks/branch_protected.go Outdated Show resolved Hide resolved

azeemshaikh38 approved these changes Jun 11, 2021

View reviewed changes

azeemshaikh38 reviewed Jun 11, 2021

View reviewed changes

checker/check_runner.go Outdated Show resolved Hide resolved

azeemshaikh38 reviewed Jun 11, 2021

View reviewed changes

checks/branch_protected_test.go Outdated Show resolved Hide resolved

asraa added 4 commits June 15, 2021 12:13

update

63e88af

Signed-off-by: Asra Ali <[email protected]>

Merge remote-tracking branch 'upstream/main' into branch

a9ebed8

address comments add TODO

b3c24f8

Signed-off-by: Asra Ali <[email protected]>

fix

14989ad

Signed-off-by: Asra Ali <[email protected]>

inferno-chromium enabled auto-merge (squash) June 15, 2021 16:35

inferno-chromium merged commit ceef465 into ossf:main Jun 15, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

✨ add release branch protection check #554

✨ add release branch protection check #554

asraa commented Jun 8, 2021 •

edited

Loading

asraa commented Jun 8, 2021

asraa commented Jun 10, 2021

laurentsimon Jun 11, 2021

asraa Jun 11, 2021

laurentsimon Jun 11, 2021 •

edited

Loading

asraa Jun 11, 2021

laurentsimon Jun 11, 2021 •

edited

Loading

laurentsimon Jun 11, 2021 •

edited

Loading

asraa Jun 15, 2021

laurentsimon Jun 15, 2021 •

edited

Loading

asraa Jun 15, 2021

asraa Jun 15, 2021

laurentsimon Jun 11, 2021

laurentsimon Jun 15, 2021

azeemshaikh38 left a comment

asraa commented Jun 15, 2021 •

edited

Loading

✨ add release branch protection check #554

✨ add release branch protection check #554

Conversation

asraa commented Jun 8, 2021 • edited Loading

asraa commented Jun 8, 2021

asraa commented Jun 10, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurentsimon Jun 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurentsimon Jun 11, 2021 • edited Loading

Choose a reason for hiding this comment

laurentsimon Jun 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

laurentsimon Jun 15, 2021 • 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

Choose a reason for hiding this comment

azeemshaikh38 left a comment

Choose a reason for hiding this comment

asraa commented Jun 15, 2021 • edited Loading

asraa commented Jun 8, 2021 •

edited

Loading

laurentsimon Jun 11, 2021 •

edited

Loading

laurentsimon Jun 11, 2021 •

edited

Loading

laurentsimon Jun 11, 2021 •

edited

Loading

laurentsimon Jun 15, 2021 •

edited

Loading

asraa commented Jun 15, 2021 •

edited

Loading