-
Notifications
You must be signed in to change notification settings - Fork 169
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
Add git hooks for branch name validation #3479
Conversation
3f9ab3c
to
4d5c428
Compare
cc @cadenmarchese @kimorris27 @mociarain thank you for your review of the old PR, I had to close it in favor of this one, and I hope I addressed all the feedback left in subsequent changes. LMK what you think. |
4d5c428
to
369aa3e
Compare
369aa3e
to
d4c27b9
Compare
/azp run ci |
Azure Pipelines successfully started running 1 pipeline(s). |
fi | ||
|
||
# e.g. "USERNAME/ARO-1234", "USERNAME/hotfix-v20240321.00", or "USERNAME/gh-issue-123" | ||
valid_branch_regex="^${git_username_lower}\/(ARO-[0-9]{4}[a-z0-9._-]*|hotfix-[a-z0-9._-]+|gh-issue-[0-9]+[a-z0-9._-]*)$" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Minor objection to the $
at the end. Could we allow for an optional 3rd free-form segment for cards/issues requiring multiple PRs. Like:
USERNAME "/" TICKET ["/" .*]
I'm just imagining a followup PR for something that was missed or deferred in the initial review. e.g. mbarnes/ARO-1234/bugfix
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is ARO-1234-bugfix
not acceptable in this case? I can add /
to the regex if that is considered a blocker.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think, for our goals with naming consistency here, we should only really need to enforce the prefix of the name, if all the required information is present at the start of the name for easy identification, it shouldn't matter what other suffixes we might need to include on the branch, be it identifications for split units of work, easy clarification to identify branches without memorizing Jira IDs, etc.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My bad, I didn't read the regex close enough. A 2nd slash would make the branch name easier to parse if there's ever a need to do that. But not important enough to block on.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
FWIW I think we should just keep it open and easy with something like [a-z0-9._-]*\/[a-z0-9._-]+$
. We never cared about naming consistency before and I don't know what we should now. My vote is to just keep it simple and move on with as few restrictions on ourselves as necessary.
Which issue this PR addresses:
Fixes ARO-6316 - moved from #3477
What this PR does / why we need it:
See ARO Branching Strategy. Now that branching is required over forks, we need to keep our repo organized. This PR introduces strict naming conventions of branches to:
Test plan for issue:
Local git - attempted to use "bad" branch names and ensured they failed to create commits locally.
git branch --move some-bad-name some-good-name
works and passes the pre-commit hooks.Is there any documentation that needs to be updated for this PR?
Yes, included in this PR. I will send notifications to all SREs both in slack and email of this change.
How do you know this will function as expected in production?
Local dev changes, not deployed to prod.