HYPERFLEET-878 - feat: pre-commit hooks registry with commitlint with Go binary#2
HYPERFLEET-878 - feat: pre-commit hooks registry with commitlint with Go binary#2
Conversation
…iner support Replaces the Node.js-based commit validation with a Go implementation for better performance and simplified cross-component deployment. - Implement commit message validator in Go with comprehensive test coverage - Add GitHub SDK integration for PR title validation (replaces gh CLI dependency) - Support both local pre-commit hooks and Prow CI environments - Build container image for reuse across all HyperFleet components - Add commit range detection with PULL_REFS priority for Prow accuracy - Create unified CLI with --pr flag for CI mode - Include comprehensive documentation for local and CI usage - Add Makefile with build, test, lint, and validation targets update
a284902 to
7c6f00e
Compare
This refactoring eliminates the external git binary dependency by using the go-git pure Go implementation, enabling a smaller container image and improving portability. Key changes: - Replace git CLI commands with go-git SDK for commit validation - Reduce final image size from 115 MB to 32.2 MB using ubi9-micro base - Add comprehensive unit tests (33.6% coverage) for commit operations - Fix context.Context anti-pattern in GitHub API client - Add .dockerignore to optimize build context - Consolidate repository operations to single open (performance) - Implement proper error handling with sentinel errors
| return fmt.Errorf("invalid head SHA: %w", err) | ||
| } | ||
|
|
||
| fmt.Fprintf(os.Stderr, "🔍 Validating commits in range: %s..%s\n", baseSHA[:8], headSHA[:8]) |
There was a problem hiding this comment.
Category: Bug
validateSHA() accepts SHAs with 7+ characters ({7,40}), but baseSHA[:8] here panics if the SHA is exactly 7 chars. Same issue at lines 176, 177, 181, 195.
Two options:
- Raise the minimum in
validateSHAto 8 (since[:8]is used everywhere):
shaPattern = regexp.MustCompile(`^[0-9a-f]{8,40}$`)- Use a helper that truncates safely:
func shortSHA(sha string) string {
if len(sha) <= 8 {
return sha
}
return sha[:8]
}Option 2 is more defensive since git itself can produce 7-char short SHAs.
| }) | ||
|
|
||
| // Save current directory | ||
| originalDir, _ := os.Getwd() |
There was a problem hiding this comment.
Category: Bug
os.Setenv/os.Unsetenv and os.Chdir are used without proper test isolation. If a subtest panics, env vars leak into the next subtest. Also, os.Getwd() error is silently ignored on line 170.
Use t.Setenv() and t.Chdir() instead — they restore state automatically, even on panic:
// Replace os.Chdir pattern (lines 170-174)
t.Chdir(tempDir) // Go 1.24+ — auto-restores on cleanup
// Replace os.Setenv/Unsetenv pattern (lines 215-222)
for k, v := range tt.env {
t.Setenv(k, v) // auto-restores when subtest ends
}
// Remove manual os.Unsetenv calls — no longer neededThis also removes the need for the final cleanup block at lines 241-244.
| var prNumber int | ||
| _, err := fmt.Sscanf(prNumberStr, "%d", &prNumber) | ||
| if err != nil { | ||
| return "", fmt.Errorf("invalid PULL_NUMBER: %s", prNumberStr) |
There was a problem hiding this comment.
Category: Bug
fmt.Sscanf returns a parse error but it's discarded — the error message only includes the raw string, losing the reason parsing failed.
// Before
return "", fmt.Errorf("invalid PULL_NUMBER: %s", prNumberStr)
// After — wrap the actual error
return "", fmt.Errorf("invalid PULL_NUMBER %q: %w", prNumberStr, err)Using %q also quotes the value, which helps debug whitespace or encoding issues in the env var.
| @@ -0,0 +1,76 @@ | |||
| package github | |||
There was a problem hiding this comment.
Category: Pattern
This file has zero test coverage. GetPRTitleFromEnv() has several testable code paths that don't require a real GitHub API call:
- Missing env vars (REPO_OWNER, REPO_NAME, PULL_NUMBER)
- Invalid PULL_NUMBER (non-numeric)
- Invalid repo format in
GetPRTitle()(missing/)
func TestGetPRTitleFromEnv_MissingEnvVars(t *testing.T) {
client := &Client{client: github.NewClient(nil)}
ctx := context.Background()
// All missing
_, err := client.GetPRTitleFromEnv(ctx)
if err == nil {
t.Error("expected error for missing env vars")
}
}
func TestGetPRTitle_InvalidRepoFormat(t *testing.T) {
client := &Client{client: github.NewClient(nil)}
ctx := context.Background()
_, err := client.GetPRTitle(ctx, "no-slash", 1)
if err == nil {
t.Error("expected error for invalid repo format")
}
}| return string(content), nil | ||
| } | ||
|
|
||
| func validatePR(validator *commitlint.Validator) error { |
There was a problem hiding this comment.
Category: Improvement
validatePR() is ~130 lines mixing 5+ concerns (git setup, commit range, SHA validation, commit validation loop, PR title fetch, summary output). Consider extracting:
- The commit validation loop (lines 168-197) into
validateCommits(validator, repo, commits) []string - The PR title validation block (lines 199-231) into
validatePRTitle(validator, client) (bool, error) - The summary output (lines 233-262) into
printSummary(failedCommits, prTitleFailed, commits, title)
This also makes each piece independently testable — right now validatePR is essentially untestable because it requires a real git repo with remote, env vars, and GitHub API access all at once.
| prTitleFailed := false | ||
| client := ghclient.NewClient() | ||
|
|
||
| ctx, cancel := context.WithTimeout(context.Background(), 30*time.Second) |
There was a problem hiding this comment.
Category: Improvement
context.Background() is used here but cmd.Context() is available from cobra — it carries cancellation signals (e.g., SIGINT). Pass the cobra context into validatePR:
// In run():
return validatePR(cmd.Context(), validator)
// In validatePR():
func validatePR(ctx context.Context, validator *commitlint.Validator) error {
// ...
ctx, cancel := context.WithTimeout(ctx, 30*time.Second)
defer cancel()
// ...
}This way the GitHub API call respects process cancellation.
| ### Architecture | ||
|
|
||
| ``` | ||
| ┌─────────────────────────────────┐ |
There was a problem hiding this comment.
Category: Standards
Several fenced code blocks in this file use bare ``` without a language identifier (MD040 violation). Add language tags:
- Line 142: ASCII diagram → use
```text - Line 204: commit format → use
```text - Line 237: valid examples → use
```text - Line 245: invalid examples → use
```text
|
Superseded by new PR with review feedback addressed. |
Achieve commit validation with a Go implementation for better performance and simplified cross-component deployment.
Summary
Test Plan
make test-allpassesmake lintpassesmake test-helm(if applicable)