From 0b09b97cca7810a3478387c37a7de9ff7b043849 Mon Sep 17 00:00:00 2001 From: Francesco Romani Date: Thu, 17 Jul 2025 08:45:37 +0200 Subject: [PATCH 1/5] [KNI] ci: rewrite the commit-msg hook Rewrite the script that verifies the commit message in go, taking into consideration the project rules described in RESYNC.README.md. Signed-off-by: Shereen Haj --- .github/workflows/ci.yaml | 4 + Makefile.kni | 5 + hack-kni/tools/verifycommit/config.json | 4 + .../test-config-missing-origin.json | 3 + .../verifycommit/verify-commit-message.go | 223 +++++++++++++++ .../verify-commit-message_test.go | 258 ++++++++++++++++++ hack-kni/verify-commits.sh | 11 +- 7 files changed, 500 insertions(+), 8 deletions(-) create mode 100644 hack-kni/tools/verifycommit/config.json create mode 100644 hack-kni/tools/verifycommit/test-config-missing-origin.json create mode 100644 hack-kni/tools/verifycommit/verify-commit-message.go create mode 100644 hack-kni/tools/verifycommit/verify-commit-message_test.go diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index bcc15954c1..776a3ec1c9 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -26,6 +26,10 @@ jobs: echo ${{ env.SOURCE_BRANCH_NAME }} echo ${{ env.TARGET_BRANCH_NAME }} + - name: Build verification tool + run: | + make -f Makefile.kni build-tools + - name: Verify commits run: | TRIGGER_BRANCH=${{ env.SOURCE_BRANCH_NAME }} \ diff --git a/Makefile.kni b/Makefile.kni index 74c3962569..57b1cfec86 100644 --- a/Makefile.kni +++ b/Makefile.kni @@ -90,3 +90,8 @@ verify-crdgen: update-vendor .PHONY: clean clean: rm -rf ./bin + +.PHONY: build-tools +build-tools: + go build -o bin/verify-commit-message hack-kni/tools/verifycommit/verify-commit-message.go + cp hack-kni/tools/verifycommit/config.json bin/ diff --git a/hack-kni/tools/verifycommit/config.json b/hack-kni/tools/verifycommit/config.json new file mode 100644 index 0000000000..a55a8529a1 --- /dev/null +++ b/hack-kni/tools/verifycommit/config.json @@ -0,0 +1,4 @@ +{ + "originName": "origin", + "upstreamName": "upstream" +} \ No newline at end of file diff --git a/hack-kni/tools/verifycommit/test-config-missing-origin.json b/hack-kni/tools/verifycommit/test-config-missing-origin.json new file mode 100644 index 0000000000..6be3632383 --- /dev/null +++ b/hack-kni/tools/verifycommit/test-config-missing-origin.json @@ -0,0 +1,3 @@ +{ + "upstreamName": "upstream" +} \ No newline at end of file diff --git a/hack-kni/tools/verifycommit/verify-commit-message.go b/hack-kni/tools/verifycommit/verify-commit-message.go new file mode 100644 index 0000000000..7da973e48c --- /dev/null +++ b/hack-kni/tools/verifycommit/verify-commit-message.go @@ -0,0 +1,223 @@ +package main + +import ( + "bufio" + "encoding/json" + "errors" + "flag" + "fmt" + "log" + "os" + "os/exec" + "strings" +) + +const ( + exitCodeSuccess = 0 + exitCodeErrorWrongArguments = 1 + exitCodeErrorProcessingFile = 2 + exitCodeErrorVerificationFailed = 3 +) + +const ( + exitCodeGitMalformedObject = 129 +) + +const ( + tagKNI = "[KNI]" + tagUpstream = "[upstream]" + + cherryPickLinePrefix = "(cherry picked from commit " + cherryPickLineSuffix = ")" // yes that simple + + signedOffByPrefix = "Signed-off-by: " + + konfluxUsername = "red-hat-konflux" + + defaultOriginName = "origin" + defaultUpstreamName = "upstream" + referenceBranchName = "master" +) + +var ( + errEmptyCommitMessage = errors.New("empty commit message") + errMissingTagKNI = errors.New("missing tag: " + tagKNI) + errMissingCherryPickReference = errors.New("missing cherry pick reference") + errWrongCherryPickReference = errors.New("wrong cherry pick reference") +) + +type commitMessage struct { + lines []string +} + +type config struct { + // OriginName is the git remote that points to the clone of this repo + OriginName string `json:"originName"` + // UpstreamName is the git remote that points kubernetes-sigs/scheduler-plugins repo + UpstreamName string `json:"upstreamName"` +} + +var conf = config{ + OriginName: defaultOriginName, + UpstreamName: defaultUpstreamName, +} + +func newCommitMessageFromString(text string) commitMessage { + var cm commitMessage + scanner := bufio.NewScanner(strings.NewReader(text)) + for scanner.Scan() { + cm.lines = append(cm.lines, scanner.Text()) + } + log.Printf("commit message has %d lines", cm.numLines()) + return cm +} + +func (cm commitMessage) numLines() int { + return len(cm.lines) +} + +func (cm commitMessage) isEmpty() bool { + return cm.numLines() == 0 +} + +func (cm commitMessage) summary() string { + return cm.lines[0] +} + +func (cm commitMessage) isKNISpecific() bool { + return strings.Contains(cm.summary(), tagKNI) +} + +func (cm commitMessage) isUpstream() bool { + return strings.Contains(cm.summary(), tagUpstream) +} + +// cherryPickOrigin returns the commit hash this commit was cherry-picked +// from if this commit has cherry-pick reference; otherwise returns empty string. +func (cm commitMessage) cherryPickOrigin() string { + for idx := cm.numLines() - 1; idx > 0; idx-- { + line := cm.lines[idx] // shortcut + cmHash, ok := strings.CutPrefix(line, cherryPickLinePrefix) + if !ok { // we don't have the prefix, so we don't care + continue + } + cmHash, ok = strings.CutSuffix(cmHash, cherryPickLineSuffix) + if !ok { // we don't have the suffix, so we don't care + continue + } + return cmHash + } + return "" // nothing found +} + +func (cm commitMessage) isKonflux() bool { + for idx := cm.numLines() - 1; idx > 0; idx-- { + line := cm.lines[idx] // shortcut + signedOff, ok := strings.CutPrefix(line, signedOffByPrefix) + if !ok { + continue + } + if strings.HasPrefix(signedOff, konfluxUsername) { + return true + } + } + return false // nothing found +} +func verifyCommitMessage(commitMessage string) error { + cm := newCommitMessageFromString(commitMessage) + + if cm.isKonflux() { + return nil + } + return verifyHumanCommitMessage(cm) +} + +func verifyHumanCommitMessage(cm commitMessage) error { + if cm.isEmpty() { + return errEmptyCommitMessage + } + + if !cm.isKNISpecific() { + return errMissingTagKNI + } + + cpOrigin := cm.cherryPickOrigin() + upstream := cm.isUpstream() + + if cpOrigin == "" { + if upstream { + return errMissingCherryPickReference + } + } + + if cpOrigin != "" { + remoteName := conf.OriginName + if upstream { + remoteName = conf.UpstreamName + } + + err := isCommitInBranch(remoteName, cpOrigin) + if err != nil { + return err + } + } + + return nil +} + +func isCommitInBranch(remoteName, cpOrigin string) error { + cmd := exec.Command("git", "branch", "-r", "--contains", cpOrigin) + out, err := cmd.Output() + if err != nil { + if isMalformedObjectErr(err, cpOrigin) { + return errWrongCherryPickReference + } + return err + } + + outStr := string(out) + + if !strings.Contains(outStr, remoteName+"/"+referenceBranchName) { + return errWrongCherryPickReference + } + return nil +} + +func isMalformedObjectErr(err error, objHash string) bool { + if exitErr, ok := err.(*exec.ExitError); ok { + if string(exitErr.Stderr) == fmt.Sprintf("error: malformed object name %s\n", objHash) && exitErr.ExitCode() == exitCodeGitMalformedObject { + return true + } + } + return false +} + +func main() { + var configFileName = flag.String("f", "config.json", "config file path") + flag.Parse() + + if flag.NArg() != 1 { + programName := os.Args[0] + log.Printf("usage: %s [-f config-file] ", programName) + os.Exit(exitCodeErrorWrongArguments) + } + + data, err := os.ReadFile(*configFileName) + if err != nil { + fmt.Printf("error reading %s: %v", *configFileName, err) + os.Exit(exitCodeErrorProcessingFile) + } + err = json.Unmarshal(data, &conf) // keep it flexible + if err != nil { + log.Printf("error parsing %s: %v", *configFileName, err) + os.Exit(exitCodeErrorProcessingFile) + } + + err = verifyCommitMessage(flag.Arg(0)) + if err != nil { + log.Printf("verification failed: %v", err) + os.Exit(exitCodeErrorVerificationFailed) + } + + os.Exit(exitCodeSuccess) // all good! redundant but let's be explicit about our success +} diff --git a/hack-kni/tools/verifycommit/verify-commit-message_test.go b/hack-kni/tools/verifycommit/verify-commit-message_test.go new file mode 100644 index 0000000000..2a8b10e2dd --- /dev/null +++ b/hack-kni/tools/verifycommit/verify-commit-message_test.go @@ -0,0 +1,258 @@ +package main + +import ( + "bytes" + "fmt" + "os/exec" + "path/filepath" + goruntime "runtime" + "testing" +) + +const ( + binariesDir = "bin" + programName = "verify-commit-message" +) + +func TestRunCommand(t *testing.T) { + validCommit := `[KNI] doc about cascading **KNI SPECIFIC** cherry-picks + +We can avoid long chains of cherry-picked from commit +references JUST AND ONLY for KNI-specific changes. + +Signed-off-by: Francesco Romani ` + + testcases := []struct { + description string + args []string + expectedExitCode int + }{ + { + description: "pattern 1: program-name ", + args: []string{validCommit}, + expectedExitCode: exitCodeSuccess, + }, + { + description: "pattern 2: program-name -f ", + args: []string{"-f", "config.json", validCommit}, + expectedExitCode: exitCodeSuccess, + }, + { + description: "pattern 3: missing config data should default to default values", + args: []string{"-f", "test-config-missing-origin.json", validCommit}, + expectedExitCode: exitCodeSuccess, + }, + { + description: "invalid: too many non-flag arguments", + args: []string{validCommit, "extra-arg"}, + expectedExitCode: exitCodeErrorWrongArguments, + }, + { + description: "invalid: no commit message", + args: []string{"-f", "config.json"}, + expectedExitCode: exitCodeErrorWrongArguments, + }, + { + description: "invalid: no arguments at all", + args: []string{}, + expectedExitCode: exitCodeErrorWrongArguments, + }, + { + description: "invalid: empty lines commit message", + args: []string{` + + +`}, + expectedExitCode: exitCodeErrorVerificationFailed, + }, + { + description: "invalid: empty commit message", + args: []string{""}, + expectedExitCode: exitCodeErrorVerificationFailed, + }, + { + description: "invalid: not-found config file", + args: []string{"-f", "not-found.json", validCommit}, + expectedExitCode: exitCodeErrorProcessingFile, + }, + } + + for _, tc := range testcases { + t.Run(tc.description, func(t *testing.T) { + _, errBuf, err := runCommand(t, tc.args...) + + if tc.expectedExitCode == exitCodeSuccess && err == nil { + // everything is as expected + return + } + + if tc.expectedExitCode == exitCodeSuccess && err != nil { + t.Fatalf("Received unexpected error %v (stderr=%s)", err, errBuf) + } + + exiterr, ok := err.(*exec.ExitError) + if !ok { + t.Fatalf("Received unexpected error type %T: %v", err, err) + } + + errCode := exiterr.ExitCode() + if errCode != tc.expectedExitCode { + t.Fatalf("Received unexpected exit code: expected %d got %d", tc.expectedExitCode, errCode) + } + }) + } +} + +func TestVerifyCommitMessage(t *testing.T) { + testcases := []struct { + description string + commitMsg string + expectedErr error + }{ + { + description: "only KNI in local fork", + commitMsg: `[KNI] hack-kni: skip commit verification for konflux commits + + Skip validating konflux commits structure. Usually konflux bot commits + signed off by either "red-hat-konflux" or "red-hat-konflux[bot]". + + Signed-off-by: Shereen Haj `, + }, + { + description: "KNI & upstream tags", + commitMsg: `[KNI][upstream] nrt: test: ensure generation is updated correctly +Add a unit test mainly for GetCachedNRTCopy() to verify the returned +generation. To help with the verification, make FlushNodes not only +report about the new generation (which only updated there) but also +return it so we'd be able to compare the values. + +Signed-off-by: Shereen Haj +(cherry picked from commit ffe2ce2)`, + }, + { + // this test depends on the local setup. The expected setup should be that the forked project remote + // name is called "origin" and the main project from which this project is forked called "upstream" + description: "KNI and local cherrypick", + commitMsg: `[KNI][release-4.18] ci: ghactions: ensure golang version in vendor check +make sure we run the vendor check in a controlled environment, +and also make sure to emit the golang version we use. + +Signed-off-by: Francesco Romani +(cherry picked from commit 2f4974a)`, + }, + { + description: "Konflux signed - github email", + commitMsg: `Update Konflux references to 252e5c9 +Signed-off-by: red-hat-konflux <126015336+red-hat-konflux[bot]@users.noreply.github.com>`, + }, + { + description: "Konflux signed - ci email", + commitMsg: `Update Konflux references +Signed-off-by: red-hat-konflux `, + }, + { + description: "Negative - no KNI tag and not konflux signed", + commitMsg: `nrt: test: ensure generation is updated correctly +Add a unit test + + Signed-off-by: Shereen Haj `, + expectedErr: errMissingTagKNI, + }, + { + description: "Negative - no KNI tag but with upstream tag", + commitMsg: `[upstream] nrt: test: ensure generation is updated correctly +Add a unit test + +Signed-off-by: Shereen Haj +(cherry picked from commit ffe2ce2)`, + expectedErr: errMissingTagKNI, + }, + { + description: "Negative - KNI & upstream tags without cherrypick hash", + commitMsg: `[KNI][upstream] nrt: test: ensure generation is updated correctly +Add a unit test + +Signed-off-by: Shereen Haj `, + expectedErr: errMissingCherryPickReference, + }, + { + description: "Negative - Konflux generated without signature", + commitMsg: `Update Konflux references to 252e5c9 + + +`, + expectedErr: errMissingTagKNI, + }, + { + description: "Negative - invalid cherry-pick reference", + commitMsg: `[KNI][upstream] nrt: test: ensure generation is updated correctly +Add a unit test + +Signed-off-by: Shereen Haj +(cherry picked from commit 123a)`, + expectedErr: errWrongCherryPickReference, + }, + { + description: "Negative - empty cherry-pick reference", + commitMsg: `[KNI][upstream] nrt: test: ensure generation is updated correctly +Add a unit test + +Signed-off-by: Shereen Haj +(cherry picked from commit )`, + expectedErr: errMissingCherryPickReference, + }, + } + for _, tc := range testcases { + t.Run(tc.description, func(t *testing.T) { + got := verifyCommitMessage(tc.commitMsg) + + if got == nil && tc.expectedErr == nil { + return + } + + if tc.expectedErr != nil && got == nil { + t.Fatalf("expected error %v but recieved nil", tc.expectedErr) + } + + if tc.expectedErr == nil && got != nil { + t.Fatalf("unexpected error %v", got) + } + + if got.Error() != tc.expectedErr.Error() { + t.Fatalf("mismatching error strings: expected %s, got %s", tc.expectedErr.Error(), got.Error()) + } + }) + } +} + +// runCommand returns stdout as string, stderr as string, error value +func runCommand(t *testing.T, args ...string) (string, string, error) { + bin, err := getBinPath() + if err != nil { + t.Fatalf("failed to find the binary path: %v", err) + } + fmt.Printf("going to use %q\n", bin) + var errBuf bytes.Buffer + cmd := exec.Command(bin, args...) + cmd.Stderr = &errBuf + out, err := cmd.Output() + fmt.Printf("tool returned <%s>\n", out) + return string(out), errBuf.String(), err +} + +func getBinPath() (string, error) { + rootDir, err := getRootPath() + if err != nil { + return "", err + } + return filepath.Join(rootDir, binariesDir, programName), nil +} + +func getRootPath() (string, error) { + _, file, _, ok := goruntime.Caller(0) + if !ok { + return "", fmt.Errorf("cannot retrieve tests directory") + } + basedir := filepath.Dir(file) + return filepath.Abs(filepath.Join(basedir, "..", "..", "..")) +} diff --git a/hack-kni/verify-commits.sh b/hack-kni/verify-commits.sh index 035db4484d..7b3d3a32ff 100755 --- a/hack-kni/verify-commits.sh +++ b/hack-kni/verify-commits.sh @@ -29,15 +29,10 @@ echo "---" # list commits for commitish in $( git log --oneline --no-merges -n ${COMMITS} | cut -d' ' -f 1); do - echo "CHECK: $commitish" + echo "CHECK: $commitish" - author_name=$( git log --pretty=format:"%an" -n 1 "$commitish" ) - if [[ "$author_name" == red-hat-konflux* ]]; then - echo "Skip verifying commit from Konflux bot" - continue - fi - - .github/hooks/commit-msg $( git log --format=%s -n 1 "$commitish" ) + cd ../bin + ./verify-commit-message $( git log --format=%s -n 1 "$commitish" ) if [[ "$?" != "0" ]]; then echo "-> FAIL: $commitish" exit 20 From db16d96042d951055cd074f8d56363a96b2cee61 Mon Sep 17 00:00:00 2001 From: Shereen Haj Date: Fri, 26 Sep 2025 09:50:09 +0300 Subject: [PATCH 2/5] [KNI] ci: move verify-commit logic entirely to go tool Now that we have a tool that verifies a single commit message, enhance it even more to deal with N commits and do the rest of the verification logic. From a real example, it turns out that the Signed-off line is prefixed with spaces, thus also fix the isKonflux method to deal with this input. Assisted-by: Cursor v1.2.2 AI-Attribution: AIA HAb Hin R Cursor v1.2.2 v1.0 Signed-off-by: Shereen Haj --- .github/hooks/commit-msg | 13 -- .github/workflows/ci.yaml | 15 +- ...g-origin.json => test-config-missing.json} | 0 .../verifycommit/verify-commit-message.go | 151 +++++++++++++++--- .../verify-commit-message_test.go | 79 +++++---- hack-kni/verify-commits.sh | 42 ----- 6 files changed, 177 insertions(+), 123 deletions(-) delete mode 100755 .github/hooks/commit-msg rename hack-kni/tools/verifycommit/{test-config-missing-origin.json => test-config-missing.json} (100%) delete mode 100755 hack-kni/verify-commits.sh diff --git a/.github/hooks/commit-msg b/.github/hooks/commit-msg deleted file mode 100755 index 366dc57e94..0000000000 --- a/.github/hooks/commit-msg +++ /dev/null @@ -1,13 +0,0 @@ -#!/bin/bash - -function enforce_commit_msg() -{ - local cmtmsg="$1" - if [[ $cmtmsg != \[KNI\]* ]]; then - echo "commit message [$cmtmsg] not formatted correctly" - echo "please refer to https://github.com/openshift-kni/scheduler-plugins/blob/master/RESYNC.md#patching-openshift-kni-specific-commits" - exit 1 - fi -} - -enforce_commit_msg "$1" diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 776a3ec1c9..77da4658f1 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -15,6 +15,11 @@ jobs: with: fetch-depth: 0 + - name: Set up golang + uses: actions/setup-go@v3 + with: + go-version: 1.23 + - name: Get branch names (pull request) shell: bash run: | @@ -26,16 +31,18 @@ jobs: echo ${{ env.SOURCE_BRANCH_NAME }} echo ${{ env.TARGET_BRANCH_NAME }} + - name: Setup git remotes + run: + git remote add upstream https://github.com/kubernetes-sigs/scheduler-plugins.git + - name: Build verification tool run: | make -f Makefile.kni build-tools - name: Verify commits run: | - TRIGGER_BRANCH=${{ env.SOURCE_BRANCH_NAME }} \ - UPSTREAM_BRANCH=${{ env.TARGET_BRANCH_NAME }} \ - COMMITS=${{ github.event.pull_request.commits }} \ - ./hack-kni/verify-commits.sh + cd bin + ./verify-commit-message -n ${{ github.event.pull_request.commits }} integration-test: runs-on: ubuntu-latest diff --git a/hack-kni/tools/verifycommit/test-config-missing-origin.json b/hack-kni/tools/verifycommit/test-config-missing.json similarity index 100% rename from hack-kni/tools/verifycommit/test-config-missing-origin.json rename to hack-kni/tools/verifycommit/test-config-missing.json diff --git a/hack-kni/tools/verifycommit/verify-commit-message.go b/hack-kni/tools/verifycommit/verify-commit-message.go index 7da973e48c..0c3ee3d261 100644 --- a/hack-kni/tools/verifycommit/verify-commit-message.go +++ b/hack-kni/tools/verifycommit/verify-commit-message.go @@ -6,9 +6,11 @@ import ( "errors" "flag" "fmt" + "io" "log" "os" "os/exec" + "path/filepath" "strings" ) @@ -37,6 +39,10 @@ const ( defaultOriginName = "origin" defaultUpstreamName = "upstream" referenceBranchName = "master" + + resyncBranchPrefix = "resync-" + + headBranchName = "HEAD" ) var ( @@ -55,6 +61,8 @@ type config struct { OriginName string `json:"originName"` // UpstreamName is the git remote that points kubernetes-sigs/scheduler-plugins repo UpstreamName string `json:"upstreamName"` + // TriggerBranch is the branch name that triggers the verification + TriggerBranch string `json:"triggerBranch"` } var conf = config{ @@ -62,6 +70,8 @@ var conf = config{ UpstreamName: defaultUpstreamName, } +var sourceBranch *string + func newCommitMessageFromString(text string) commitMessage { var cm commitMessage scanner := bufio.NewScanner(strings.NewReader(text)) @@ -92,6 +102,25 @@ func (cm commitMessage) isUpstream() bool { return strings.Contains(cm.summary(), tagUpstream) } +func (cm commitMessage) isKonflux() bool { + for idx := cm.numLines() - 1; idx > 0; idx-- { + line := cm.lines[idx] // shortcut + line = strings.TrimSpace(line) + signedOff, ok := strings.CutPrefix(line, signedOffByPrefix) + if !ok { + continue + } + if strings.HasPrefix(signedOff, konfluxUsername) { + return true + } + } + return false // nothing found +} + +func isResyncBranch(branch string) bool { + return strings.HasPrefix(branch, resyncBranchPrefix) +} + // cherryPickOrigin returns the commit hash this commit was cherry-picked // from if this commit has cherry-pick reference; otherwise returns empty string. func (cm commitMessage) cherryPickOrigin() string { @@ -110,20 +139,7 @@ func (cm commitMessage) cherryPickOrigin() string { return "" // nothing found } -func (cm commitMessage) isKonflux() bool { - for idx := cm.numLines() - 1; idx > 0; idx-- { - line := cm.lines[idx] // shortcut - signedOff, ok := strings.CutPrefix(line, signedOffByPrefix) - if !ok { - continue - } - if strings.HasPrefix(signedOff, konfluxUsername) { - return true - } - } - return false // nothing found -} -func verifyCommitMessage(commitMessage string) error { +func validateCommitMessage(commitMessage string) error { cm := newCommitMessageFromString(commitMessage) if cm.isKonflux() { @@ -132,6 +148,49 @@ func verifyCommitMessage(commitMessage string) error { return verifyHumanCommitMessage(cm) } +func getCommitMessageByHash(commitHash string) (string, error) { + cmd := exec.Command("git", "show", "--format=%B", commitHash) + out, err := cmd.Output() + if err != nil { + return "", fmt.Errorf("failed to get commit message for %s: %v", commitHash, err) + } + return string(out), nil +} + +func verifyLastNCommits(numCommits int) error { + log.Printf("considering %d commits in PR whose head is %s:\n", numCommits, *sourceBranch) + + // Get the list of commits + cmd := exec.Command("git", "log", *sourceBranch, "--oneline", "--no-merges", "-n", fmt.Sprintf("%d", numCommits)) + out, err := cmd.Output() + if err != nil { + return fmt.Errorf("failed to get commit list: %v", err) + } + log.Println(string(out)) + + commitLines := strings.Split(strings.TrimSpace(string(out)), "\n") + for _, line := range commitLines { + log.Printf("examining %q", line) + + fields := strings.Fields(line) + if len(fields) == 0 { + continue + } + commitHash := fields[0] + + commit, err := getCommitMessageByHash(commitHash) + if err != nil { + return err + } + log.Printf("verifying message:\n%q\n", commit) + if err = validateCommitMessage(commit); err != nil { + return err + } + } + + return nil +} + func verifyHumanCommitMessage(cm commitMessage) error { if cm.isEmpty() { return errEmptyCommitMessage @@ -194,26 +253,41 @@ func isMalformedObjectErr(err error, objHash string) bool { func main() { var configFileName = flag.String("f", "config.json", "config file path") + var numCommits = flag.Int("n", 0, "number of last commits to verify (defaults to 0)") + sourceBranch = flag.String("b", "", "source branch name") flag.Parse() - if flag.NArg() != 1 { + if *numCommits < 0 { programName := os.Args[0] - log.Printf("usage: %s [-f config-file] ", programName) + log.Printf("usage: %s -b -n [-f config-file]", programName) + log.Printf(" -b: source branch name (if empty, the tool will use the current branch)") + log.Printf(" -n: number of last commits to verify (if not provided it defaults to 0)") + log.Printf(" -f: config file path (optional, remote defaults are origin and upstream)") os.Exit(exitCodeErrorWrongArguments) } - data, err := os.ReadFile(*configFileName) - if err != nil { - fmt.Printf("error reading %s: %v", *configFileName, err) - os.Exit(exitCodeErrorProcessingFile) + if *numCommits == 0 { + log.Printf("number of commits to verify is 0, skipping verification") + os.Exit(exitCodeSuccess) } - err = json.Unmarshal(data, &conf) // keep it flexible + + if strings.TrimSpace(*sourceBranch) == "" { + *sourceBranch = headBranchName + log.Printf("using branch: %s", *sourceBranch) + } + + if isResyncBranch(*sourceBranch) { + log.Printf("WARN: resync branch no commit enforcement will be triggered\n") + os.Exit(exitCodeSuccess) + } + + err := processConfigFile(*configFileName) if err != nil { - log.Printf("error parsing %s: %v", *configFileName, err) + log.Printf("error processing config file: %v", err) os.Exit(exitCodeErrorProcessingFile) } - err = verifyCommitMessage(flag.Arg(0)) + err = verifyLastNCommits(*numCommits) if err != nil { log.Printf("verification failed: %v", err) os.Exit(exitCodeErrorVerificationFailed) @@ -221,3 +295,34 @@ func main() { os.Exit(exitCodeSuccess) // all good! redundant but let's be explicit about our success } + +func processConfigFile(filePath string) error { + // Use os.OpenRoot to prevent directory traversal attacks + // This ensures file access is scoped to the validated absolute path + root, err := os.OpenRoot(filepath.Dir(filePath)) + if err != nil { + return fmt.Errorf("error opening root directory for %s: %v", filePath, err) + } + defer func() { + _ = root.Close() + }() + + file, err := root.Open(filepath.Base(filePath)) + if err != nil { + return fmt.Errorf("error opening file %s: %v", filePath, err) + } + defer func() { + _ = file.Close() + }() + + fileContent, err := io.ReadAll(file) + if err != nil { + return fmt.Errorf("error reading content from %s: %v", filePath, err) + } + + err = json.Unmarshal(fileContent, &conf) // keep it flexible + if err != nil { + return fmt.Errorf("error parsing %s: %v", filePath, err) + } + return nil +} diff --git a/hack-kni/tools/verifycommit/verify-commit-message_test.go b/hack-kni/tools/verifycommit/verify-commit-message_test.go index 2a8b10e2dd..b181874031 100644 --- a/hack-kni/tools/verifycommit/verify-commit-message_test.go +++ b/hack-kni/tools/verifycommit/verify-commit-message_test.go @@ -15,75 +15,67 @@ const ( ) func TestRunCommand(t *testing.T) { - validCommit := `[KNI] doc about cascading **KNI SPECIFIC** cherry-picks - -We can avoid long chains of cherry-picked from commit -references JUST AND ONLY for KNI-specific changes. - -Signed-off-by: Francesco Romani ` - testcases := []struct { description string args []string expectedExitCode int }{ { - description: "pattern 1: program-name ", - args: []string{validCommit}, + description: "valid call", + args: []string{"-n", "5", "-b", "release-4.19"}, expectedExitCode: exitCodeSuccess, }, { - description: "pattern 2: program-name -f ", - args: []string{"-f", "config.json", validCommit}, + description: "valid call with config file", + args: []string{"-f", "config.json", "-n", "5", "-b", "release-4.19"}, expectedExitCode: exitCodeSuccess, }, { - description: "pattern 3: missing config data should default to default values", - args: []string{"-f", "test-config-missing-origin.json", validCommit}, + description: "valid:missing config data should default to default values", + args: []string{"-f", "test-config-missing.json", "-n", "5", "-b", "resync-docs"}, expectedExitCode: exitCodeSuccess, }, { - description: "invalid: too many non-flag arguments", - args: []string{validCommit, "extra-arg"}, - expectedExitCode: exitCodeErrorWrongArguments, - }, - { - description: "invalid: no commit message", - args: []string{"-f", "config.json"}, - expectedExitCode: exitCodeErrorWrongArguments, + description: "valid: too many non-flag arguments should be ignored", + args: []string{"-n", "5", "-b", "release-4.19", "extra-arg"}, + expectedExitCode: exitCodeSuccess, }, { - description: "invalid: no arguments at all", - args: []string{}, - expectedExitCode: exitCodeErrorWrongArguments, + description: "valid: missing number of commits defaults to 0", + args: []string{"-b", "release-4.19"}, + expectedExitCode: exitCodeSuccess, }, { - description: "invalid: empty lines commit message", - args: []string{` - - -`}, - expectedExitCode: exitCodeErrorVerificationFailed, + description: "valid: missing source branch", + args: []string{"-n", "5"}, + expectedExitCode: exitCodeSuccess, }, { - description: "invalid: empty commit message", - args: []string{""}, - expectedExitCode: exitCodeErrorVerificationFailed, + description: "valid: no arguments at all", + args: []string{}, + expectedExitCode: exitCodeSuccess, }, { description: "invalid: not-found config file", - args: []string{"-f", "not-found.json", validCommit}, + args: []string{"-f", "not-found.json", "-n", "5", "-b", "release-4.17"}, expectedExitCode: exitCodeErrorProcessingFile, }, + { + description: "invalid: not-found branch name", + args: []string{"-n", "5", "-b", "branch-not-found"}, + expectedExitCode: exitCodeErrorVerificationFailed, + }, } for _, tc := range testcases { t.Run(tc.description, func(t *testing.T) { - _, errBuf, err := runCommand(t, tc.args...) + out, errBuf, err := runCommand(t, tc.args...) - if tc.expectedExitCode == exitCodeSuccess && err == nil { - // everything is as expected - return + if err == nil { + if tc.expectedExitCode == exitCodeSuccess { // everything is as expected + return + } + t.Fatalf("Received unexpected success %s", out) } if tc.expectedExitCode == exitCodeSuccess && err != nil { @@ -103,7 +95,7 @@ Signed-off-by: Francesco Romani ` } } -func TestVerifyCommitMessage(t *testing.T) { +func TestValidateCommitMessage(t *testing.T) { testcases := []struct { description string commitMsg string @@ -143,13 +135,18 @@ Signed-off-by: Francesco Romani { description: "Konflux signed - github email", commitMsg: `Update Konflux references to 252e5c9 -Signed-off-by: red-hat-konflux <126015336+red-hat-konflux[bot]@users.noreply.github.com>`, + Signed-off-by: red-hat-konflux <126015336+red-hat-konflux[bot]@users.noreply.github.com>`, }, { description: "Konflux signed - ci email", commitMsg: `Update Konflux references Signed-off-by: red-hat-konflux `, }, + { + description: "Konflux signed - github email", + commitMsg: `chore(deps): update konflux references to 2c32152 + Signed-off-by: red-hat-konflux <126015336+red-hat-konflux[bot]@users.noreply.github.com>`, + }, { description: "Negative - no KNI tag and not konflux signed", commitMsg: `nrt: test: ensure generation is updated correctly @@ -204,7 +201,7 @@ Signed-off-by: Shereen Haj } for _, tc := range testcases { t.Run(tc.description, func(t *testing.T) { - got := verifyCommitMessage(tc.commitMsg) + got := validateCommitMessage(tc.commitMsg) if got == nil && tc.expectedErr == nil { return diff --git a/hack-kni/verify-commits.sh b/hack-kni/verify-commits.sh deleted file mode 100755 index 7b3d3a32ff..0000000000 --- a/hack-kni/verify-commits.sh +++ /dev/null @@ -1,42 +0,0 @@ -#!/bin/bash - -set -e -o pipefail - -function finish { - if [ -f "$commit_msg_filename" ]; then - rm -f "$commit_msg_filename" - fi -} -trap finish EXIT - -echo "checking branch: [$TRIGGER_BRANCH]" - -shopt -s extglob -if [[ "$TRIGGER_BRANCH" == resync-* ]]; then - echo "WARN: resync branch no commit enforcement will be triggered" - exit 0 -fi - -if [ -z "$COMMITS" ] || (( $COMMITS <= 0 )); then - echo "WARN: no changes detected [COMMITS=${COMMITS}]" - exit 0 -fi - -echo "considering ${COMMITS} commits in PR whose head is $TRIGGER_BRANCH (into $UPSTREAM_BRANCH):" -echo "---" -git log --oneline --no-merges -n ${COMMITS} -echo "---" - -# list commits -for commitish in $( git log --oneline --no-merges -n ${COMMITS} | cut -d' ' -f 1); do - echo "CHECK: $commitish" - - cd ../bin - ./verify-commit-message $( git log --format=%s -n 1 "$commitish" ) - if [[ "$?" != "0" ]]; then - echo "-> FAIL: $commitish" - exit 20 - fi - echo "-> PASS: $commitish" -done - From 6510bbb026c315b17bb4ddd0b9b9eccf8055afe5 Mon Sep 17 00:00:00 2001 From: Shereen Haj Date: Thu, 23 Oct 2025 09:32:13 +0300 Subject: [PATCH 3/5] [KNI] ci: bump actions versions Update to the latest for checkout and go setup. https://github.com/actions/setup-go/tags https://github.com/actions/checkout/tags Signed-off-by: Shereen Haj --- .github/workflows/ci.yaml | 18 +++++++++--------- .github/workflows/release.yaml | 6 +++--- 2 files changed, 12 insertions(+), 12 deletions(-) diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 77da4658f1..8625fbf41f 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -11,14 +11,14 @@ jobs: runs-on: ubuntu-latest steps: - name: Checkout sources - uses: actions/checkout@v3 + uses: actions/checkout@v5 with: fetch-depth: 0 - name: Set up golang - uses: actions/setup-go@v3 + uses: actions/setup-go@v6 with: - go-version: 1.23 + go-version: 1.24 - name: Get branch names (pull request) shell: bash @@ -51,14 +51,14 @@ jobs: GOPATH: "/home/runner/go" steps: - name: Checkout sources - uses: actions/checkout@v3 + uses: actions/checkout@v5 with: fetch-depth: 0 - name: Set up golang - uses: actions/setup-go@v3 + uses: actions/setup-go@v6 with: - go-version: 1.23 + go-version: 1.24 - name: Run integration test run: @@ -68,14 +68,14 @@ jobs: runs-on: ubuntu-latest steps: - name: Checkout sources - uses: actions/checkout@v3 + uses: actions/checkout@v5 with: fetch-depth: 0 - name: Set up golang - uses: actions/setup-go@v3 + uses: actions/setup-go@v6 with: - go-version: 1.23 + go-version: 1.24 - name: Verify vendoring run: ./hack-kni/verify-vendoring.sh diff --git a/.github/workflows/release.yaml b/.github/workflows/release.yaml index 813d2a4857..a2a65ee484 100644 --- a/.github/workflows/release.yaml +++ b/.github/workflows/release.yaml @@ -18,15 +18,15 @@ jobs: runs-on: ubuntu-22.04 steps: - name: Checkout sources - uses: actions/checkout@v3 + uses: actions/checkout@v5 with: fetch-depth: 0 - name: Setup golang - uses: actions/setup-go@v3 + uses: actions/setup-go@v6 id: go with: - go-version: 1.23 + go-version: 1.24 - name: Set release version env var run: | From c8b6d21c368dfd30d86e0dd2507c9f9883715036 Mon Sep 17 00:00:00 2001 From: Shereen Haj Date: Thu, 23 Oct 2025 09:36:58 +0300 Subject: [PATCH 4/5] [KNI] ci: run commit verification on all product branches Isolate the commits verification and support running it on all product branches. Assisted-by: Cursor v1.2.2 AI-Attribution: AIA Primarily human, Content edits, Human-initiated, Reviewed, cursor v1.2.2 v1.0 Signed-off-by: Shereen Haj --- .github/workflows/ci.yaml | 37 ------------------------ .github/workflows/verifycommit.yaml | 45 +++++++++++++++++++++++++++++ 2 files changed, 45 insertions(+), 37 deletions(-) create mode 100644 .github/workflows/verifycommit.yaml diff --git a/.github/workflows/ci.yaml b/.github/workflows/ci.yaml index 8625fbf41f..9d8c34032c 100644 --- a/.github/workflows/ci.yaml +++ b/.github/workflows/ci.yaml @@ -7,43 +7,6 @@ on: branches: [ master, release-4.12, release-4.14, release-4.16, release-4.18 ] jobs: - commit-check: - runs-on: ubuntu-latest - steps: - - name: Checkout sources - uses: actions/checkout@v5 - with: - fetch-depth: 0 - - - name: Set up golang - uses: actions/setup-go@v6 - with: - go-version: 1.24 - - - name: Get branch names (pull request) - shell: bash - run: | - echo "SOURCE_BRANCH_NAME=$(echo ${GITHUB_HEAD_REF} | tr / -)" >> $GITHUB_ENV - echo "TARGET_BRANCH_NAME=$(echo ${GITHUB_BASE_REF} | tr / -)" >> $GITHUB_ENV - - - name: Debug - run: | - echo ${{ env.SOURCE_BRANCH_NAME }} - echo ${{ env.TARGET_BRANCH_NAME }} - - - name: Setup git remotes - run: - git remote add upstream https://github.com/kubernetes-sigs/scheduler-plugins.git - - - name: Build verification tool - run: | - make -f Makefile.kni build-tools - - - name: Verify commits - run: | - cd bin - ./verify-commit-message -n ${{ github.event.pull_request.commits }} - integration-test: runs-on: ubuntu-latest env: diff --git a/.github/workflows/verifycommit.yaml b/.github/workflows/verifycommit.yaml new file mode 100644 index 0000000000..39101ef518 --- /dev/null +++ b/.github/workflows/verifycommit.yaml @@ -0,0 +1,45 @@ +name: Verify Commits + +on: + push: + branches: [ "master", "release-4.*" ] + pull_request: + branches: [ "master", "release-4.*" ] + +jobs: + commit-check: + runs-on: ubuntu-latest + steps: + - name: Checkout sources + uses: actions/checkout@v5 + with: + fetch-depth: 0 + + - name: Set up golang + uses: actions/setup-go@v6 + with: + go-version: 1.24 + + - name: Get branch names (pull request) + shell: bash + run: | + echo "SOURCE_BRANCH_NAME=$(echo ${GITHUB_HEAD_REF} | tr / -)" >> $GITHUB_ENV + echo "TARGET_BRANCH_NAME=$(echo ${GITHUB_BASE_REF} | tr / -)" >> $GITHUB_ENV + + - name: Debug + run: | + echo ${{ env.SOURCE_BRANCH_NAME }} + echo ${{ env.TARGET_BRANCH_NAME }} + + - name: Setup git remote + run: + git remote add upstream https://github.com/kubernetes-sigs/scheduler-plugins.git + + - name: Build verification tool + run: | + make -f Makefile.kni build-tools + + - name: Verify commits + run: | + cd bin + ./verify-commit-message -n ${{ github.event.pull_request.commits }} From 7dd4bc3c454275d836015806a61afb5c06406c24 Mon Sep 17 00:00:00 2001 From: Shereen Haj Date: Thu, 23 Oct 2025 11:52:06 +0300 Subject: [PATCH 5/5] [KNI] ci: run verify-commit tool tests To ensure nothing gets merged broken. Signed-off-by: Shereen Haj --- .github/workflows/verifycommit.yaml | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/.github/workflows/verifycommit.yaml b/.github/workflows/verifycommit.yaml index 39101ef518..fde46a1fbe 100644 --- a/.github/workflows/verifycommit.yaml +++ b/.github/workflows/verifycommit.yaml @@ -43,3 +43,28 @@ jobs: run: | cd bin ./verify-commit-message -n ${{ github.event.pull_request.commits }} + + unit-testing: + runs-on: ubuntu-latest + steps: + - name: Checkout sources + uses: actions/checkout@v5 + with: + fetch-depth: 0 + + - name: Set up golang + uses: actions/setup-go@v6 + with: + go-version: 1.24 + + - name: Add upstream remote + run: | + git remote add upstream https://github.com/kubernetes-sigs/scheduler-plugins.git + + - name: Build verification tool + run: | + make -f Makefile.kni build-tools + + - name: Run unit tests + run: + go test -v ./hack-kni/tools/verifycommit