feat: Add a script to see if the CLA check is required. #39
Conversation
This adds the CLA check in a fairly generic way so that we can add other checks and fixes as we figure them out.
feanil
force-pushed
the
feanil/cla_check
branch
from
57cd526 to
e5f4aa6
Compare
For the CLA check to be properly enforced, two things need to be true.
1. The check has to be a required check so that changes can't be merged
without it passing.
2. The `cla-checker` team needs write access to the repo so that it can
actually post the check results on PRs in the repo.
* The `cla-checker` teams is just the openedx-webhooks bot that
currently posts the check status.
This change fixes the second issue, ensuring that the cla-checker team
has write(push) access to the repo.
| self.team_setup_correctly = True | ||
| return (True, f"'{self.cla_team}' team has 'push' access.") | ||
|
|
||
| def dry_run(self): |
There was a problem hiding this comment.
Nit: I think it would make sense to make dry_run a member of the class instead of passing it around since it doesn't look like you're calling the fix method more than one way in the same run. I can see an argument for leaving it as is for manual testing, it really depends on how you expect it to be used.
migrate/repo_checks.py
Outdated
| def __init__(self, api, org, repo): | ||
| raise NotImplementedError |
There was a problem hiding this comment.
Why specify this __init__ and not implement anything? You could give a default that assigns these attributes to member variables and then have subclasses implement their own customized ones and call this via super().__init__()
migrate/repo_checks.py
Outdated
| def check(self): | ||
| raise NotImplementedError |
There was a problem hiding this comment.
Nit: maybe run() instead of check() (given the class is Check)?
There was a problem hiding this comment.
Also would pair better with dry_run, though if they share enough logic, you could make it a param to check instead.
There was a problem hiding this comment.
Oh wait, I have this mixed up. dry_run() is for the fix() counterpart, and check() has no side-effects?
| from ghapi.all import GhApi, paged | ||
|
|
||
|
|
||
| class Check: |
There was a problem hiding this comment.
For all of these comments I'm making, I'm assuming that this is something we're going to be building on in the future. If it's likely to only be used once or twice, it's probably not worth the effort to refactor around.
| self.team_setup_correctly = False | ||
|
|
||
| def check(self): | ||
| is_required_check = self._check_cla_is_required_check() |
There was a problem hiding this comment.
In the longer term, it might be worthwhile to think about decoupling the logic of "how does this check run" from "should this check run on this repo", since the latter is a policy decision that may be very separate from the logic of how to actually execute those checks.
There was a problem hiding this comment.
One of the things that putting it in the same place makes me wonder off the top of my head is, "What is the expected result of check() if the repo doesn't pass the check, but it's also not a required check for this repo."
There was a problem hiding this comment.
FYI, not addressing this in this round but I'm thinking more about this and will probably add a separate function to see if it should run separate from whether or not the check would succeed. I'll probably try to make that change as I add more checks.
- Make the init function in the base class useful and call it. - Add docstrings to better show intent of the different functions.
Security forks are not like regular repositories in most of the ways we care about. In particular, you don't manage access to them via teams the same way since they are linked to a security advisory. For now we'll skip these for all checks but later it will probably make sense to figure out a way for each check to evaluate a repo for whether or not it should run on that repo, independent of whether or not it will pass or fail if run. This was great advice from Dave Ormsbee and we can iterate on it futher as we add more checks.
* Fix a spelling issue in the check data. * Extract restriction strings from input data. * Fix an unformatted string.
This adds the CLA check in a fairly generic way so that we can add other
checks and fixes as we figure them out.