landing_checks: block commits with REPO- flag mismatch (bug 2032267)#1091
landing_checks: block commits with REPO- flag mismatch (bug 2032267)#1091
Conversation
|
View this pull request in Lando to land it once approved. |
| ) | ||
| return | ||
|
|
||
| if match := REPO_FLAG_RE.search(firstline): |
There was a problem hiding this comment.
Note: this is the simplest implementation, added to the existing CommitMessagesCheck. This means we won't be able to selectively disable only the REPO- check.
I think it should be sufficient for our purpose, though.
There was a problem hiding this comment.
Agreed, I think this is okay to move here. :)
shtrom
force-pushed
the
bug2032267/repo-blocker
branch
from
d356116 to
f3a45db
Compare
| self.commit_message_issues.append( | ||
| f"Commit locked to another repo than {self.repo_name}: {commit_message}" | ||
| ) | ||
| return |
There was a problem hiding this comment.
Question: By returning early here, and in other checks below, we only give a single error at a time per commit message. Should we instead not return and present all errors at once?
There was a problem hiding this comment.
If we can detect and warn about more issues that sounds like a good idea. Since we return a single string as the result of each check, it becomes a question of how the string is displayed in the UI.
I'd say this is acceptable for now, and in the future we can improve the checks so something more dynamic is returned from each warning/blocker.
There was a problem hiding this comment.
We return nothing here. We just add one error message with the commit message, and give up. We could add all the errors regarding every commit message in one go.
Given the way we currently render the errors, though, this would be trading off cognitive load for repeated partial fixes.
| self.commit_message_issues.append( | ||
| f"Commit locked to another repo than {self.repo_name}: {commit_message}" | ||
| ) | ||
| return |
There was a problem hiding this comment.
If we can detect and warn about more issues that sounds like a good idea. Since we return a single string as the result of each check, it becomes a question of how the string is displayed in the UI.
I'd say this is acceptable for now, and in the future we can improve the checks so something more dynamic is returned from each warning/blocker.
| ), | ||
| "`git-format-patch` cruft should result in a failed check.", | ||
| ), | ||
| ( |
There was a problem hiding this comment.
Could we expand this to cover more of the test cases from the hg hook?
| ) | ||
| return | ||
|
|
||
| if match := REPO_FLAG_RE.search(firstline): |
There was a problem hiding this comment.
Agreed, I think this is okay to move here. :)
| if not self.skip_checks(job, new_commits) and repo.hooks_enabled: | ||
| patch_helpers = repo.scm.get_patch_helpers_for_commits(new_commits) | ||
| landing_checks = LandingChecks(job.requester_email) | ||
| landing_checks = LandingChecks(job.requester_email, repo.name) |
There was a problem hiding this comment.
Using repo.name here may be problematic, as the repo.name is the name in lando, e.g., firefox-autoland, which doesn't match the name that the VCT hook uses, e.g., autoland. This means either one or the other check will choke...
2 participants
No description provided.