feat(pre-commit): add hook to enforce branch name validation rules #293

purshottam-sharma · 2025-02-05T13:57:20Z

Description

This pull request introduces a pre-commit hook to enforce specific rules for branch name validation. The goal is to ensure that all branches follow a consistent naming convention, improving the overall development workflow. The pre-commit hook will check the branch name format before allowing commits to proceed.

Related Issue

Issue # (e.g., #CSN-590) - Create Pre-Commit Hook to Enforce Branch Name Rules.

Changes Made

Added pre-commit hook: Implemented a custom script (check-branch-name.sh) that validates the branch name format.
- The branch name must include a valid JIRA ticket (e.g., CSN-1234).
- The description should be lowercase and use kebab-case (no spaces, underscores, or special characters).
- The branch name should start with an allowed prefix (e.g., feat, bugfix, release, etc.).
Updated .pre-commit-config.yml:
- Added the custom check-branch-name hook that runs the validation script.
Added file permission changes:
- Made the check-branch-name.sh script executable.

How to Test

Create a branch: Try creating a branch with an invalid name (e.g., missing the JIRA ticket, using uppercase letters, or spaces).
Attempt to commit: The commit should fail with an error message specifying the correct branch name format.
Create a valid branch: Try creating a branch that follows the naming conventions (e.g., feat/CSN-1234-add-new-feature), and the commit should pass.

Checklist

Make sure that all of the following are complete before submitting the PR:

I have checked the code for typos or issues.
I have run tests and verified that they pass.
I have updated documentation if necessary.
I have added relevant unit/integration tests.
The PR is ready for review.

Screenshots

IvanAnishchuk

I think it's good overall but I'd change the way it's run. Pre-commit is generally a wrong hook for checking branches, it's not the point where branches are created.

IvanAnishchuk · 2025-02-05T17:54:02Z

.github/workflows/pre-commit.yml

+    - name: Install pre-commit
+      run: |
+        python -m pip install --upgrade pip
+        pip install pre-commit
+
+    - name: Install pre-commit hooks
+      run: pre-commit install


Is this really necessary? It should be better documented why we decided to hack copy-pasted code and how the desired behavior differs from default.

IvanAnishchuk · 2025-02-05T18:01:30Z

.pre-commit-config.yaml

+    hooks:
+      - id: check-branch-name
+        name: 'Check Branch Name'
+        entry: ./scripts/check-branch-name.sh


TBH I'm not convinced that in this particular case placing it in pre-commit is actually smart. It's not just for CI, the main purpose of pre-commit is running locally before commits creation (notably not before branch creation! after pre-commit runs I can still git branch whatever && git push).

I'd rather not to do anything there (perhaps pre-push hook tho) and only check this naming convention for pull requests instead - and there's also easier way to do that for PRs (any script we wrap into CI workflow yaml will have branch name as environment variable eliminating the need to hack git plumbing to get those values).

IvanAnishchuk · 2025-02-05T18:02:19Z

scripts/check-branch-name.sh

+#!/bin/sh
+
+# Define allowed prefixes including release
+allowed_prefixes="feat|feature|bugfix|hotfix|chore|refactor|test|spike|prototype|release|docs"


Double-check, I think this differs from what Don wanted and doesn't include archive/ that we use.

IvanAnishchuk · 2025-02-05T18:05:56Z

scripts/check-branch-name.sh

+release_version_pattern="release/v[0-9]+\.[0-9]+\.[0-9]+(-[a-z0-9]+)?"
+
+# Get the current branch name
+branch_name=$(git symbolic-ref --short HEAD 2>/dev/null || git rev-parse --abbrev-ref HEAD 2>/dev/null)


The script is pretty good overall but I'd separate the validation logic from integration bits like this. I think it could be beneficial to run in some places git is not accessible or not yet.

Also these two specific ways of retrieving current branch name would actually break in detached head mode (git branch --show-current is probably safer) but in CI context it's actually safer to just use ${GITHUB_HEAD_REF}

Great spot! Thanks for the detailed explanation. Will definitely integrate this to handle detached HEAD correctly.

IvanAnishchuk · 2025-02-06T04:41:00Z

.pre-commit-config.yaml

+        name: 'Check Branch Name'
+        entry: ./scripts/check-branch-name.sh
+        language: system
+        files: ^.*


Does this check need or use files?

Also stages should always be included.

Two separate scripts: one detects current value, the other validates it, no dependency on git or files or anything in the validation script. + Configs cleanup.

I find post-checkout and pre-push the most effective places for branch name check - after actual branch creation and before things get pushed.

IvanAnishchuk · 2025-02-06T05:48:18Z

Pushed a couple small changes so that it can do this (and also some adjustments so that the script doesn't run multiple times sometimes)

-- that requires additional repo hook installed, I added a helper script for that and a line in QA.md document.

IvanAnishchuk · 2025-02-06T05:48:52Z

The business logic in the check script is unchanged, only the branch detection part.

IvanAnishchuk

I added some changes, it starts looking good to me but I'd recommend some extra testing before we make this new behaviour the default for everyone (it's easy to change after and doesn't affect operations currently, all checks are just for display purposes, so we shouldn't preoptimize too much either).

purshottam-sharma · 2025-02-10T03:53:35Z

@IvanAnishchuk Thanks for improving the PR! I have completed my testing, and the changes look good to me. Please confirm your validation so we can proceed further with this PR.

IvanAnishchuk

LGTM! Good to go, I think.

feat(pre-commit): add hook to enforce branch name validation rules

ccc7b25

purshottam-sharma requested review from IvanAnishchuk, mo0haned, thomas-chu123 and nauttiilus February 5, 2025 13:59

feat(pre-commit): doc branch added to rule

968ac8a

IvanAnishchuk requested changes Feb 5, 2025

View reviewed changes

IvanAnishchuk reviewed Feb 6, 2025

View reviewed changes

IvanAnishchuk added 2 commits February 6, 2025 12:39

feat(pre-commit): separate branch name retrieving and validation

92405cc

Two separate scripts: one detects current value, the other validates it, no dependency on git or files or anything in the validation script. + Configs cleanup.

feat(pre-commit): add a script to install additional git repo hooks

7a1b165

I find post-checkout and pre-push the most effective places for branch name check - after actual branch creation and before things get pushed.

IvanAnishchuk approved these changes Feb 7, 2025

View reviewed changes

feat(pre-commit): enforce fix over bugfix and feat over feature

d540678

IvanAnishchuk approved these changes Feb 10, 2025

View reviewed changes

purshottam-sharma merged commit 6040364 into dev Feb 10, 2025
5 checks passed

purshottam-sharma deleted the feat/CSN-509-pre-commit-hook-branch-name-rules branch February 10, 2025 09:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(pre-commit): add hook to enforce branch name validation rules #293

feat(pre-commit): add hook to enforce branch name validation rules #293

purshottam-sharma commented Feb 5, 2025 •

edited by IvanAnishchuk

Loading

IvanAnishchuk left a comment

IvanAnishchuk Feb 5, 2025

IvanAnishchuk Feb 5, 2025

IvanAnishchuk Feb 5, 2025

IvanAnishchuk Feb 5, 2025

IvanAnishchuk Feb 5, 2025

purshottam-sharma Feb 6, 2025

IvanAnishchuk Feb 6, 2025

IvanAnishchuk Feb 6, 2025

IvanAnishchuk commented Feb 6, 2025

IvanAnishchuk commented Feb 6, 2025

IvanAnishchuk left a comment

purshottam-sharma commented Feb 10, 2025

IvanAnishchuk left a comment

feat(pre-commit): add hook to enforce branch name validation rules #293

feat(pre-commit): add hook to enforce branch name validation rules #293

Conversation

purshottam-sharma commented Feb 5, 2025 • edited by IvanAnishchuk Loading

Description

Related Issue

Changes Made

How to Test

Checklist

Screenshots

IvanAnishchuk left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

IvanAnishchuk commented Feb 6, 2025

IvanAnishchuk commented Feb 6, 2025

IvanAnishchuk left a comment

Choose a reason for hiding this comment

purshottam-sharma commented Feb 10, 2025

IvanAnishchuk left a comment

Choose a reason for hiding this comment

purshottam-sharma commented Feb 5, 2025 •

edited by IvanAnishchuk

Loading