diff --git a/.claude/commands/api-lint-diff.md b/.claude/commands/api-lint-diff.md new file mode 100644 index 000000000..5ba1c457a --- /dev/null +++ b/.claude/commands/api-lint-diff.md @@ -0,0 +1,184 @@ +--- +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. + +## Instructions for Claude AI + +When this command is invoked, you MUST: + +**CRITICAL:** The final output MUST be a comprehensive analysis report displayed directly in the conversation for the user to read. Do NOT just create temp files - output the full report as your response. + +1. **Execute the shell script** + ```bash + bash hack/api-lint-diff/run.sh + ``` + +2. **Understand the shell script's output**: + - **False positives (IGNORED)**: Standard CRD scaffolding patterns that kube-api-linter incorrectly flags + - **NEW issues (ERRORS)**: Introduced in current branch → MUST fix + - **PRE-EXISTING issues (WARNINGS)**: Existed before changes → Can fix separately + +3. **Filter false positives** - Operator projects scaffold standard Kubernetes CRD patterns that kube-api-linter incorrectly flags as errors, even with WhenRequired configuration. + + **Scenario 1: optionalfields on Status field** + ```go + Status MyResourceStatus `json:"status,omitzero"` + ``` + **Error reported:** + ``` + optionalfields: field Status has a valid zero value ({}), but the validation + is not complete (e.g. min properties/adding required fields). The field should + be a pointer to allow the zero value to be set. If the zero value is not a + valid use case, complete the validation and remove the pointer. + ``` + **Why it's a FALSE POSITIVE:** + - Status is NEVER a pointer in any Kubernetes API + - Works correctly with `omitzero` tag + - Validation incompleteness is expected - Status is controller-managed, not user-provided + - **ACTION: IGNORE this error** + + **Scenario 2: requiredfields on Spec field** + ```go + Spec MyResourceSpec `json:"spec"` + ``` + **Error reported:** + ``` + requiredfields: field Spec has a valid zero value ({}), but the validation is + not complete (e.g. min properties/adding required fields). The field should be + a pointer to allow the zero value to be set. If the zero value is not a valid + use case, complete the validation and remove the pointer. + ``` + **Why it's a FALSE POSITIVE:** + - Spec is NEVER a pointer in Kubernetes APIs + - Scaffolds are starting points - users add validation when they implement their business logic + - **ACTION: IGNORE this error** + + **Scenario 3: conditions markers on metav1.Condition** + ```go + Conditions []metav1.Condition `json:"conditions,omitempty"` + ``` + **Error reported:** + ``` + conditions: Conditions field is missing the following markers: + patchStrategy=merge, patchMergeKey=type + ``` + **Why it's a FALSE POSITIVE:** + - `metav1.Condition` already handles patches correctly + - Adding these markers is redundant for this standard Kubernetes type + - **ACTION: IGNORE this error** + +4. **For reported issues, provide intelligent analysis**: + + a. **Categorize by fix complexity**: + - NON-BREAKING: Marker replacements, adding listType, adding +required/+optional + - BREAKING: Pointer conversions, type changes (int→int32) + + b. **Search for actual usage** (REQUIRED FOR ALL ISSUES - NOT OPTIONAL): + - **CRITICAL:** Do NOT just look at JSON tags - analyze actual code usage patterns + - **Exception:** Deprecated marker replacements (`+kubebuilder:validation:Required` → `+required`) are straightforward - no usage analysis needed + - **For all other issues:** MUST analyze actual usage before making recommendations + - Use Grep to find ALL occurrences where each field is: + * **Read/accessed**: `obj.Spec.FieldName`, `cat.Spec.Priority` + * **Written/set**: `obj.Spec.FieldName = value` + * **Checked for zero/nil**: `if obj.Spec.FieldName == ""`, `if ptr != nil` + * **Used in conditionals**: Understand semantic meaning of zero values + - Search in: controllers, reconcilers, internal packages, tests, examples + - **Smart assessment based on usage patterns**: + * Field ALWAYS set in code? → Should be **required**, no omitempty + * Field SOMETIMES set? → Should be **optional** with omitempty + * Code checks `if field == zero`? → May need **pointer** to distinguish zero vs unset + * Zero value semantically valid? → Keep as value type with omitempty + * Zero value semantically invalid? → Use pointer OR mark required + * Field never read but only set by controller? → Likely Status field + - **Example analysis workflow for a field**: + ``` + 1. Grep for field usage: `CatalogFilter.Version` + 2. Found 5 occurrences: + - controllers/extension.go:123: if filter.Version != "" { ... } + - controllers/extension.go:456: result.Version = bundle.Version + - tests/filter_test.go:89: Version: "1.2.3" + 3. Analysis: Version is checked for empty, sometimes set, sometimes omitted + 4. Recommendation: Optional with omitempty (current usage supports this) + ``` + + c. **Generate EXACT code fixes** grouped by file: + - Show current code + - Show replacement code (copy-pasteable) + - **Explain why based on actual usage analysis** (not just JSON tags): + * Include usage summary: "Found N occurrences" + * Cite specific examples: "Used in resolve/catalog.go:163 as direct int32" + * Explain semantic meaning: "Field distinguishes priority 0 vs unset" + * Justify recommendation: "Since code checks for empty, should be optional" + - Note breaking change impact with reasoning + - **Each fix MUST include evidence from code usage** + + d. **Prioritize recommendations**: + - NEW issues first (must fix) + - Group PRE-EXISTING by NON-BREAKING vs BREAKING + +5. **Present actionable report directly to user**: + - **IMPORTANT:** Output the full comprehensive analysis in the conversation (not just to a temp file) + - Summary: False positives filtered, NEW count, PRE-EXISTING count + - Group issues by file and fix type + - Provide code snippets ready to apply (current code → fixed code) + - **DO NOT include "Next Steps" or "Conclusion" sections** - just present the analysis + + **Report Structure:** + ``` + # API Lint Diff Analysis Report + + **Generated:** [date] + **Baseline:** main branch + **Current:** [branch name] + **Status:** [status icon and message based on logic below] + + **Status Logic:** + - ✅ PASSED: 0 real issues (after filtering false positives) + - ⚠️ 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 + + ## REAL ISSUES - FIXES NEEDED (Z issues) + + ### Category 1: [Issue Type] (N issues) - [BREAKING/NON-BREAKING] + + #### File: [filename] + + **[Issue #]. Line X - [Field Name]** + ```go + // CURRENT: + [current code] + + // FIX: + [fixed code] + ``` + **Usage Analysis:** + - Found N occurrences in codebase + - [Specific usage example 1]: path/file.go:123 + - [Specific usage example 2]: path/file.go:456 + - Pattern: [always set / sometimes set / checked for zero / etc.] + + **Why:** [Recommendation based on usage analysis with evidence] + **Breaking:** [YES/NO] ([detailed reason with impact]) + + [Repeat for all issues] + + ## Summary of Breaking Changes + [Table of breaking changes if any] + ``` + +## Related Documentation + +- [Kubernetes API Conventions](https://github.com/kubernetes/community/blob/master/contributors/devel/sig-architecture/api-conventions.md) +- [kube-api-linter](https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/code-generator/cmd/kube-api-linter) +- AGENTS.md in this repository for understanding operator patterns diff --git a/hack/api-lint-diff/run.sh b/hack/api-lint-diff/run.sh new file mode 100755 index 000000000..a34775f32 --- /dev/null +++ b/hack/api-lint-diff/run.sh @@ -0,0 +1,415 @@ +#!/usr/bin/env bash + +# Copyright 2024 The Operator-SDK Authors +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +set -euo pipefail + +# Colors for output +RED='\033[0;31m' +YELLOW='\033[1;33m' +GREEN='\033[0;32m' +BLUE='\033[0;34m' +NC='\033[0m' # No Color + +# Temporary directory for this run +TEMP_DIR="" +BASELINE_BRANCH="${BASELINE_BRANCH:-main}" +API_DIR="api" +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" + +# Cleanup function +cleanup() { + if [[ -n "${TEMP_DIR}" && -d "${TEMP_DIR}" ]]; then + rm -rf "${TEMP_DIR}" + fi +} + +trap cleanup EXIT + +# Ensure we're in the repository root +if [[ ! -d ".git" ]]; then + echo -e "${RED}Error: Must be run from repository root${NC}" + exit 1 +fi + +if [[ ! -d "${API_DIR}" ]]; then + echo -e "${RED}Error: ${API_DIR}/ directory not found${NC}" + exit 1 +fi + +# Create temporary directory +TEMP_DIR=$(mktemp -d) +echo -e "${BLUE}Using temporary directory: ${TEMP_DIR}${NC}" >&2 + +# Get golangci-lint version from bingo +get_golangci_version() { + # Extract version from .bingo/Variables.mk + # Format: GOLANGCI_LINT := $(GOBIN)/golangci-lint-v2.7.2 + local version + version=$(grep 'GOLANGCI_LINT :=' .bingo/Variables.mk 2>/dev/null | sed -E 's/.*golangci-lint-(v[0-9.]+).*/\1/') + + # Validate that we got a version + if [[ -z "${version}" ]]; then + echo -e "${YELLOW}Warning: Could not extract golangci-lint version from .bingo/Variables.mk${NC}" >&2 + echo -e "${YELLOW}Using default version: latest${NC}" >&2 + version="latest" + fi + + echo "${version}" +} + +# Create temporary golangci-lint config for kube-api-linter +# This config focuses only on kube-api-linter for the api/ directory +create_temp_config() { + cat > "${TEMP_DIR}/.golangci.yaml" <<'EOF' +version: "2" +output: + formats: + tab: + path: stdout + colors: false +linters: + enable: + - kubeapilinter + settings: + custom: + kubeapilinter: + type: module + description: "Kube API Linter plugin" + original-url: "sigs.k8s.io/kube-api-linter" + settings: + linters: {} + lintersConfig: + optionalfields: + pointers: + preference: WhenRequired +run: + timeout: 5m + +issues: + exclude-rules: + # Ignore generated files + - path: zz_generated\..*\.go + linters: + - kubeapilinter + max-issues-per-linter: 0 + max-same-issues: 0 +EOF +} + +# Create custom golangci-lint configuration +create_custom_gcl_config() { + # Get golangci-lint version from bingo + local golangci_version + golangci_version=$(get_golangci_version) + + # Create custom-gcl config + cat > "${TEMP_DIR}/.custom-gcl.yml" <&2 + + # Create custom config + create_custom_gcl_config + + # Build custom golangci-lint using the 'custom' command + # This requires the base golangci-lint binary and runs from TEMP_DIR + # where .custom-gcl.yml is located + echo -e "${BLUE}Running golangci-lint custom build...${NC}" >&2 + local build_output + local abs_base_linter + # Convert to absolute path + if [[ "${base_linter}" != /* ]]; then + abs_base_linter="$(pwd)/${base_linter}" + else + abs_base_linter="${base_linter}" + fi + + if ! build_output=$(cd "${TEMP_DIR}" && "${abs_base_linter}" custom -v 2>&1); then + echo -e "${YELLOW}Warning: Failed to build custom golangci-lint${NC}" >&2 + echo -e "${YELLOW}Build output:${NC}" >&2 + echo "${build_output}" >&2 + echo -e "${YELLOW}Falling back to base linter (kube-api-linter will not be available)${NC}" >&2 + return 1 + fi + echo -e "${BLUE}Custom linter build completed${NC}" >&2 + + if [[ -f "${custom_binary}" ]]; then + echo -e "${GREEN}Successfully built custom golangci-lint at ${custom_binary}${NC}" >&2 + # Only echo the binary path to stdout for capture + echo "${custom_binary}" + return 0 + else + echo -e "${YELLOW}Warning: Custom binary not found at expected location${NC}" >&2 + return 1 + fi +} + +# Function to check if golangci-lint has kube-api-linter +check_linter_support() { + local linter_path="$1" + if ! "${linter_path}" linters 2>/dev/null | grep -q "kubeapilinter"; then + echo -e "${YELLOW}Warning: golangci-lint at ${linter_path} does not have kubeapilinter plugin${NC}" + echo -e "${YELLOW}Linting results may be incomplete. Consider using custom golangci-lint build.${NC}" + return 1 + fi + return 0 +} + +# Find golangci-lint binary +find_golangci_lint() { + # Check for custom build first + if [[ -f ".bingo/golangci-lint" ]]; then + echo ".bingo/golangci-lint" + return 0 + fi + + # Check for bin/golangci-lint + if [[ -f "bin/golangci-lint" ]]; then + echo "bin/golangci-lint" + return 0 + fi + + # Fall back to system golangci-lint + if command -v golangci-lint &> /dev/null; then + echo "golangci-lint" + return 0 + fi + + echo -e "${RED}Error: golangci-lint not found${NC}" + exit 1 +} + +# Run linter and capture output +run_linter() { + local config_file="$1" + local output_file="$2" + local linter_path="$3" + local repo_root="${4:-$(pwd)}" + + # Run golangci-lint on api/ directory only + # Use absolute paths to ensure consistency + (cd "${repo_root}" && "${linter_path}" run \ + --config="${config_file}" \ + --path-prefix="" \ + ./api/...) > "${output_file}" 2>&1 || true +} + +# Parse linter output into structured format +# Format: filename:line:column:linter:message +parse_linter_output() { + local output_file="$1" + local parsed_file="$2" + + # Expected format: path/api/v1/file.go:123:45 linter message + # We need to: extract api/ relative path, parse line:col, linter, and message + grep "/${API_DIR}/" "${output_file}" | \ + sed -E "s|^.*/("${API_DIR}"/[^:]+):([0-9]+):([0-9]+)[[:space:]]+([^[:space:]]+)[[:space:]]+(.+)$|\1:\2:\3:\4:\5|" \ + > "${parsed_file}" || true +} + +# Get list of files changed in api/ directory compared to baseline +get_changed_files() { + git diff "${BASELINE_BRANCH}...HEAD" --name-only -- "${API_DIR}/" | \ + grep '\.go$' | \ + grep -v 'zz_generated' || true +} + +# Categorize issues as NEW or PRE-EXISTING +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" + + # Read changed files into array + local changed_files=() + if [[ -f "${changed_files_file}" ]]; then + while IFS= read -r file; do + changed_files+=("${file}") + done < "${changed_files_file}" + fi + + # Check if current_file exists and is not empty + if [[ ! -f "${current_file}" || ! -s "${current_file}" ]]; then + # No issues found in current branch, nothing to categorize + return 0 + fi + + # Process current issues + while IFS= read -r line; do + [[ -z "${line}" ]] && continue + + local file + file=$(echo "${line}" | cut -d: -f1) + + # Check if file was changed + local file_changed=false + for changed_file in "${changed_files[@]}"; do + if [[ "${file}" == "${changed_file}" ]]; then + file_changed=true + break + fi + done + + # If file wasn't changed, it's pre-existing + if [[ "${file_changed}" == "false" ]]; then + echo "${line}" >> "${preexisting_issues_file}" + continue + fi + + # Check if issue exists in baseline + # 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 + local file_linter_msg + file_linter_msg=$(echo "${line}" | cut -d: -f1,4,5) + + if grep -F ":$(echo "${line}" | cut -d: -f4,5)" "${baseline_file}" 2>/dev/null | \ + grep -Fq "$(echo "${line}" | cut -d: -f1):"; then + echo "${line}" >> "${preexisting_issues_file}" + else + echo "${line}" >> "${new_issues_file}" + fi + done < "${current_file}" +} + +# Output issue (basic format) +output_issue() { + echo "$1" +} + +# Generate basic report +generate_report() { + local new_issues_file="$1" + local preexisting_issues_file="$2" + + local new_count=0 + local preexisting_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 ' ') + + # Simple summary + echo "API Lint Diff Results" + echo "Baseline: ${BASELINE_BRANCH}" + echo "NEW: ${new_count}" + echo "PRE-EXISTING: ${preexisting_count}" + echo "" + + # Show NEW issues + if [[ ${new_count} -gt 0 ]]; then + echo "=== NEW ISSUES ===" + while IFS= read -r line; do + output_issue "${line}" + done < "${new_issues_file}" + echo "" + fi + + # Show PRE-EXISTING issues + if [[ ${preexisting_count} -gt 0 ]]; then + echo "=== PRE-EXISTING ISSUES ===" + while IFS= read -r line; do + output_issue "${line}" + done < "${preexisting_issues_file}" + echo "" + fi + + # Exit based on NEW issues count + if [[ ${new_count} -eq 0 ]]; then + echo -e "${GREEN}NO NEW ISSUES found. Lint check passed.${NC}" + if [[ ${preexisting_count} -gt 0 ]]; then + echo -e "${YELLOW}WARNING: Pre-existing issues detected. Please address them separately.${NC}" + fi + return 0 + else + echo -e "${RED}FAILED: ${new_count} new issue(s)${NC}" + return 1 + fi +} + +# Main execution +main() { + # Find golangci-lint + BASE_LINTER_PATH=$(find_golangci_lint) + + # Build custom linter with kube-api-linter plugin + LINTER_PATH="${BASE_LINTER_PATH}" + if CUSTOM_LINTER=$(build_custom_linter "${BASE_LINTER_PATH}"); then + LINTER_PATH="${CUSTOM_LINTER}" + fi + + # Convert to absolute path if needed + if [[ "${LINTER_PATH}" != /* ]]; then + LINTER_PATH="$(pwd)/${LINTER_PATH}" + fi + + # Create temporary config + create_temp_config + + # Get changed files + get_changed_files > "${TEMP_DIR}/changed_files.txt" + + # Run linter on current branch + REPO_ROOT="$(pwd)" + run_linter "${TEMP_DIR}/.golangci.yaml" "${TEMP_DIR}/current_output.txt" "${LINTER_PATH}" "${REPO_ROOT}" + parse_linter_output "${TEMP_DIR}/current_output.txt" "${TEMP_DIR}/current_parsed.txt" + + # Run linter on baseline + WORKTREE_DIR="${TEMP_DIR}/baseline_worktree" + if ! git worktree add --detach "${WORKTREE_DIR}" "${BASELINE_BRANCH}" 2>&1 | grep -v "Preparing worktree" >&2; then + echo -e "${RED}Error: Failed to create git worktree for baseline branch '${BASELINE_BRANCH}'${NC}" >&2 + echo -e "${RED}Please ensure the branch exists and try again.${NC}" >&2 + exit 1 + fi + run_linter "${TEMP_DIR}/.golangci.yaml" "${TEMP_DIR}/baseline_output.txt" "${LINTER_PATH}" "${WORKTREE_DIR}" + parse_linter_output "${TEMP_DIR}/baseline_output.txt" "${TEMP_DIR}/baseline_parsed.txt" + git worktree remove "${WORKTREE_DIR}" --force &> /dev/null + + # Categorize issues + touch "${TEMP_DIR}/new_issues.txt" + touch "${TEMP_DIR}/preexisting_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" + + # Generate report + generate_report \ + "${TEMP_DIR}/new_issues.txt" \ + "${TEMP_DIR}/preexisting_issues.txt" + + return $? +} + +# Run main function +main "$@"