diff --git a/AGENTS.md b/AGENTS.md index cc75399..b39aa52 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -13,7 +13,7 @@ Go 1.26.0 · Cobra CLI · go-git · go-github · stretchr/testify · golangci-li | `make lint` | golangci-lint (pinned in tools/go.mod, config: `.golangci.yml`) | | `make test` | Unit tests with race detection + coverage profile | | `make test-coverage` | `make test` + opens HTML coverage report | -| `make validate-commits` | Build + `hyperfleet-hooks commitlint --pr` (CI mode) | +| `make validate-commits` | Build + `hyperfleet-hooks commitlint --pr --commits` (needs the Prow PR env) | | `make image` | Build container image (podman/docker auto-detected) | Pre-push order: `make lint` → `make test` → `make build`. @@ -23,7 +23,8 @@ Pre-push order: `make lint` → `make test` → `make build`. ```bash hyperfleet-hooks commitlint # Validate single commit message from file echo "feat: ..." | hyperfleet-hooks commitlint # Validate from stdin -hyperfleet-hooks commitlint --pr # CI mode: validate all PR commits + PR title +hyperfleet-hooks commitlint --pr # CI mode: validate the PR title (becomes the squashed commit) +hyperfleet-hooks commitlint --pr --commits # CI mode: also validate every commit in the PR range hyperfleet-hooks version # Print version info ``` @@ -36,6 +37,7 @@ hyperfleet-hooks version # Print version info | Pre-commit hook definitions | `.pre-commit-hooks.yaml` | | Validation logic | `pkg/commitlint/validator.go` | | GitHub API client | `pkg/github/client.go` | +| Prow JOB_SPEC parsing | `pkg/prow/jobspec.go` | | CLI command wiring | `cmd/hyperfleet-hooks/commitlint/commitlint.go` | | Linter config | `.golangci.yml` | | Container image | `Dockerfile` | @@ -47,7 +49,12 @@ Four pre-commit hooks defined in `.pre-commit-hooks.yaml`: - `hyperfleet-commitlint` — validates commit messages (Go binary, `language: golang`) - `hyperfleet-golangci-lint`, `hyperfleet-gofmt`, `hyperfleet-go-vet` — delegate to consuming repo's Make targets (`language: system`) -CI mode (`--pr`) commit range detection priority: +CI mode (`--pr`) validates the PR title only: PRs are squashed on merge, so the title is the commit that lands on `main`. Commits inside a PR need only the `: ` shape, enforced locally. PR title source priority: +1. `JOB_SPEC` env var (`refs.pulls[0].title`, parsed by `pkg/prow/jobspec.go`) +2. GitHub API via `REPO_OWNER` + `REPO_NAME` + `PULL_NUMBER` +3. Neither: fail (never warn and pass) + +Per-commit validation (`--pr --commits`) commit range detection priority: 1. `PULL_REFS` env var (Prow standard, most accurate) 2. `PULL_BASE_SHA` + `PULL_PULL_SHA` 3. `PULL_BASE_REF` + `HEAD` (local fallback) diff --git a/Makefile b/Makefile index 10de6a0..c6034d7 100644 --- a/Makefile +++ b/Makefile @@ -153,6 +153,6 @@ image-push: check-container-tool image ## Build and push container image # CI # ------------------------------------------------------------------------------ -validate-commits: build ## Validate commits in current branch (CI mode) +validate-commits: build ## Validate PR title and commits in current branch (needs JOB_SPEC or REPO_OWNER/REPO_NAME/PULL_NUMBER) @echo "Validating commits..." - @./bin/hyperfleet-hooks commitlint --pr + @./bin/hyperfleet-hooks commitlint --pr --commits diff --git a/README.md b/README.md index 460eae8..43e4ffd 100644 --- a/README.md +++ b/README.md @@ -34,7 +34,7 @@ From this point on, every `git commit` in that repository will validate the comm ### For CI (Prow) -In Prow, a pre-built container image (`quay.io/openshift-hyperfleet/hyperfleet-git-hooks`) is used to validate all commits and the PR title. See the [commitlint documentation](docs/commitlint.md) for Prow configuration details. +In Prow, a pre-built container image (`quay.io/openshift-hyperfleet/hyperfleet-git-hooks`) runs `hyperfleet-hooks commitlint --pr`, which validates the PR title. PRs are squashed on merge, so the PR title is the commit message that lands on `main` and it must carry the JIRA ticket. Commits inside a PR are not checked by CI: they only need the `: ` shape, which the local `commit-msg` hook enforces, so push review fixes as new commits rather than amending. See the [commitlint documentation](docs/commitlint.md) for Prow configuration details. ## Available Hooks @@ -47,7 +47,7 @@ In Prow, a pre-built container image (`quay.io/openshift-hyperfleet/hyperfleet-g ### Commitlint -Validates commit messages and PR titles against the [HyperFleet Commit Standard](https://github.com/openshift-hyperfleet/architecture/blob/main/hyperfleet/standards/commit-standard.md). +Validates commit messages locally and PR titles in CI against the [HyperFleet Commit Standard](https://github.com/openshift-hyperfleet/architecture/blob/main/hyperfleet/standards/commit-standard.md). In CI the PR title becomes the squashed commit, so that is what `--pr` validates; add `--commits` to also check every commit in the PR. **[→ Full Documentation](docs/commitlint.md)** diff --git a/cmd/hyperfleet-hooks/commitlint/commitlint.go b/cmd/hyperfleet-hooks/commitlint/commitlint.go index fb3c566..566069d 100644 --- a/cmd/hyperfleet-hooks/commitlint/commitlint.go +++ b/cmd/hyperfleet-hooks/commitlint/commitlint.go @@ -8,6 +8,7 @@ import ( "os" "os/exec" "regexp" + "strconv" "strings" "time" @@ -15,6 +16,7 @@ import ( "github.com/go-git/go-git/v5/plumbing" "github.com/openshift-hyperfleet/hyperfleet-hooks/pkg/commitlint" ghclient "github.com/openshift-hyperfleet/hyperfleet-hooks/pkg/github" + "github.com/openshift-hyperfleet/hyperfleet-hooks/pkg/prow" "github.com/spf13/cobra" ) @@ -25,12 +27,16 @@ var ( // errValidationFailed is returned when commit message validation fails errValidationFailed = errors.New("validation failed") + // errCommitsRequiresPR is returned when --commits is used without --pr + errCommitsRequiresPR = errors.New("--commits can only be used together with --pr") + // shaPattern matches valid git SHA (7-40 hex characters) shaPattern = regexp.MustCompile(`^[0-9a-f]{7,40}$`) ) var ( - pr bool + flagPR bool + flagCommits bool ) // NewCommand creates the commitlint command @@ -45,9 +51,19 @@ Usage: hyperfleet-hooks commitlint # from file (e.g., .git/COMMIT_EDITMSG) echo "feat: ..." | hyperfleet-hooks commitlint # from stdin - # CI: Validate entire PR (all commits + PR title) + # CI: Validate the PR title (the title becomes the squashed commit on merge) hyperfleet-hooks commitlint --pr + # CI: Also validate every commit in the PR range + hyperfleet-hooks commitlint --pr --commits + +In --pr mode the title is read from the JOB_SPEC environment variable that Prow +sets on every job, falling back to the GitHub API (REPO_OWNER, REPO_NAME, +PULL_NUMBER). Validation fails when no title can be obtained. + +Commits inside a PR are not validated by CI unless --commits is given: they only +need the : shape enforced locally by the commit-msg hook. + Examples: # Pre-commit hook (local) hyperfleet-hooks commitlint .git/COMMIT_EDITMSG @@ -61,7 +77,10 @@ Examples: RunE: run, } - cmd.Flags().BoolVar(&pr, "pr", false, "Validate all commits and PR title (CI mode, auto-detect from environment)") + cmd.Flags().BoolVar(&flagPR, "pr", false, + "Validate the PR title (CI mode, auto-detect from environment)") + cmd.Flags().BoolVar(&flagCommits, "commits", false, + "With --pr, also validate every commit in the PR range") return cmd } @@ -69,9 +88,13 @@ Examples: func run(cmd *cobra.Command, args []string) error { validator := commitlint.NewValidator() - // If --pr is provided, validate all commits and PR title - if pr { - return validatePR(cmd.Context(), validator) + if flagCommits && !flagPR { + return errCommitsRequiresPR + } + + // If --pr is provided, validate the PR title (and the commits with --commits) + if flagPR { + return validatePR(cmd.Context(), validator, flagCommits) } // Single commit validation (from file or stdin) @@ -129,25 +152,55 @@ func readStdin() (string, error) { return string(content), nil } -func validatePR(ctx context.Context, validator *commitlint.Validator) error { +// commitsResult holds the outcome of per-commit validation for the summary +type commitsResult struct { + commits []string + failed []string + passedCount int + checked bool +} + +// validatePR validates the PR title and, when withCommits is set, every commit in the PR range. +// The title is required: validation fails when it cannot be obtained. +func validatePR(ctx context.Context, validator *commitlint.Validator, withCommits bool) error { + var commits commitsResult + if withCommits { + result, err := validatePRCommits(ctx, validator) + if err != nil { + return err + } + commits = *result + } + + title, prTitleFailed, err := validatePRTitle(ctx, validator) + if err != nil { + return err + } + + return printSummary(commits, prTitleFailed, title) +} + +// validatePRCommits validates each commit in the PR range, preferring local git history +// and falling back to the GitHub API when the history is unavailable. +func validatePRCommits(ctx context.Context, validator *commitlint.Validator) (*commitsResult, error) { // Open git repository once and reuse repo, err := git.PlainOpen(".") if err != nil { - return fmt.Errorf("failed to open git repository: %w", err) + return nil, fmt.Errorf("failed to open git repository: %w", err) } // Get commit range from environment baseSHA, headSHA, err := getCommitRange(repo) if err != nil { - return fmt.Errorf("failed to get commit range: %w", err) + return nil, fmt.Errorf("failed to get commit range: %w", err) } // Validate SHA format if errBase := validateSHA(baseSHA); errBase != nil { - return fmt.Errorf("invalid base SHA: %w", errBase) + return nil, fmt.Errorf("invalid base SHA: %w", errBase) } if errHead := validateSHA(headSHA); errHead != nil { - return fmt.Errorf("invalid head SHA: %w", errHead) + return nil, fmt.Errorf("invalid head SHA: %w", errHead) } fmt.Fprintf(os.Stderr, "🔍 Validating commits in range: %s..%s\n", shortSHA(baseSHA), shortSHA(headSHA)) @@ -157,27 +210,24 @@ func validatePR(ctx context.Context, validator *commitlint.Validator) error { if err != nil { fmt.Fprintf(os.Stderr, "⚠️ Local git history unavailable: %v\n", err) fmt.Fprintln(os.Stderr, " Falling back to GitHub API...") - return validatePRFromAPI(ctx, validator) + return validatePRCommitsFromAPI(ctx, validator) } + result := &commitsResult{checked: true, commits: commits} if len(commits) == 0 { fmt.Fprintln(os.Stderr, "⚠️ No commits found in range") - return nil + return result, nil } fmt.Fprintf(os.Stderr, "📝 Found %d commit(s) to validate\n\n", len(commits)) // Validate each commit - failedCommits, passedCount := validateCommits(validator, repo, commits) + result.failed, result.passedCount = validateCommits(validator, repo, commits) - // Validate PR title - title, prTitleFailed := validatePRTitle(ctx, validator) - - // Print summary and return result - return printSummary(failedCommits, prTitleFailed, commits, passedCount, title) + return result, nil } -func validatePRFromAPI(ctx context.Context, validator *commitlint.Validator) error { +func validatePRCommitsFromAPI(ctx context.Context, validator *commitlint.Validator) (*commitsResult, error) { client := ghclient.NewClient() apiCtx, cancel := context.WithTimeout(ctx, 30*time.Second) @@ -185,50 +235,47 @@ func validatePRFromAPI(ctx context.Context, validator *commitlint.Validator) err prCommits, err := client.GetPRCommitsFromEnv(apiCtx) if err != nil { - return fmt.Errorf("failed to get commits from GitHub API: %w", err) + return nil, fmt.Errorf("failed to get commits from GitHub API: %w", err) } + result := &commitsResult{checked: true} if len(prCommits) == 0 { fmt.Fprintln(os.Stderr, "⚠️ No commits found in PR") - return nil + return result, nil } fmt.Fprintf(os.Stderr, "📝 Found %d commit(s) to validate (via GitHub API)\n\n", len(prCommits)) - var failedCommits []string - var passedCount int - commitSHAs := make([]string, 0, len(prCommits)) + result.commits = make([]string, 0, len(prCommits)) for _, pc := range prCommits { - commitSHAs = append(commitSHAs, pc.SHA) + result.commits = append(result.commits, pc.SHA) subject := strings.SplitN(strings.TrimSpace(pc.Message), "\n", 2)[0] fmt.Fprintf(os.Stderr, "Checking: %s - %s\n", shortSHA(pc.SHA), subject) if validator.IsWhitelistedAuthor(pc.AuthorEmail, pc.AuthorName) { fmt.Fprintf(os.Stderr, " ⏭️ SKIP (whitelisted author: %s)\n", pc.AuthorEmail) - passedCount++ + result.passedCount++ continue } - result := validator.Validate(strings.TrimSpace(pc.Message)) - if result.Valid { + res := validator.Validate(strings.TrimSpace(pc.Message)) + if res.Valid { fmt.Fprintln(os.Stderr, " ✅ PASS") - passedCount++ + result.passedCount++ } else { fmt.Fprintln(os.Stderr, " ❌ FAIL") fmt.Fprintln(os.Stderr, "") fmt.Fprintln(os.Stderr, " Error details:") - for _, e := range result.Errors { + for _, e := range res.Errors { fmt.Fprintf(os.Stderr, " ✖ %s\n", e.Error()) } fmt.Fprintln(os.Stderr, "") - failedCommits = append(failedCommits, shortSHA(pc.SHA)) + result.failed = append(result.failed, shortSHA(pc.SHA)) } } - title, prTitleFailed := validatePRTitle(ctx, validator) - - return printSummary(failedCommits, prTitleFailed, commitSHAs, passedCount, title) + return result, nil } // validateCommits validates each commit message and returns failed commit SHAs and passed count @@ -274,36 +321,81 @@ func validateCommits( return failedCommits, passedCount } -// validatePRTitle fetches and validates the PR title via GitHub API -func validatePRTitle(ctx context.Context, validator *commitlint.Validator) (title string, failed bool) { - fmt.Fprintln(os.Stderr, "") - fmt.Fprintln(os.Stderr, "━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━") - fmt.Fprintln(os.Stderr, "📋 Validating PR title...") +// prTitle is a PR title together with where it was read from +type prTitle struct { + title string + author string + number string + source string +} + +// getPRTitle resolves the PR title. +// Priority: JOB_SPEC (set by Prow on every job) > GitHub API (REPO_OWNER, REPO_NAME, PULL_NUMBER). +// It returns an error when neither source yields a title. +func getPRTitle(ctx context.Context) (*prTitle, error) { + var jobSpecErr error + if raw := os.Getenv(prow.JobSpecEnv); raw != "" { + pull, err := prow.PullFromJobSpec(raw) + if err == nil { + return &prTitle{ + title: pull.Title, + author: pull.Author, + number: strconv.Itoa(pull.Number), + source: prow.JobSpecEnv, + }, nil + } + jobSpecErr = err + fmt.Fprintf(os.Stderr, "⚠️ Could not read PR title from %s: %v\n", prow.JobSpecEnv, err) + fmt.Fprintln(os.Stderr, " Falling back to GitHub API...") + } client := ghclient.NewClient() - ctx, cancel := context.WithTimeout(ctx, 30*time.Second) + apiCtx, cancel := context.WithTimeout(ctx, 30*time.Second) defer cancel() - prInfo, prNumberStr, err := client.GetPRInfoFromEnv(ctx) + info, number, err := client.GetPRInfoFromEnv(apiCtx) if err != nil { - fmt.Fprintf(os.Stderr, "⚠️ Could not fetch PR title: %v\n", err) - fmt.Fprintln(os.Stderr, " Skipping PR title validation") - return "", false + if jobSpecErr != nil { + return nil, fmt.Errorf("could not get PR title: %s: %w; GitHub API: %w", prow.JobSpecEnv, jobSpecErr, err) + } + return nil, fmt.Errorf("could not get PR title: %s not set; GitHub API: %w", prow.JobSpecEnv, err) } - title = prInfo.Title - fmt.Fprintf(os.Stderr, "PR #%s: %s\n", prNumberStr, title) + return &prTitle{ + title: info.Title, + author: info.AuthorLogin, + number: number, + source: "GitHub API", + }, nil +} + +// validatePRTitle resolves and validates the PR title. +// It returns an error when the title cannot be obtained, and failed=true when it does not +// meet the standard. +func validatePRTitle(ctx context.Context, validator *commitlint.Validator) (title string, failed bool, err error) { + fmt.Fprintln(os.Stderr, "") + fmt.Fprintln(os.Stderr, "━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━") + fmt.Fprintln(os.Stderr, "📋 Validating PR title...") - if validator.IsWhitelistedAuthor(prInfo.AuthorLogin) { - fmt.Fprintf(os.Stderr, " ⏭️ SKIP (whitelisted author: %s)\n", prInfo.AuthorLogin) - return title, false + info, err := getPRTitle(ctx) + if err != nil { + fmt.Fprintf(os.Stderr, " ❌ %v\n", err) + return "", true, err + } + + title = info.title + fmt.Fprintf(os.Stderr, "PR #%s (via %s): %s\n", info.number, info.source, title) + + if validator.IsWhitelistedAuthor(info.author) { + fmt.Fprintf(os.Stderr, " ⏭️ SKIP (whitelisted author: %s)\n", info.author) + return title, false, nil } result := validator.ValidatePRTitle(title) if result.Valid { fmt.Fprintln(os.Stderr, " ✅ PASS") - return title, false + return title, false, nil } fmt.Fprintln(os.Stderr, " ❌ FAIL") @@ -313,16 +405,18 @@ func validatePRTitle(ctx context.Context, validator *commitlint.Validator) (titl fmt.Fprintf(os.Stderr, " ✖ %s\n", e.Error()) } fmt.Fprintln(os.Stderr, "") - return title, true + return title, true, nil } // printSummary prints the validation summary and returns an error if any validation failed -func printSummary(failedCommits []string, prTitleFailed bool, commits []string, passedCount int, title string) error { +func printSummary(commits commitsResult, prTitleFailed bool, title string) error { fmt.Fprintln(os.Stderr, "") fmt.Fprintln(os.Stderr, "━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━") - if len(failedCommits) == 0 && !prTitleFailed { - fmt.Fprintf(os.Stderr, "✅ All %d commit(s) passed validation!\n", passedCount) + if len(commits.failed) == 0 && !prTitleFailed { + if commits.checked { + fmt.Fprintf(os.Stderr, "✅ All %d commit(s) passed validation!\n", commits.passedCount) + } if title != "" { fmt.Fprintln(os.Stderr, "✅ PR title passed validation!") } @@ -330,8 +424,8 @@ func printSummary(failedCommits []string, prTitleFailed bool, commits []string, return nil } - if len(failedCommits) > 0 { - fmt.Fprintf(os.Stderr, "❌ %d of %d commit(s) failed validation\n", len(failedCommits), len(commits)) + if len(commits.failed) > 0 { + fmt.Fprintf(os.Stderr, "❌ %d of %d commit(s) failed validation\n", len(commits.failed), len(commits.commits)) } if prTitleFailed { fmt.Fprintln(os.Stderr, "❌ PR title failed validation") @@ -342,6 +436,7 @@ func printSummary(failedCommits []string, prTitleFailed bool, commits []string, fmt.Fprintln(os.Stderr, " Format: [- - ]: ") fmt.Fprintln(os.Stderr, " Types: feat, fix, docs, style, refactor, perf, test, build, ci, chore, revert") fmt.Fprintln(os.Stderr, " Max length: 72 chars (excluding JIRA prefix)") + fmt.Fprintln(os.Stderr, " PR titles must include the JIRA ticket: the title becomes the squashed commit") fmt.Fprintln(os.Stderr, "") fmt.Fprintf(os.Stderr, " Full spec: %s\n", commitStandardURL) diff --git a/cmd/hyperfleet-hooks/commitlint/commitlint_test.go b/cmd/hyperfleet-hooks/commitlint/commitlint_test.go index 40dc832..5e60845 100644 --- a/cmd/hyperfleet-hooks/commitlint/commitlint_test.go +++ b/cmd/hyperfleet-hooks/commitlint/commitlint_test.go @@ -1,6 +1,7 @@ package commitlint import ( + "context" "os" "path/filepath" "testing" @@ -523,3 +524,265 @@ func TestShortSHA(t *testing.T) { }) } } + +// prEnvVars lists every environment variable the PR mode reads, so tests can start clean +var prEnvVars = []string{ + "JOB_SPEC", "REPO_OWNER", "REPO_NAME", "PULL_NUMBER", "GITHUB_TOKEN", + "PULL_REFS", "PULL_BASE_SHA", "PULL_PULL_SHA", "PULL_BASE_REF", +} + +func clearPREnv(t *testing.T) { + t.Helper() + for _, key := range prEnvVars { + t.Setenv(key, "") + } +} + +func jobSpecJSON(title, author string) string { + return `{"type":"presubmit","job":"pull-ci-validate-commits","refs":{` + + `"org":"openshift-hyperfleet","repo":"hyperfleet-api","base_ref":"main",` + + `"pulls":[{"number":42,"author":"` + author + `","sha":"abc1234","title":"` + title + `"}]}}` +} + +// newRepoWithCommit creates a temp repo, chdirs into it, commits one file with the given +// message and author, and returns the repo and the commit hash +func newRepoWithCommit(t *testing.T, message, authorName, authorEmail string) (*git.Repository, string) { + t.Helper() + tempDir := t.TempDir() + repo, err := git.PlainInit(tempDir, false) + require.NoError(t, err) + + worktree, err := repo.Worktree() + require.NoError(t, err) + + require.NoError(t, os.WriteFile(filepath.Join(tempDir, "test.txt"), []byte("test"), 0644)) + _, err = worktree.Add("test.txt") + require.NoError(t, err) + + hash, err := worktree.Commit(message, &git.CommitOptions{ + Author: &object.Signature{Name: authorName, Email: authorEmail}, + }) + require.NoError(t, err) + + t.Chdir(tempDir) + return repo, hash.String() +} + +// addCommit commits one more file to repo with the given message and returns the commit hash +func addCommit(t *testing.T, repo *git.Repository, message, filename string) string { + t.Helper() + worktree, err := repo.Worktree() + require.NoError(t, err) + require.NoError(t, os.WriteFile(filename, []byte(filename), 0644)) + _, err = worktree.Add(filename) + require.NoError(t, err) + + hash, err := worktree.Commit(message, &git.CommitOptions{ + Author: &object.Signature{Name: "developer", Email: "developer@redhat.com"}, + }) + require.NoError(t, err) + return hash.String() +} + +// newRepoWithBadCommit creates a repo whose head is a commit that fails the commit rules, +// on top of a valid base commit, and returns the base and head hashes so a PULL_REFS +// range of base..head contains exactly the bad commit +func newRepoWithBadCommit(t *testing.T) (baseHash, badHash string) { + t.Helper() + repo, baseHash := newRepoWithCommit(t, "feat: base commit", "developer", "developer@redhat.com") + badHash = addCommit(t, repo, "this does not conform to the standard", "bad.txt") + return baseHash, badHash +} + +func TestGetPRTitle(t *testing.T) { + tests := []struct { + env map[string]string + name string + wantTitle string + wantAuthor string + wantNumber string + wantSource string + wantErr bool + }{ + { + name: "JOB_SPEC: valid spec yields title without touching the API", + env: map[string]string{ + "JOB_SPEC": jobSpecJSON("HYPERFLEET-42 - feat: add thing", "developer"), + }, + wantTitle: "HYPERFLEET-42 - feat: add thing", + wantAuthor: "developer", + wantNumber: "42", + wantSource: "JOB_SPEC", + wantErr: false, + }, + { + name: "JOB_SPEC: wins over GitHub API env", + env: map[string]string{ + "JOB_SPEC": jobSpecJSON("HYPERFLEET-42 - feat: add thing", "developer"), + "REPO_OWNER": "openshift-hyperfleet", + "REPO_NAME": "hyperfleet-api", + "PULL_NUMBER": "99", + }, + wantTitle: "HYPERFLEET-42 - feat: add thing", + wantAuthor: "developer", + wantNumber: "42", + wantSource: "JOB_SPEC", + wantErr: false, + }, + { + name: "no source: JOB_SPEC unset and no API env", + env: map[string]string{}, + wantErr: true, + }, + { + name: "no source: JOB_SPEC has no pulls and no API env", + env: map[string]string{ + "JOB_SPEC": `{"type":"periodic","job":"nightly"}`, + }, + wantErr: true, + }, + { + name: "no source: JOB_SPEC malformed and no API env", + env: map[string]string{ + "JOB_SPEC": `{"refs":{"pulls":[`, + }, + wantErr: true, + }, + { + name: "no source: JOB_SPEC pull has empty title and no API env", + env: map[string]string{ + "JOB_SPEC": jobSpecJSON("", "developer"), + }, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + clearPREnv(t) + for k, v := range tt.env { + t.Setenv(k, v) + } + + got, err := getPRTitle(context.Background()) + + if (err != nil) != tt.wantErr { + t.Errorf("getPRTitle() error = %v, wantErr %v", err, tt.wantErr) + return + } + if tt.wantErr { + return + } + + require.Equal(t, tt.wantTitle, got.title) + require.Equal(t, tt.wantAuthor, got.author) + require.Equal(t, tt.wantNumber, got.number) + require.Equal(t, tt.wantSource, got.source) + }) + } +} + +func TestValidatePR_TitleOnlyIgnoresCommitMessages(t *testing.T) { + clearPREnv(t) + + // The range contains exactly one commit, and it fails the commit rules outright + baseHash, badHash := newRepoWithBadCommit(t) + t.Setenv("PULL_REFS", "main:"+baseHash+",42:"+badHash) + t.Setenv("JOB_SPEC", jobSpecJSON("HYPERFLEET-42 - feat: add thing", "developer")) + + validator := pkgcommitlint.NewValidator() + + err := validatePR(context.Background(), validator, false) + require.NoError(t, err, "--pr alone validates the title only, so a bad commit must not fail it") +} + +func TestValidatePR_WithCommitsValidatesCommitMessages(t *testing.T) { + clearPREnv(t) + + // Same range as the title-only test, so the two differ only in the flag + baseHash, badHash := newRepoWithBadCommit(t) + t.Setenv("PULL_REFS", "main:"+baseHash+",42:"+badHash) + t.Setenv("JOB_SPEC", jobSpecJSON("HYPERFLEET-42 - feat: add thing", "developer")) + + validator := pkgcommitlint.NewValidator() + + err := validatePR(context.Background(), validator, true) + require.ErrorIs(t, err, errValidationFailed, "--commits must still fail on a bad commit message") +} + +func TestValidatePR_FailsWhenNoTitleAvailable(t *testing.T) { + clearPREnv(t) + + // Commits are fine; only the title source is missing + _, goodHash := newRepoWithCommit(t, "feat: add thing", "developer", "developer@redhat.com") + t.Setenv("PULL_REFS", "main:"+goodHash+",42:"+goodHash) + + validator := pkgcommitlint.NewValidator() + + err := validatePR(context.Background(), validator, false) + require.Error(t, err, "missing title must fail instead of passing silently") + require.NotErrorIs(t, err, errValidationFailed, "missing title is a lookup error, not a rule failure") + require.Contains(t, err.Error(), "could not get PR title") +} + +func TestValidatePR_TitleRules(t *testing.T) { + tests := []struct { + wantErr error + name string + title string + author string + }{ + { + name: "valid: JIRA prefix and type", + title: "HYPERFLEET-42 - feat: add thing", + author: "developer", + wantErr: nil, + }, + { + name: "invalid: missing JIRA prefix", + title: "feat: add thing", + author: "developer", + wantErr: errValidationFailed, + }, + { + name: "invalid: bad type", + title: "HYPERFLEET-42 - feature: add thing", + author: "developer", + wantErr: errValidationFailed, + }, + { + name: "skipped: whitelisted bot author with non-conforming title", + title: "Red Hat Konflux update hyperfleet-api", + author: "red-hat-konflux-kflux-prd-rh02[bot]", + wantErr: nil, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + clearPREnv(t) + t.Setenv("JOB_SPEC", jobSpecJSON(tt.title, tt.author)) + + validator := pkgcommitlint.NewValidator() + + err := validatePR(context.Background(), validator, false) + if tt.wantErr == nil { + require.NoError(t, err) + return + } + require.ErrorIs(t, err, tt.wantErr) + }) + } +} + +func TestRun_CommitsRequiresPR(t *testing.T) { + cmd := NewCommand() + require.NoError(t, cmd.Flags().Set("commits", "true")) + t.Cleanup(func() { + flagPR = false + flagCommits = false + }) + + err := run(cmd, nil) + require.ErrorIs(t, err, errCommitsRequiresPR) +} diff --git a/docs/commitlint.md b/docs/commitlint.md index fddc2a1..15ec49b 100644 --- a/docs/commitlint.md +++ b/docs/commitlint.md @@ -17,11 +17,13 @@ Complete guide for using hyperfleet-hooks commitlint in local development and Pr `hyperfleet-hooks commitlint` validates commit messages against the HyperFleet Commit Standard, based on [Conventional Commits](https://www.conventionalcommits.org/). **Key Features**: -- ✅ Validates commit message format -- ✅ Validates PR titles in CI -- ✅ Works locally (pre-commit) and in Prow CI +- ✅ Validates commit message format locally (pre-commit) +- ✅ Validates the PR title in Prow CI (the title becomes the squashed commit) +- ✅ Optional per-commit validation in CI with `--commits` - ✅ Zero configuration for other components +**What is checked where**: CI validates the PR title, which becomes the squashed commit on `main`. Commits inside a PR need only the `: ` shape, enforced locally by the `commit-msg` hook. Push review fixes as new commits instead of amending. + ## Local Development ### Installation @@ -62,16 +64,18 @@ echo "feat: add new feature" | hyperfleet-hooks commitlint ### Overview `hyperfleet-hooks commitlint --pr` automatically: -1. Detects commit range from environment variables -2. Validates all commits in the PR -3. Validates PR title via GitHub API -4. Reports results +1. Reads the PR title from the `JOB_SPEC` environment variable that Prow sets on every job +2. Falls back to the GitHub API (`REPO_OWNER`, `REPO_NAME`, `PULL_NUMBER`) when `JOB_SPEC` has no pull request +3. Fails the job when no title can be obtained from either source +4. Validates the PR title and reports the result + +With `--pr --commits` it also detects the commit range from the environment and validates every commit in the PR before the title. ### PR Title Requirements PR titles must include JIRA ticket: `- - : ` -*Why?* For squash merges, PR title becomes the final commit message with traceability to JIRA. +*Why?* PRs are squashed on merge, so the PR title becomes the commit message on `main` and carries the traceability to JIRA. Individual commits inside the PR do not reach `main`, which is why CI does not check them. Examples: - ✅ `HYPERFLEET-123 - feat: add cluster validation` @@ -111,18 +115,24 @@ Prow automatically provides these variables (no configuration needed): | Variable | Example | Description | |----------|---------|-------------| -| `PULL_REFS` | `main:abc123,456:def789` | Base and PR SHAs (most accurate) | -| `PULL_BASE_SHA` | `abc123...` | Base branch SHA (fallback) | -| `PULL_PULL_SHA` | `def789...` | PR head SHA (fallback) | -| `PULL_BASE_REF` | `main` | Target branch name | -| `PULL_NUMBER` | `123` | PR number | -| `REPO_OWNER` | `openshift-hyperfleet` | Repository owner | -| `REPO_NAME` | `hyperfleet-api` | Repository name | -| `GITHUB_TOKEN` | `ghp_...` | GitHub API token (optional) | - -> **Note**: If `GITHUB_TOKEN` is not set, the tool uses the unauthenticated GitHub API (60 req/hr rate limit) and prints a warning to stderr. Set `GITHUB_TOKEN` in CI environments to avoid hitting rate limits. - -**Commit range detection priority**: +| `JOB_SPEC` | `{"refs":{"pulls":[{"number":123,"title":"..."}]}}` | Prow job spec JSON, the primary source of the PR title and author | +| `PULL_NUMBER` | `123` | PR number (GitHub API fallback for the title) | +| `REPO_OWNER` | `openshift-hyperfleet` | Repository owner (GitHub API fallback) | +| `REPO_NAME` | `hyperfleet-api` | Repository name (GitHub API fallback) | +| `GITHUB_TOKEN` | `ghp_...` | GitHub API token (optional, only used by the fallback) | +| `PULL_REFS` | `main:abc123,456:def789` | Base and PR SHAs for `--commits` (most accurate) | +| `PULL_BASE_SHA` | `abc123...` | Base branch SHA for `--commits` (fallback) | +| `PULL_PULL_SHA` | `def789...` | PR head SHA for `--commits` (fallback) | +| `PULL_BASE_REF` | `main` | Target branch name for `--commits` | + +> **Note**: If `GITHUB_TOKEN` is not set, the GitHub API fallback is unauthenticated (60 req/hr rate limit) and prints a warning to stderr. In Prow the title normally comes from `JOB_SPEC`, so the API is only reached when that spec has no pull request. + +**PR title source priority**: +1. `JOB_SPEC` (`refs.pulls[0].title`, set by Prow on every decorated job) +2. GitHub API via `REPO_OWNER`, `REPO_NAME`, `PULL_NUMBER` +3. Neither available: the job fails + +**Commit range detection priority** (`--commits` only): 1. `PULL_REFS` (most accurate) 2. `PULL_BASE_SHA` + `PULL_PULL_SHA` 3. `PULL_BASE_REF` + `HEAD` (local fallback) @@ -180,9 +190,12 @@ git commit -m "ci: update hyperfleet-git-hooks to v1.1.0" # Validate single commit message hyperfleet-hooks commitlint [file] -# Validate entire PR (commits + title) +# Validate the PR title (CI mode) hyperfleet-hooks commitlint --pr +# Validate the PR title and every commit in the PR +hyperfleet-hooks commitlint --pr --commits + # Show version hyperfleet-hooks version ``` @@ -191,7 +204,8 @@ hyperfleet-hooks version | Flag | Description | |------|-------------| -| `--pr` | Validate all commits and PR title (CI mode) | +| `--pr` | Validate the PR title (CI mode, title read from `JOB_SPEC` or the GitHub API) | +| `--commits` | With `--pr`, also validate every commit in the PR range | | `-h, --help` | Show help | ### Exit Codes @@ -259,6 +273,7 @@ Common errors: - `type must be one of` → Use valid type (feat, fix, docs, etc.) - `header must not exceed 72 characters` → `: ` must be ≤ 72 chars (excluding JIRA prefix) - `pr-title-requires-jira` → PR titles must start with `- - ` +- `could not get PR title` → neither `JOB_SPEC` nor the GitHub API env (`REPO_OWNER`, `REPO_NAME`, `PULL_NUMBER`) yielded a title; check the Prow job is decorated Hook issues: - Reinstall: `pre-commit install --hook-type commit-msg` diff --git a/pkg/prow/jobspec.go b/pkg/prow/jobspec.go new file mode 100644 index 0000000..90bdbf3 --- /dev/null +++ b/pkg/prow/jobspec.go @@ -0,0 +1,72 @@ +// Package prow reads the job metadata Prow injects into every decorated job. +package prow + +import ( + "encoding/json" + "fmt" +) + +// JobSpecEnv is the environment variable Prow sets to the JSON-encoded job spec. +const JobSpecEnv = "JOB_SPEC" + +// JobSpec is the subset of the Prow job spec this tool reads. +// It mirrors prowapi.JobSpec, keeping only the fields needed here. +type JobSpec struct { + Refs *Refs `json:"refs,omitempty"` + Type string `json:"type,omitempty"` + Job string `json:"job,omitempty"` +} + +// Refs describes the repository and pull requests a job runs against. +// It mirrors prowapi.Refs. +type Refs struct { + Org string `json:"org"` + Repo string `json:"repo"` + BaseRef string `json:"base_ref,omitempty"` + BaseSHA string `json:"base_sha,omitempty"` + Pulls []Pull `json:"pulls,omitempty"` +} + +// Pull describes a single pull request in the job refs. +// It mirrors prowapi.Pull. +type Pull struct { + Author string `json:"author"` + SHA string `json:"sha"` + Title string `json:"title,omitempty"` + Number int `json:"number"` +} + +// ParseJobSpec decodes a JSON-encoded Prow job spec. +func ParseJobSpec(raw string) (*JobSpec, error) { + if raw == "" { + return nil, fmt.Errorf("%s is empty", JobSpecEnv) + } + + var spec JobSpec + if err := json.Unmarshal([]byte(raw), &spec); err != nil { + return nil, fmt.Errorf("invalid %s JSON: %w", JobSpecEnv, err) + } + + return &spec, nil +} + +// PullFromJobSpec returns the first pull request in a JSON-encoded Prow job spec. +// It fails when the spec is malformed, carries no pull requests (for example on a +// periodic or postsubmit job), or the pull request has no title. +func PullFromJobSpec(raw string) (*Pull, error) { + spec, err := ParseJobSpec(raw) + if err != nil { + return nil, err + } + + if spec.Refs == nil || len(spec.Refs.Pulls) == 0 { + return nil, fmt.Errorf("%s has no pull requests in refs", JobSpecEnv) + } + + pull := spec.Refs.Pulls[0] + if pull.Title == "" { + return nil, fmt.Errorf("%s pull request #%d has no title", JobSpecEnv, pull.Number) + } + + return &pull, nil +} diff --git a/pkg/prow/jobspec_test.go b/pkg/prow/jobspec_test.go new file mode 100644 index 0000000..f0e5092 --- /dev/null +++ b/pkg/prow/jobspec_test.go @@ -0,0 +1,161 @@ +package prow + +import ( + "testing" +) + +const validJobSpec = `{ + "type": "presubmit", + "job": "pull-ci-openshift-hyperfleet-hyperfleet-api-main-validate-commits", + "refs": { + "org": "openshift-hyperfleet", + "repo": "hyperfleet-api", + "base_ref": "main", + "base_sha": "a1b2c3d4e5f6a7b8c9d0e1f2a3b4c5d6e7f8a9b0", + "pulls": [ + { + "number": 123, + "author": "developer", + "sha": "0b9a8f7e6d5c4b3a2f1e0d9c8b7a6f5e4d3c2b1a", + "title": "HYPERFLEET-123 - feat: add cluster validation" + } + ] + } +}` + +func TestParseJobSpec(t *testing.T) { + tests := []struct { + name string + raw string + wantJob string + wantErr bool + }{ + { + name: "valid: presubmit spec", + raw: validJobSpec, + wantJob: "pull-ci-openshift-hyperfleet-hyperfleet-api-main-validate-commits", + wantErr: false, + }, + { + name: "valid: periodic spec without refs", + raw: `{"type":"periodic","job":"nightly"}`, + wantJob: "nightly", + wantErr: false, + }, + { + name: "invalid: empty string", + raw: "", + wantErr: true, + }, + { + name: "invalid: malformed JSON", + raw: `{"type":"presubmit","refs":{`, + wantErr: true, + }, + { + name: "invalid: not an object", + raw: `"presubmit"`, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + spec, err := ParseJobSpec(tt.raw) + + if (err != nil) != tt.wantErr { + t.Errorf("ParseJobSpec() error = %v, wantErr %v", err, tt.wantErr) + return + } + + if !tt.wantErr && spec.Job != tt.wantJob { + t.Errorf("ParseJobSpec() Job = %q, want %q", spec.Job, tt.wantJob) + } + }) + } +} + +func TestPullFromJobSpec(t *testing.T) { + tests := []struct { + name string + raw string + wantAuthor string + wantTitle string + wantNumber int + wantErr bool + }{ + { + name: "valid: single pull request", + raw: validJobSpec, + wantNumber: 123, + wantAuthor: "developer", + wantTitle: "HYPERFLEET-123 - feat: add cluster validation", + wantErr: false, + }, + { + name: "valid: batch job uses the first pull request", + raw: `{"type":"batch","refs":{"org":"o","repo":"r","pulls":[` + + `{"number":1,"author":"first","sha":"abc1234","title":"HYPERFLEET-1 - fix: first"},` + + `{"number":2,"author":"second","sha":"def5678","title":"HYPERFLEET-2 - fix: second"}]}}`, + wantNumber: 1, + wantAuthor: "first", + wantTitle: "HYPERFLEET-1 - fix: first", + wantErr: false, + }, + { + name: "invalid: malformed JSON", + raw: `{"refs":{"pulls":[}`, + wantErr: true, + }, + { + name: "invalid: empty string", + raw: "", + wantErr: true, + }, + { + name: "invalid: missing refs", + raw: `{"type":"periodic","job":"nightly"}`, + wantErr: true, + }, + { + name: "invalid: missing pulls", + raw: `{"type":"postsubmit","refs":{"org":"o","repo":"r","base_ref":"main"}}`, + wantErr: true, + }, + { + name: "invalid: empty pulls list", + raw: `{"type":"presubmit","refs":{"org":"o","repo":"r","pulls":[]}}`, + wantErr: true, + }, + { + name: "invalid: pull request without title", + raw: `{"type":"presubmit","refs":{"org":"o","repo":"r","pulls":[{"number":7,"author":"dev","sha":"abc1234"}]}}`, + wantErr: true, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + pull, err := PullFromJobSpec(tt.raw) + + if (err != nil) != tt.wantErr { + t.Errorf("PullFromJobSpec() error = %v, wantErr %v", err, tt.wantErr) + return + } + + if tt.wantErr { + return + } + + if pull.Number != tt.wantNumber { + t.Errorf("PullFromJobSpec() Number = %d, want %d", pull.Number, tt.wantNumber) + } + if pull.Author != tt.wantAuthor { + t.Errorf("PullFromJobSpec() Author = %q, want %q", pull.Author, tt.wantAuthor) + } + if pull.Title != tt.wantTitle { + t.Errorf("PullFromJobSpec() Title = %q, want %q", pull.Title, tt.wantTitle) + } + }) + } +}