Check for branch protection before raising a PR #824
Conversation
NovemberTang
force-pushed
the
nt/require-branch-protection
branch
from
477a7a6
to
3a76e78
Compare
| ); | ||
| } | ||
| } else { | ||
| console.log(`Testing generation of ${fileName} for ${repoName}`); |
There was a problem hiding this comment.
this now exists as logPr()
|
|
||
| console.debug(allResults.data); | ||
|
|
||
| const result = allResults.data.find( |
There was a problem hiding this comment.
repos that contain the custom property production_status : production have a branch protection ruleset applied to them automatically
NovemberTang
force-pushed
the
nt/require-branch-protection
branch
from
3a76e78
to
8fa756c
Compare
| const hasProtection = | ||
| protection.data.enabled === true && | ||
| !!protection.data.required_pull_request_reviews; |
There was a problem hiding this comment.
Any reason to not get this from the database? There's a protected column in the github_repository_branches table.
There was a problem hiding this comment.
Defence in depth. It's possible that this is overkill but my reasoning is:
Ideally whoever is invoking this function would have made sure that the repo they're referring to has branch protection, but there's no guarantee of that. There's also no guarantee that the branch protection they have includes a review requirement. Basically, I'm trying to trust repocop as little as possible. (FWIW when repocop starts sending messages to this lambda I do plan to have it filter out unprotected repos, but someone using this a year from now might forget).
An alternative approach could be establishing a requirement to bundle prisma into any lambda that uses this function, and calling out to the DB at runtime, but I think that'll increase the setup complexity, increase the size of the zip 100-1000x, make inline lambda editing impossible, and is a bit tricky to document, so may not be worth it to grab two pieces of information.
Another (minor) point is that it's possible that a repo's branch protection has been changed between CQ running, and this lambda being invoked, and this code handles that edge case.
There was a problem hiding this comment.
Hmm, I'm not quite sure I understand how we're expecting this lambda to work... Are you suggesting it's manually invoked by a person every time? If so, I'd suggest we don't need AWS infrastructure here; a script that we run locally would simplify things.
I'd suggest we move to a setup where the lambda is invoked either on a schedule, or on an event (e.g. the CloudQuery task completing). This removes human error, and answers:
a repo's branch protection has been changed between CQ running, and this lambda being invoked
If the lambda does need to be invoked by a human from time to time, could we script it? The script would perform the pre-flight checks of ensuring branch protection is enabled, etc.
make inline lambda editing impossible
I'd challenge the use-case for this. Editing the lambda function in place removes any audit trail!
As I said though, I'm not sure I understand the role/context of this lambda - happy to chat about it.
| repoName: string, | ||
| ): Promise<boolean> { | ||
| const allResults = await octokit.request( | ||
| 'GET /repos/{owner}/{repo}/properties/values', |
There was a problem hiding this comment.
Similar to above, any reason to not get this from the database? There's a custom_properties field in the github_repositories table.
| prTitle: string, | ||
| prBody: string, | ||
| ) { | ||
| console.log(`Testing generation of ${fileName} for ${repoName}`); |
There was a problem hiding this comment.
Does this line make sense for all uses of logPr?
There was a problem hiding this comment.
I thought that it did. Where do you think it doesn't make sense?
|
Reasons to close: To keep functions relatively single-purpose, it's probably best for the protection check to happen somewhere else. |
What does this change?
Any part of the service catalogue using
createPrAndAddToProjectwill now check that the repo has sufficient branch protections before raising a PR.Why?
These PRs need to be reviewed by teams as they have been raised by a bot. We make requests to GitHub to get this information. We can't rely on repocop to provide this information for two reasons:
How has it been verified?
This has been tested on PROD. #823 was generated as a result, as the repo does not have branch protection, but does have the ruleset applied as a consequence of its
production_statusproperty