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

BUG: Failures in Branch-Protection during release testing #1320

Closed
azeemshaikh38 opened this issue Nov 20, 2021 · 8 comments · Fixed by #1965
Closed

BUG: Failures in Branch-Protection during release testing #1320

azeemshaikh38 opened this issue Nov 20, 2021 · 8 comments · Fixed by #1965
Assignees
Labels
kind/bug Something isn't working
Projects

Comments

@azeemshaikh38
Copy link
Contributor

Release tests found some issues with Branch-Protection check. Looks like related to recent scoring PR. @laurentsimon assigning to you for further debugging.

./scorecard --repo=github.com/kubermatic/dashboard-v2 --checks=Branhc-Protection
./scorecard --repo=github.com/xwp/stream --checks=Branch-Protection
./scorecard --repo=github.com/americanexpress/one-app-cli --checks=Branch-Protection
@azeemshaikh38 azeemshaikh38 added the kind/bug Something isn't working label Nov 20, 2021
@laurentsimon
Copy link
Contributor

Release tests found some issues with Branch-Protection check. Looks like related to recent scoring PR. @laurentsimon assigning to you for further debugging.

./scorecard --repo=github.com/kubermatic/dashboard-v2 --checks=Branhc-Protection

"reason": "could not find branch name release/v2.18: branch not found",
@asraa can you take care of this one?

./scorecard --repo=github.com/xwp/stream --checks=Branch-Protection
./scorecard --repo=github.com/americanexpress/one-app-cli --checks=Branch-Protection

"reason": "internal error: invalid score 2 != 0",
I will look into these two.

@laurentsimon
Copy link
Contributor

Assigning to @asraa the part on version not being found. not urgent

@asraa
Copy link
Contributor

asraa commented Mar 1, 2022

It seems like this is because there's a large number of branches (>30) and the GitHub API making the call to ListBranches only returns 30 at a time. I'll see if we can pagify/get all.

@asraa
Copy link
Contributor

asraa commented Mar 1, 2022

Ah, this was fixed by increasing the refsToAnalyze.

Is this a hard-limit? I could also increase this so that it makes another API call to fetch branches if it wasn't found in the first 30.

@laurentsimon
Copy link
Contributor

laurentsimon commented Mar 2, 2022

I see. @azeemshaikh38 can you chime in? Will this trigger issues with API rate limiting in the weekly cron?

@asraa how do we calculate the "right" number of refsToAnalyze? Heuristics?

@azeemshaikh38
Copy link
Contributor Author

Nice catch! I'm a bit hesitant about increasing refsToAnalyze since that'll increase the API cost and we don't know how many branches we'll have to iterate through to get the right ones.

My suggestion would be to update ListBranches API to take a ListBranchesOptions{} struct that contains an optional list of branch names to retrieve. I haven't found a way to batch retrieve these branches, so we'll then have to make multiple calls to get the BranchProtectionRules for these names. . This still means more API calls but at least since the number of branches we are interested in is probably a handful, it's better than iterating through a bucket load of them.

One FYI, GitHub allows you to list all BranchProtectionRules for a repository, which is great but there is no similar option for RefUpdateRules which is what we rely on mostly given a read-only token. So another long-term solution here is to work with GitHub to get this exposed.

@asraa
Copy link
Contributor

asraa commented Mar 3, 2022

My suggestion would be to update ListBranches API to take a ListBranchesOptions{} struct that contains an optional list of branch names to retrieve.

I think I can work with that! Given that we only want the release and default branches for branch protection, I can plumb that in through setup to create the query for those branch names.

@naveensrinivasan
Copy link
Member

I am going to work on this issue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
kind/bug Something isn't working
Projects
Development

Successfully merging a pull request may close this issue.

4 participants