From 6601b2990cb2bb184d29b717913247ea7ab72112 Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Wed, 24 Dec 2025 11:38:09 +0000 Subject: [PATCH 1/2] Fix command and shell consider fixed sceanrios --- .claude/commands/api-lint-diff.md | 36 +++++++--- hack/api-lint-diff/run.sh | 114 +++++++++++++++++++++++++++--- 2 files changed, 130 insertions(+), 20 deletions(-) diff --git a/.claude/commands/api-lint-diff.md b/.claude/commands/api-lint-diff.md index f9d102d1e..549809bc4 100644 --- a/.claude/commands/api-lint-diff.md +++ b/.claude/commands/api-lint-diff.md @@ -4,7 +4,7 @@ description: Validate API issues using kube-api-linter with diff-aware analysis # API Lint Diff -Validates API issues in `api/` directory using kube-api-linter with diff-aware analysis that distinguishes between NEW and PRE-EXISTING issues. +Validates API issues in `api/` directory using kube-api-linter with diff-aware analysis that distinguishes between FIXED, NEW, and PRE-EXISTING issues. ## Instructions for Claude AI @@ -19,6 +19,7 @@ When this command is invoked, you MUST: 2. **Understand the shell script's output**: - **False positives (IGNORED)**: Standard CRD scaffolding patterns that kube-api-linter incorrectly flags + - **FIXED issues (SUCCESS)**: Issues that existed in baseline but were resolved in current branch → Celebrate! 🎉 - **NEW issues (ERRORS)**: Introduced in current branch → MUST fix - **PRE-EXISTING issues (WARNINGS)**: Existed before changes → Can fix separately @@ -132,23 +133,38 @@ When this command is invoked, you MUST: # API Lint Diff Analysis Report **Generated:** [date] - **Baseline:** main branch - **Current:** [branch name] + **Baseline:** main branch (X issues) + **Current:** [branch name] (Y issues) **Status:** [status icon and message based on logic below] **Status Logic:** - - ✅ PASSED: 0 real issues (after filtering false positives) + - ✅ PASSED: 0 new issues (fixed issues are OK) - ⚠️ WARN: 0 new issues but has pre-existing issues - ❌ FAIL: Has new issues that must be fixed ## Executive Summary - - Total issues: X - - False positives (IGNORED): Y - - Real issues (NEED FIXING): Z - - NEW issues: N - - PRE-EXISTING issues: P + - Baseline issues: X + - Current issues: Y + - **FIXED**: F (issues resolved in this branch) + - **NEW**: N (issues introduced in this branch) + - **PRE-EXISTING**: P (issues that still remain) + - False positives (IGNORED): Z - ## REAL ISSUES - FIXES NEEDED (Z issues) + ## FIXED ISSUES (F issues) + + [List of issues that were fixed in this branch - show the baseline line numbers] + + ## NEW ISSUES (N issues) + + [List of issues introduced in this branch - these MUST be fixed] + + ## PRE-EXISTING ISSUES (P issues) + + [List of issues that existed before and still exist - can be fixed separately] + + --- + + ## DETAILED ANALYSIS FOR ISSUES NEEDING FIXES ### Category 1: [Issue Type] (N issues) - [BREAKING/NON-BREAKING] diff --git a/hack/api-lint-diff/run.sh b/hack/api-lint-diff/run.sh index ca067d618..993ec9052 100755 --- a/hack/api-lint-diff/run.sh +++ b/hack/api-lint-diff/run.sh @@ -257,13 +257,14 @@ get_changed_files() { grep -v 'zz_generated' || true } -# Categorize issues as NEW or PRE-EXISTING +# Categorize issues as NEW, PRE-EXISTING, or FIXED categorize_issues() { local current_file="$1" local baseline_file="$2" local changed_files_file="$3" local new_issues_file="$4" local preexisting_issues_file="$5" + local fixed_issues_file="$6" # Read changed files into array local changed_files=() @@ -306,17 +307,81 @@ categorize_issues() { # Compare without line numbers since line numbers can change when code is added/removed # Format is: file:line:col:linter:message # We'll compare: file:linter:message - # Use f1,4,5- to capture field 5 and all remaining fields (handles colons in messages) + # Extract file (field 1), linter (field 4), and message (field 5+) from current issue local file_linter_msg file_linter_msg=$(echo "${line}" | cut -d: -f1,4,5-) - if grep -Fq "${file_linter_msg}" "${baseline_file}" 2>/dev/null; then + # Check if baseline has a matching issue (same file, linter, message but possibly different line number) + # We need to extract the same fields from baseline and compare + local found=false + if [[ -f "${baseline_file}" ]]; then + while IFS= read -r baseline_line; do + [[ -z "${baseline_line}" ]] && continue + local baseline_file_linter_msg + baseline_file_linter_msg=$(echo "${baseline_line}" | cut -d: -f1,4,5-) + if [[ "${file_linter_msg}" == "${baseline_file_linter_msg}" ]]; then + found=true + break + fi + done < "${baseline_file}" + fi + + if $found; then echo "${line}" >> "${preexisting_issues_file}" else echo "${line}" >> "${new_issues_file}" fi done < "${current_file}" fi + + # Find FIXED issues - issues in baseline that are NOT in current + if [[ -f "${baseline_file}" && -s "${baseline_file}" ]]; then + while IFS= read -r baseline_line; do + [[ -z "${baseline_line}" ]] && continue + + local file + file=$(echo "${baseline_line}" | cut -d: -f1) + + # Only check files that were changed + if [[ ${#changed_files[@]} -gt 0 ]]; then + local file_changed=false + for changed_file in "${changed_files[@]}"; do + if [[ "${file}" == "${changed_file}" ]]; then + file_changed=true + break + fi + done + + # Skip if file wasn't changed + if ! $file_changed; then + continue + fi + fi + + # Extract file:linter:message from baseline + local baseline_file_linter_msg + baseline_file_linter_msg=$(echo "${baseline_line}" | cut -d: -f1,4,5-) + + # Check if this issue still exists in current + local still_exists=false + if [[ -f "${current_file}" ]]; then + while IFS= read -r current_line; do + [[ -z "${current_line}" ]] && continue + local current_file_linter_msg + current_file_linter_msg=$(echo "${current_line}" | cut -d: -f1,4,5-) + if [[ "${baseline_file_linter_msg}" == "${current_file_linter_msg}" ]]; then + still_exists=true + break + fi + done < "${current_file}" + fi + + # If issue doesn't exist in current, it was fixed + if ! $still_exists; then + echo "${baseline_line}" >> "${fixed_issues_file}" + fi + done < "${baseline_file}" + fi } # Output issue (basic format) @@ -328,20 +393,41 @@ output_issue() { generate_report() { local new_issues_file="$1" local preexisting_issues_file="$2" + local fixed_issues_file="$3" + local baseline_file="$4" local new_count=0 local preexisting_count=0 + local fixed_count=0 + local baseline_count=0 [[ -f "${new_issues_file}" ]] && new_count=$(wc -l < "${new_issues_file}" | tr -d ' ') [[ -f "${preexisting_issues_file}" ]] && preexisting_count=$(wc -l < "${preexisting_issues_file}" | tr -d ' ') + [[ -f "${fixed_issues_file}" ]] && fixed_count=$(wc -l < "${fixed_issues_file}" | tr -d ' ') + [[ -f "${baseline_file}" ]] && baseline_count=$(wc -l < "${baseline_file}" | tr -d ' ') + + local current_total=$((new_count + preexisting_count)) - # Simple summary + # Summary header echo "API Lint Diff Results" - echo "Baseline: ${BASELINE_BRANCH}" + echo "=====================" + echo "Baseline (${BASELINE_BRANCH}): ${baseline_count} issues" + echo "Current branch: ${current_total} issues" + echo "" + echo "FIXED: ${fixed_count}" echo "NEW: ${new_count}" echo "PRE-EXISTING: ${preexisting_count}" echo "" + # Show FIXED issues + if [[ ${fixed_count} -gt 0 ]]; then + echo "=== FIXED ISSUES ===" + while IFS= read -r line; do + output_issue "${line}" + done < "${fixed_issues_file}" + echo "" + fi + # Show NEW issues if [[ ${new_count} -gt 0 ]]; then echo "=== NEW ISSUES ===" @@ -362,13 +448,17 @@ generate_report() { # Exit based on NEW issues count if [[ ${new_count} -eq 0 ]]; then - echo -e "${GREEN}NO NEW ISSUES found. Lint check passed.${NC}" + if [[ ${fixed_count} -gt 0 ]]; then + echo -e "${GREEN}SUCCESS: Fixed ${fixed_count} issue(s), no new issues introduced.${NC}" + else + echo -e "${GREEN}NO NEW ISSUES found. Lint check passed.${NC}" + fi if [[ ${preexisting_count} -gt 0 ]]; then - echo -e "${YELLOW}WARNING: Pre-existing issues detected. Please address them separately.${NC}" + echo -e "${YELLOW}WARNING: ${preexisting_count} pre-existing issue(s) remain. Please address them separately.${NC}" fi return 0 else - echo -e "${RED}FAILED: ${new_count} new issue(s)${NC}" + echo -e "${RED}FAILED: ${new_count} new issue(s) introduced${NC}" return 1 fi } @@ -414,18 +504,22 @@ main() { # Categorize issues touch "${TEMP_DIR}/new_issues.txt" touch "${TEMP_DIR}/preexisting_issues.txt" + touch "${TEMP_DIR}/fixed_issues.txt" categorize_issues \ "${TEMP_DIR}/current_parsed.txt" \ "${TEMP_DIR}/baseline_parsed.txt" \ "${TEMP_DIR}/changed_files.txt" \ "${TEMP_DIR}/new_issues.txt" \ - "${TEMP_DIR}/preexisting_issues.txt" + "${TEMP_DIR}/preexisting_issues.txt" \ + "${TEMP_DIR}/fixed_issues.txt" # Generate report generate_report \ "${TEMP_DIR}/new_issues.txt" \ - "${TEMP_DIR}/preexisting_issues.txt" + "${TEMP_DIR}/preexisting_issues.txt" \ + "${TEMP_DIR}/fixed_issues.txt" \ + "${TEMP_DIR}/baseline_parsed.txt" return $? } From 4dbd085f49aa673638cf3f7e2d192c25dbe05095 Mon Sep 17 00:00:00 2001 From: Camila Macedo <7708031+camilamacedo86@users.noreply.github.com> Date: Wed, 24 Dec 2025 11:58:01 +0000 Subject: [PATCH 2/2] Add a Makefile target and start running the API diff linter as part of CI. --- .github/workflows/sanity.yaml | 4 +++ Makefile | 8 ++++-- hack/api-lint-diff/run.sh | 49 ++++++++++++++++++++++++++++++++++- 3 files changed, 58 insertions(+), 3 deletions(-) diff --git a/.github/workflows/sanity.yaml b/.github/workflows/sanity.yaml index 55487ab1e..e3f6cb4dc 100644 --- a/.github/workflows/sanity.yaml +++ b/.github/workflows/sanity.yaml @@ -13,6 +13,8 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v6 + with: + fetch-depth: 0 - uses: actions/setup-go@v6 with: @@ -23,6 +25,8 @@ jobs: runs-on: ubuntu-latest steps: - uses: actions/checkout@v6 + with: + fetch-depth: 0 # Fetch all history for all branches (needed for API diff) - uses: actions/setup-go@v6 with: diff --git a/Makefile b/Makefile index f1d808e4b..4adedac24 100644 --- a/Makefile +++ b/Makefile @@ -118,7 +118,7 @@ help-extended: #HELP Display extended help. #SECTION Development .PHONY: lint -lint: lint-custom $(GOLANGCI_LINT) #HELP Run golangci linter. +lint: lint-custom lint-api-diff $(GOLANGCI_LINT) #HELP Run golangci linter. $(GOLANGCI_LINT) run --build-tags $(GO_BUILD_TAGS) $(GOLANGCI_LINT_ARGS) .PHONY: lint-helm @@ -149,6 +149,10 @@ custom-linter-build: #EXHELP Build custom linter lint-custom: custom-linter-build #EXHELP Call custom linter for the project go vet -tags=$(GO_BUILD_TAGS) -vettool=./bin/custom-linter ./... +.PHONY: lint-api-diff +lint-api-diff: $(GOLANGCI_LINT) #HELP Validate API changes using kube-api-linter with diff-aware analysis + bash hack/api-lint-diff/run.sh + .PHONY: k8s-pin k8s-pin: #EXHELP Pin k8s staging modules based on k8s.io/kubernetes version (in go.mod or from K8S_IO_K8S_VERSION env var) and run go mod tidy. K8S_IO_K8S_VERSION='$(K8S_IO_K8S_VERSION)' go run hack/tools/k8smaintainer/main.go @@ -198,7 +202,7 @@ generate: $(CONTROLLER_GEN) #EXHELP Generate code containing DeepCopy, DeepCopyI $(CONTROLLER_GEN) --load-build-tags=$(GO_BUILD_TAGS) object:headerFile="hack/boilerplate.go.txt" paths="./..." .PHONY: verify -verify: k8s-pin kind-verify-versions fmt generate manifests update-tls-profiles crd-ref-docs verify-bingo #HELP Verify all generated code is up-to-date. Runs k8s-pin instead of just tidy. +verify: k8s-pin kind-verify-versions fmt generate manifests update-tls-profiles crd-ref-docs verify-bingo lint-api-diff #HELP Verify all generated code is up-to-date. Runs k8s-pin instead of just tidy. git diff --exit-code .PHONY: verify-bingo diff --git a/hack/api-lint-diff/run.sh b/hack/api-lint-diff/run.sh index 993ec9052..a5e08eb9b 100755 --- a/hack/api-lint-diff/run.sh +++ b/hack/api-lint-diff/run.sh @@ -196,7 +196,36 @@ check_linter_support() { # Find golangci-lint binary find_golangci_lint() { - # Check for custom build first + # Check if Variables.mk exists and extract golangci-lint path + if [[ -f ".bingo/Variables.mk" ]]; then + # Extract version from GOLANGCI_LINT variable + # Format: GOLANGCI_LINT := $(GOBIN)/golangci-lint-v2.7.2 + local version + version=$(grep '^GOLANGCI_LINT' .bingo/Variables.mk | sed -E 's/.*golangci-lint-(v[0-9]+\.[0-9]+\.[0-9]+).*/\1/') + + if [[ -n "${version}" ]]; then + # Use go env to get the actual GOBIN/GOPATH + local gobin + gobin=$(go env GOBIN) + + # If GOBIN is empty, use GOPATH/bin + if [[ -z "${gobin}" ]]; then + local gopath + gopath=$(go env GOPATH) + # Take first entry if GOPATH has multiple paths (colon-separated) + gobin="${gopath%%:*}/bin" + fi + + # Check if the versioned binary exists + local bingo_path="${gobin}/golangci-lint-${version}" + if [[ -f "${bingo_path}" ]]; then + echo "${bingo_path}" + return 0 + fi + fi + fi + + # Check for custom build if [[ -f ".bingo/golangci-lint" ]]; then echo ".bingo/golangci-lint" return 0 @@ -216,6 +245,7 @@ find_golangci_lint() { echo -e "${RED}Error: golangci-lint not found.${NC}" >&2 echo -e "${RED}Searched for:${NC}" >&2 + echo -e " - .bingo/Variables.mk (bingo-managed versioned binary)" >&2 echo -e " - .bingo/golangci-lint" >&2 echo -e " - bin/golangci-lint" >&2 echo -e " - golangci-lint on your \$PATH" >&2 @@ -482,6 +512,23 @@ main() { # Create temporary config create_temp_config + # Ensure baseline branch is available (important for CI environments like GitHub Actions) + if ! git rev-parse --verify "${BASELINE_BRANCH}" &> /dev/null; then + echo -e "${YELLOW}Baseline branch '${BASELINE_BRANCH}' not found locally. Fetching from origin...${NC}" >&2 + + # Fetch the baseline branch from origin + if ! git fetch origin "${BASELINE_BRANCH}:${BASELINE_BRANCH}" 2>&1; then + # If direct fetch fails, try fetching with remote tracking + if ! git fetch origin "${BASELINE_BRANCH}" 2>&1; then + echo -e "${RED}Error: Failed to fetch baseline branch '${BASELINE_BRANCH}' from origin${NC}" >&2 + echo -e "${RED}Please ensure the branch exists in the remote repository.${NC}" >&2 + exit 1 + fi + # Use the remote tracking branch + BASELINE_BRANCH="origin/${BASELINE_BRANCH}" + fi + fi + # Get changed files get_changed_files > "${TEMP_DIR}/changed_files.txt"