-
Notifications
You must be signed in to change notification settings - Fork 0
feat: Implement validation for PR titles based on Conventional Commits and Jira references #11
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
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,11 @@ | ||
| # Agent Standards & Documentation | ||
|
|
||
| **ATTENTION AGENTS**: Before making changes, you **MUST** read the `README.md` files in the relevant directories (e.g., `.github/workflows/README.md`) to understand the current context and testing procedures. | ||
|
|
||
| ## Development Workflow for Agents | ||
|
|
||
| 1. **Read Docs**: Check `.github/workflows/README.md` for testing instructions. | ||
| 2. **Modify Script**: Edit the Deno script in the workflow directory. | ||
| 3. **Unit Test**: Run the unit tests using `deno test`. | ||
| 4. **Integration Test**: Run `bash .github/test/test-workflow-with-act.sh` to verify the workflow configuration. | ||
| 5. **Commit**: Push changes. |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,17 @@ | ||
| { | ||
| "action": "opened", | ||
| "number": 1, | ||
| "pull_request": { | ||
| "title": "feat(learning-activity): SC-12345-description-of-changes", | ||
| "body": "Test PR for acting locally", | ||
| "head": { | ||
| "ref": "D/conventional-commit-lint-workflow" | ||
| }, | ||
| "base": { | ||
| "ref": "main" | ||
| } | ||
| }, | ||
| "repository": { | ||
| "full_name": "simpleclub/.github" | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,50 @@ | ||
| #!/usr/bin/env bash | ||
| set -euo pipefail | ||
|
|
||
| # Helper to run the PR workflow locally with act | ||
| # Requires: act (https://github.com/nektos/act), Docker | ||
|
|
||
| WORKFLOW_FILE=".github/workflows/validate-pull-request-title/validate-pull-request-title.yml" | ||
| EVENT_FILE=".github/test/mock-events/pull_request_opened.json" | ||
| # Use a newer image that supports Node 24 (required by actions/setup-node@v6) | ||
| IMAGE="ghcr.io/catthehacker/ubuntu:act-22.04" | ||
|
|
||
| echo "Running workflow $WORKFLOW_FILE with event $EVENT_FILE" | ||
|
|
||
| # Detect Docker Host if not set | ||
| if [[ -z "${DOCKER_HOST-}" ]]; then | ||
| if command -v docker >/dev/null 2>&1; then | ||
| DETECTED_HOST=$(docker context inspect --format '{{.Endpoints.docker.Host}}' 2>/dev/null || true) | ||
| if [[ -n "$DETECTED_HOST" ]]; then | ||
| echo "Detected DOCKER_HOST: $DETECTED_HOST" | ||
| export DOCKER_HOST="$DETECTED_HOST" | ||
| fi | ||
| fi | ||
| fi | ||
|
|
||
| # If the user has a GITHUB_TOKEN set, use it. | ||
| # If not, try to get it from gh CLI. | ||
| # Otherwise, fall back to a dummy token (which might fail for cloning actions). | ||
| if [[ -z "${GITHUB_TOKEN-}" ]]; then | ||
| if command -v gh >/dev/null 2>&1; then | ||
| TOKEN=$(gh auth token 2>/dev/null || true) | ||
| if [[ -n "$TOKEN" ]]; then | ||
| echo "Using GITHUB_TOKEN from gh CLI" | ||
| export GITHUB_TOKEN="$TOKEN" | ||
| fi | ||
| fi | ||
| fi | ||
|
|
||
| if [[ -z "${GITHUB_TOKEN-}" ]]; then | ||
| echo "No GITHUB_TOKEN found; using dummy token (warning: this may fail action cloning)" | ||
| export GITHUB_TOKEN=dummy_token | ||
| fi | ||
|
|
||
| # Add architecture flag for Apple Silicon if needed | ||
| ARCH_ARGS="" | ||
| if [[ "$(uname -m)" == "arm64" ]]; then | ||
| ARCH_ARGS="--container-architecture linux/amd64" | ||
| fi | ||
|
|
||
| # Run act | ||
| act pull_request -W "$WORKFLOW_FILE" -e "$EVENT_FILE" -P ubuntu-latest=$IMAGE $ARCH_ARGS --env-file <(echo "GITHUB_TOKEN=$GITHUB_TOKEN") |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| # GitHub Actions Workflows | ||
|
|
||
| This directory contains the shared GitHub Actions workflows used across the organization. | ||
|
|
||
| ## Available Workflows | ||
|
|
||
| - **Validate Pull Request Title** (`validate-pull-request-title/`): Ensures PR titles follow Conventional Commits and include Jira ticket references. | ||
| - **Auto Approve** (`auto-approve.yml`): Automatically approves specific types of PRs (configuration details inside the file). | ||
|
|
||
| ## Testing Workflows | ||
|
|
||
| ### 1. Unit Tests (Deno) | ||
|
|
||
| Scripts used in workflows are written in JavaScript / TypeScript, and run by Deno. | ||
|
|
||
| - **Why Deno?**: Deno allows granular security permissions (e.g., explicit network/file access). Since these scripts run in a repository with global impact, this security control is worth the trade-off of deviating from more commonly used tech at simpleclub (Node/Python/Dart). | ||
| - **Requirement**: No `package.json`. Use standard Deno imports (e.g., `jsr:@std/...`). | ||
| - **Running Tests**: | ||
| ```bash | ||
| # Example for PR title validation | ||
| deno test .github/workflows/validate-pull-request-title/validate-pull-request-title.test.js | ||
| ``` | ||
|
|
||
| ### 2. Integration Tests (act) | ||
|
|
||
| We use [act](https://github.com/nektos/act) to simulate GitHub Actions locally using Docker. | ||
|
|
||
| **Prerequisites:** | ||
|
|
||
| - [Docker Desktop](https://www.docker.com/products/docker-desktop/) | ||
| - [act](https://github.com/nektos/act) | ||
|
|
||
| **Running Integration Tests:** | ||
| A helper script is available to run the workflows with mock events: | ||
|
|
||
| ```bash | ||
| # Run from the root of the repository | ||
| bash .github/test/test-workflow-with-act.sh | ||
| ``` | ||
|
|
||
| You can also run `act` directly if you prefer: | ||
|
|
||
| ```bash | ||
| act pull_request -W .github/workflows/validate-pull-request-title/validate-pull-request-title.yml --container-architecture linux/amd64 -e .github/test/mock-events/pull_request.json | ||
| ``` |
This file was deleted.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,19 @@ | ||
| # Validate Pull Request Title | ||
|
|
||
| This workflow validates that a Pull Request title adheres to the [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) specification and includes a Jira ticket reference. | ||
|
|
||
| ## Validation Rules | ||
|
|
||
| 1. **Prefix**: Must start with a valid type (e.g., `feat`, `fix`, `chore`) and optional scope. | ||
| - Regex: `^(feat|fix|docs|style|refactor|perf|test|build|ci|chore|revert)(\([a-z0-9-]+\))?:` | ||
| 2. **Ticket**: Must contain a ticket reference (e.g., `SC-1234`, `AI-5678`). | ||
| - Regex: `(SC|AI|PT)-[0-9]+` | ||
| 3. **Format**: The preferred format is `type(scope): TICKET subject`. | ||
|
|
||
| ## Usage | ||
|
|
||
| This workflow is triggered on `pull_request` events (opened, edited, reopened, synchronized). | ||
|
|
||
| ## Testing | ||
|
|
||
| For instructions on how to run unit tests and local integration tests, please refer to the [Workflows README](../README.md). |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,93 @@ | ||
| /** | ||
| * Validates the PR title against Conventional Commits and Jira Ticket requirements. | ||
| * @param {string} title | ||
| * @returns {{ isValid: boolean, errors: string[], warnings: string[] }} | ||
| */ | ||
| export function validateTitle(title) { | ||
| const errors = []; | ||
| const warnings = []; | ||
|
|
||
| // Regex Definitions | ||
| const REGEX_PREFIX = | ||
| /^(feat|fix|docs|style|refactor|perf|test|build|ci|chore|revert)(\([a-z0-9-]+\))?:/; | ||
| const REGEX_TICKET = /(SC|AI|PT)-[0-9]+/; | ||
| const REGEX_FULL = | ||
| /^(feat|fix|docs|style|refactor|perf|test|build|ci|chore|revert)(\([a-z0-9-]+\))?: (SC|AI|PT)-[0-9]+[- ].+$/; | ||
|
|
||
| // 1. Fast Path: Check if it matches the perfect format | ||
| if (REGEX_FULL.test(title)) { | ||
| return { isValid: true, errors, warnings }; | ||
| } | ||
|
|
||
| // 2. Granular Diagnostics | ||
| let failed = false; | ||
|
|
||
| // Check 1: Prefix | ||
| if (!REGEX_PREFIX.test(title)) { | ||
| errors.push( | ||
| "Title must start with a valid conventional commit prefix (e.g., 'feat:', 'fix(scope):')." | ||
| ); | ||
| errors.push( | ||
| "Allowed types: feat, fix, docs, style, refactor, perf, test, build, ci, chore, revert." | ||
| ); | ||
| failed = true; | ||
| } | ||
|
|
||
| // Check 2: Ticket Presence & Format | ||
| if (REGEX_TICKET.test(title)) { | ||
| // Ticket is present but full regex failed -> Warning | ||
| warnings.push("Ticket reference found, but the format is incorrect."); | ||
| warnings.push("Expected: `type(scope): TICKET subject`"); | ||
| warnings.push( | ||
| "Example: `feat(learning-activity): SC-12345 description of changes`" | ||
| ); | ||
| } else { | ||
| // Ticket is missing -> Error | ||
| errors.push("Ticket reference (e.g., SC-1234) is missing."); | ||
| failed = true; | ||
| } | ||
|
|
||
| return { isValid: !failed, errors, warnings }; | ||
| } | ||
|
|
||
| // CLI Execution | ||
| if (import.meta.main) { | ||
| const title = Deno.args[0]; | ||
| if (!title) { | ||
| console.error("Error: No title provided."); | ||
| Deno.exit(1); | ||
| } | ||
|
|
||
| const { isValid, errors, warnings } = validateTitle(title); | ||
|
|
||
| // Output Summary Header to stdout | ||
| console.log(`## PR Title Validation | ||
| Validating title: ${title} | ||
| `); | ||
|
|
||
| if (isValid) { | ||
| console.log("✅ PR title is valid."); | ||
| } else { | ||
| console.log("❌ PR title validation failed."); | ||
| } | ||
|
|
||
| // Output Errors | ||
| errors.forEach((err) => { | ||
| console.error(`::error::${err}`); // Annotation to stderr | ||
| console.log(`❌ ${err}`); // Markdown to stdout | ||
| }); | ||
|
|
||
| // Output Warnings | ||
| warnings.forEach((warn) => { | ||
| console.error(`::warning::${warn}`); // Annotation to stderr | ||
| console.log(`⚠️ ${warn}`); // Markdown to stdout | ||
| }); | ||
|
|
||
| console.log( | ||
| "See [Conventional Commits](https://www.conventionalcommits.org/en/v1.0.0/) for more details." | ||
| ); | ||
|
|
||
| if (!isValid) { | ||
| Deno.exit(1); | ||
| } | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,98 @@ | ||
| import { describe, it } from "jsr:@std/testing/bdd"; | ||
| import { expect } from "jsr:@std/expect"; | ||
| import { validateTitle } from "./validate-pull-request-title.js"; | ||
|
|
||
| describe("PR Title Validation", () => { | ||
| const validCases = [ | ||
| { | ||
| title: "feat(api): SC-1234 Add new endpoint", | ||
| desc: "accepts standard feature with scope and ticket", | ||
| }, | ||
| { | ||
| title: "fix: AI-5678 Fix crash on startup", | ||
| desc: "accepts fix with ticket and no scope", | ||
| }, | ||
| { | ||
| title: "chore: PT-999 Update dependencies", | ||
| desc: "accepts chore with PT ticket", | ||
| }, | ||
| { | ||
| title: "revert(auth): SC-0000 Revert bad commit", | ||
| desc: "accepts revert with scope and ticket", | ||
| }, | ||
| { | ||
| title: "feat(api): SC-1234-description-with-hyphens", | ||
| desc: "accepts hyphen as separator after ticket", | ||
| }, | ||
| ]; | ||
|
|
||
| describe("Valid Titles", () => { | ||
| validCases.forEach(({ title, desc }) => { | ||
| it(desc, () => { | ||
| const result = validateTitle(title); | ||
| expect(result.isValid).toBe(true); | ||
| expect(result.errors).toEqual([]); | ||
| expect(result.warnings).toEqual([]); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| const warningCases = [ | ||
| { | ||
| title: "feat: Add feature SC-1234", | ||
| desc: "warns when ticket is at the end", | ||
| expectedWarningMatch: /format is incorrect/, | ||
| }, | ||
| ]; | ||
|
|
||
| describe("Warnings", () => { | ||
| warningCases.forEach(({ title, desc, expectedWarningMatch }) => { | ||
| it(desc, () => { | ||
| const result = validateTitle(title); | ||
| expect(result.isValid).toBe(true); | ||
| expect(result.warnings.length).toBeGreaterThan(0); | ||
| expect(result.warnings[0]).toMatch(expectedWarningMatch); | ||
| }); | ||
| }); | ||
| }); | ||
|
|
||
| const invalidCases = [ | ||
| { | ||
| title: "feat: Add feature", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I also think we should allow no tickets. Though other changes like updating docs, READMEs, etc. would be too annoying to create an issue just to have an issue.
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. yes fair point -> i'll make it warn, but otherwise succeed |
||
| desc: "fails when ticket is missing", | ||
| expectedErrorMatch: /Ticket reference.*missing/, | ||
| }, | ||
| { | ||
| title: "random: SC-1234 Add feature", | ||
| desc: "fails when prefix is invalid", | ||
| expectedErrorMatch: /valid conventional commit prefix/, | ||
| }, | ||
| { | ||
| title: "feat(api) SC-1234 Missing colon", | ||
| desc: "fails when colon is missing", | ||
| expectedErrorMatch: /valid conventional commit prefix/, | ||
| }, | ||
| { | ||
| title: "Update README", | ||
| desc: "fails when completely malformed", | ||
| expectedErrorMatch: /valid conventional commit prefix/, | ||
| }, | ||
| { | ||
| title: "SC-1234 feat: Add feature", | ||
| desc: "fails when ticket is at the start (invalid prefix)", | ||
| expectedErrorMatch: /valid conventional commit prefix/, | ||
| }, | ||
| ]; | ||
|
|
||
| describe("Invalid Titles", () => { | ||
| invalidCases.forEach(({ title, desc, expectedErrorMatch }) => { | ||
| it(desc, () => { | ||
| const result = validateTitle(title); | ||
| expect(result.isValid).toBe(false); | ||
| expect(result.errors.some((e) => expectedErrorMatch.test(e))).toBe( | ||
| true | ||
| ); | ||
| }); | ||
| }); | ||
| }); | ||
| }); | ||
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 disagree that this should pass!
The changelog should be readable not some identifier.
This is also why I'm in favor of putting the ticket at the end, if you read the changelog the least important information is the ticket.
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.
we discussed briefly in backend check-in -> we don't care strongly about this -> i'll put it to the end