From 5a7763dbf4a0d3bff1a9bd2431d6262c227264dd Mon Sep 17 00:00:00 2001 From: Jeremy Eder Date: Thu, 29 Jan 2026 00:23:26 -0500 Subject: [PATCH] Feature: Add local make self-validate target (#372) Implements a `make self-validate` target that developers can run locally before pushing changes. This provides the same quality checks as CI/CD, allowing developers to catch issues earlier in their workflow. Changes: - Add `self-validate` target to Makefile - Validates Makefile syntax - Checks shell scripts with shellcheck - Verifies required targets exist - Validates .PHONY declarations - Checks for hardcoded values - Tests help target output - Verifies target documentation coverage - Runs comprehensive validation - Update GitHub Actions workflow - Replace inline validation steps with single `make self-validate` call - Simplifies workflow maintenance - Ensures local and CI validation stay in sync Benefits: - Developers can test locally: `make self-validate` - Faster feedback loop (catch issues before pushing) - Single source of truth for validation logic - Easier to maintain (changes only in Makefile) Co-Authored-By: Claude Sonnet 4.5 --- .github/workflows/makefile-quality.yml | 235 +------------------------ Makefile | 109 +++++++++++- 2 files changed, 111 insertions(+), 233 deletions(-) diff --git a/.github/workflows/makefile-quality.yml b/.github/workflows/makefile-quality.yml index 4dfb52876..7d978f828 100644 --- a/.github/workflows/makefile-quality.yml +++ b/.github/workflows/makefile-quality.yml @@ -90,239 +90,10 @@ jobs: sudo apt-get install -y shellcheck shellcheck --version - - name: Validate Makefile syntax + - name: Run Makefile self-validation run: | - echo "🔍 Validating Makefile syntax and best practices..." - make lint-makefile - - - name: Check shell scripts - run: | - echo "🔍 Checking embedded shell scripts..." - make check-shell - - - name: Verify all required targets exist - run: | - echo "🔍 Verifying required targets exist..." - - # AGENT INSTRUCTIONS: This array contains the minimum set of targets that must exist - # in the Makefile for the project to function correctly. Update this list when: - # - # ADD a target here if: It's a critical user-facing target that other automation depends on - # REMOVE a target if: It's being deprecated AND no other workflows/docs reference it - # VERIFY before changing: Search codebase for references to target name - # - # Current categories: - # - Core: help (required for discoverability) - # - Local dev: local-up, local-down, local-status, local-test-quick - # - Quality: validate-makefile, lint-makefile, check-shell, makefile-health - # - CI/CD: build-all, deploy, clean - # - # To verify a target exists: grep "^target-name:" Makefile - required_targets=( - "help" - "local-up" - "local-down" - "local-status" - "local-test-quick" - "validate-makefile" - "lint-makefile" - "check-shell" - "makefile-health" - "build-all" - "deploy" - "clean" - ) - - missing_targets=() - for target in "${required_targets[@]}"; do - if ! grep -q "^${target}:" Makefile; then - missing_targets+=("$target") - fi - done - - if [ ${#missing_targets[@]} -gt 0 ]; then - echo "❌ Missing required targets:" - printf ' - %s\n' "${missing_targets[@]}" - exit 1 - else - echo "✅ All required targets present" - fi - - - name: Verify .PHONY declarations - run: | - echo "🔍 Verifying .PHONY declarations..." - - # Extract all target names (excluding internal targets starting with _) - targets=$(grep -E '^[a-zA-Z][a-zA-Z0-9_-]+:' Makefile | grep -v '^_' | cut -d: -f1 | sort -u) - - # Extract .PHONY declarations - phony_targets=$(grep '^\.PHONY:' Makefile | sed 's/^\.PHONY: //' | tr ' ' '\n' | sort -u) - - # Count targets and phony declarations - target_count=$(echo "$targets" | wc -l) - phony_count=$(echo "$phony_targets" | wc -l) - - echo "📊 Found $target_count targets, $phony_count in .PHONY" - - # Find targets not in .PHONY (excluding pattern rules and variable assignments) - not_phony=() - while IFS= read -r target; do - if ! echo "$phony_targets" | grep -q "^${target}$"; then - # Skip if it's a pattern rule or special target - if [[ ! "$target" =~ ^% ]] && [[ ! "$target" =~ ^\. ]]; then - not_phony+=("$target") - fi - fi - done <<< "$targets" - - if [ ${#not_phony[@]} -gt 0 ]; then - echo "⚠️ Targets not declared in .PHONY (may be intentional for file targets):" - printf ' - %s\n' "${not_phony[@]}" | head -10 - else - echo "✅ All non-file targets properly declared in .PHONY" - fi - - - name: Check for hardcoded values - run: | - echo "🔍 Checking for hardcoded values that should be variables..." - - # AGENT INSTRUCTIONS: This check detects hardcoded values that should use variables. - # It prevents configuration drift and ensures Makefile remains configurable. - # - # WHEN TO UPDATE THESE CHECKS: - # - # 1. IF VARIABLE NAMES CHANGE (e.g., NAMESPACE → KUBE_NAMESPACE): - # - Update the variable name in grep commands - # - Update exclusion patterns (grep -v lines) - # - Test: Ensure variable declaration line is excluded from warnings - # - # 2. IF DEFAULT VALUES CHANGE (e.g., ambient-code → new-namespace): - # - Update the search string to match new default value - # - Keep the variable name check pattern - # - Current defaults defined in Makefile lines ~7-11 - # - # 3. IF ADDING NEW CONFIGURABLE VALUES: - # - Add similar check block following this pattern - # - Exclude: variable declarations, comments, help text - # - Example: Check for hardcoded registry names - # - # CURRENT CHECKS: - # - namespace: "ambient-code" should use $(NAMESPACE) variable - # - container engine: "docker" or "podman" should use $(CONTAINER_ENGINE) variable - # - # WHY THESE EXCLUSIONS: - # - "NAMESPACE ?=" = variable declaration itself (Makefile line 10) - # - "^[0-9]*:#" = comments (documentation, not code) - # - "@echo" = help text displayed to users (intentionally literal) - # - # TO VERIFY: grep -n "value" Makefile | grep -v "VARIABLE ?=" | grep -v "^[0-9]*:#" - - # Check for hardcoded namespaces outside of variable declaration (should use $(NAMESPACE)) - # Excludes: NAMESPACE ?= line, comments, and @echo help text - if grep -n "ambient-code" Makefile | grep -v "NAMESPACE ?=" | grep -v "^[0-9]*:#" | grep -v "@echo" | grep -q .; then - echo "⚠️ Found hardcoded 'ambient-code' references (should use \$(NAMESPACE) variable):" - grep -n "ambient-code" Makefile | grep -v "NAMESPACE ?=" | grep -v "^[0-9]*:#" | grep -v "@echo" | head -5 - echo " To fix: Replace 'ambient-code' with '\$(NAMESPACE)' in these locations" - fi - - # Check for hardcoded container engines outside of variable references - # Excludes: lines with CONTAINER_ENGINE variable, comments, and help text - if grep -nE "docker|podman" Makefile | grep -v "CONTAINER_ENGINE" | grep -v "^[0-9]*:#" | grep -v "@echo" | grep -q .; then - echo "⚠️ Found hardcoded docker/podman references (should use \$(CONTAINER_ENGINE) variable):" - grep -nE "docker|podman" Makefile | grep -v "CONTAINER_ENGINE" | grep -v "^[0-9]*:#" | grep -v "@echo" | head -5 - echo " To fix: Replace 'docker' or 'podman' with '\$(CONTAINER_ENGINE)' in these locations" - fi - - echo "✅ No problematic hardcoded values found" - - - name: Test help target output - run: | - echo "🔍 Testing help target..." - make help > /tmp/help-output.txt - - # AGENT INSTRUCTIONS: These strings MUST match the exact section headers in the Makefile - # help target output. When modifying the Makefile help format: - # - # 1. Run: make help | grep -E "Section Name:" to see actual output - # 2. Update the grep patterns below to match new header text exactly - # 3. Consider if renaming improves user experience (prefer clarity over brevity) - # 4. These checks ensure help output meets minimum usability standards - # - # Current required sections defined in Makefile (lines ~39-49): - # - "Quick Start:" - Essential commands for new users - # - "Quality Assurance:" - Quality/validation commands - # - "Available Targets:" - Complete target listing (auto-generated by awk) - # - # To verify: make help | grep -o "^[A-Za-z ]*:" | sort -u - - if ! grep -q "Quick Start:" /tmp/help-output.txt; then - echo "❌ Help output missing 'Quick Start' section" - echo " Update this check if you renamed the section in Makefile" - exit 1 - fi - - if ! grep -q "Quality Assurance:" /tmp/help-output.txt; then - echo "❌ Help output missing 'Quality Assurance' section" - echo " Update this check if you renamed the section in Makefile" - exit 1 - fi - - if ! grep -q "Available Targets:" /tmp/help-output.txt; then - echo "❌ Help output missing 'Available Targets' section" - echo " This is auto-generated by awk in Makefile line ~49" - exit 1 - fi - - echo "✅ Help target produces expected output" - - - name: Verify target documentation - run: | - echo "🔍 Checking target documentation..." - - # AGENT INSTRUCTIONS: This check measures documentation quality by counting - # how many Makefile targets have inline help text (## comments). - # - # DOCUMENTATION FORMAT: target-name: ## Help text shown in 'make help' - # - # WHEN TO ADJUST THRESHOLD: - # - 50% minimum = Current threshold for acceptable quality - # - Increase to 75%+ if requiring comprehensive documentation - # - Decrease to 30%+ only if many internal/experimental targets exist - # - # RATIONALE FOR 50% THRESHOLD: - # - All user-facing targets SHOULD have documentation - # - Internal targets (prefixed with _) are automatically excluded - # - Helper targets may not need docs if only called by other targets - # - 50% ensures at least half of public API is documented - # - # TO ADD DOCUMENTATION: Add ## comment after target definition - # Example: my-target: ## Description of what this target does - # - # TO VERIFY: grep '^target-name:.*##' Makefile - - # Count targets with documentation (excluding internal targets starting with _) - total_targets=$(grep -E '^[a-zA-Z][a-zA-Z0-9_-]+:' Makefile | grep -v '^_' | wc -l) - documented_targets=$(grep -E '^[a-zA-Z][a-zA-Z0-9_-]+:.*##' Makefile | wc -l) - - echo "📊 $documented_targets/$total_targets targets have help text" - - # Calculate percentage (threshold: 50% minimum for good documentation practice) - if [ "$total_targets" -gt 0 ]; then - percentage=$((documented_targets * 100 / total_targets)) - echo "📈 Documentation coverage: ${percentage}%" - - if [ "$percentage" -lt 50 ]; then - echo "⚠️ Documentation coverage below 50%, consider adding help text (## comments) to more targets" - echo " Add documentation by appending '## Description' after target definition" - else - echo "✅ Good target documentation coverage (${percentage}%)" - fi - fi - - - name: Run comprehensive validation - run: | - echo "🔍 Running comprehensive Makefile validation..." - make validate-makefile + echo "🔍 Running Makefile self-validation (all quality checks)..." + make self-validate - name: Summary if: success() diff --git a/Makefile b/Makefile index 1edcec9e4..6db741566 100644 --- a/Makefile +++ b/Makefile @@ -6,7 +6,7 @@ .PHONY: push-all registry-login setup-hooks remove-hooks check-minikube check-kind check-kubectl .PHONY: e2e-test e2e-setup e2e-clean deploy-langfuse-openshift .PHONY: setup-minio minio-console minio-logs minio-status -.PHONY: validate-makefile lint-makefile check-shell makefile-health +.PHONY: self-validate validate-makefile lint-makefile check-shell makefile-health .PHONY: _create-operator-config _auto-port-forward _show-access-info _build-and-load # Default target @@ -369,6 +369,113 @@ test-all: local-test-quick local-test-dev ## Run all tests (quick + comprehensiv ##@ Quality Assurance +self-validate: ## Run complete Makefile validation (same checks as CI) + @echo "$(COLOR_BOLD)🔍 Running Makefile Self-Validation$(COLOR_RESET)" + @echo "" + @echo "$(COLOR_BLUE)▶$(COLOR_RESET) Step 1/8: Validating Makefile syntax..." + @$(MAKE) --no-print-directory lint-makefile + @echo "" + @echo "$(COLOR_BLUE)▶$(COLOR_RESET) Step 2/8: Checking shell scripts..." + @$(MAKE) --no-print-directory check-shell + @echo "" + @echo "$(COLOR_BLUE)▶$(COLOR_RESET) Step 3/8: Verifying required targets exist..." + @required_targets=( \ + "help" \ + "local-up" \ + "local-down" \ + "local-status" \ + "local-test-quick" \ + "self-validate" \ + "validate-makefile" \ + "lint-makefile" \ + "check-shell" \ + "makefile-health" \ + "build-all" \ + "deploy" \ + "clean" \ + ); \ + missing_targets=(); \ + for target in "$${required_targets[@]}"; do \ + if ! grep -q "^$$target:" Makefile; then \ + missing_targets+=("$$target"); \ + fi; \ + done; \ + if [ $${#missing_targets[@]} -gt 0 ]; then \ + echo "$(COLOR_RED)✗$(COLOR_RESET) Missing required targets:"; \ + printf ' - %s\n' "$${missing_targets[@]}"; \ + exit 1; \ + else \ + echo "$(COLOR_GREEN)✓$(COLOR_RESET) All required targets present"; \ + fi + @echo "" + @echo "$(COLOR_BLUE)▶$(COLOR_RESET) Step 4/8: Verifying .PHONY declarations..." + @targets=$$(grep -E '^[a-zA-Z][a-zA-Z0-9_-]+:' Makefile | grep -v '^_' | cut -d: -f1 | sort -u); \ + phony_targets=$$(grep '^\.PHONY:' Makefile | sed 's/^\.PHONY: //' | tr ' ' '\n' | sort -u); \ + target_count=$$(echo "$$targets" | wc -l); \ + phony_count=$$(echo "$$phony_targets" | wc -l); \ + echo "$(COLOR_GREEN)✓$(COLOR_RESET) Found $$target_count targets, $$phony_count in .PHONY"; \ + not_phony=(); \ + while IFS= read -r target; do \ + if ! echo "$$phony_targets" | grep -q "^$${target}$$"; then \ + if [[ ! "$$target" =~ ^% ]] && [[ ! "$$target" =~ ^\. ]]; then \ + not_phony+=("$$target"); \ + fi; \ + fi; \ + done <<< "$$targets"; \ + if [ $${#not_phony[@]} -gt 0 ]; then \ + echo "$(COLOR_YELLOW)⚠$(COLOR_RESET) Targets not declared in .PHONY (may be intentional):"; \ + printf ' - %s\n' "$${not_phony[@]}" | head -10; \ + fi + @echo "" + @echo "$(COLOR_BLUE)▶$(COLOR_RESET) Step 5/8: Checking for hardcoded values..." + @if grep -n "ambient-code" Makefile | grep -v "NAMESPACE ?=" | grep -v "^[0-9]*:#" | grep -v "@echo" | grep -q .; then \ + echo "$(COLOR_YELLOW)⚠$(COLOR_RESET) Found hardcoded 'ambient-code' references:"; \ + grep -n "ambient-code" Makefile | grep -v "NAMESPACE ?=" | grep -v "^[0-9]*:#" | grep -v "@echo" | head -5; \ + echo " To fix: Replace 'ambient-code' with '\$$(NAMESPACE)'"; \ + fi; \ + if grep -nE "docker|podman" Makefile | grep -v "CONTAINER_ENGINE" | grep -v "^[0-9]*:#" | grep -v "@echo" | grep -q .; then \ + echo "$(COLOR_YELLOW)⚠$(COLOR_RESET) Found hardcoded docker/podman references:"; \ + grep -nE "docker|podman" Makefile | grep -v "CONTAINER_ENGINE" | grep -v "^[0-9]*:#" | grep -v "@echo" | head -5; \ + echo " To fix: Replace with '\$$(CONTAINER_ENGINE)'"; \ + fi; \ + echo "$(COLOR_GREEN)✓$(COLOR_RESET) Hardcoded value check complete" + @echo "" + @echo "$(COLOR_BLUE)▶$(COLOR_RESET) Step 6/8: Testing help target output..." + @make help > /tmp/help-output.txt 2>&1; \ + if ! grep -q "Quick Start:" /tmp/help-output.txt; then \ + echo "$(COLOR_RED)✗$(COLOR_RESET) Help output missing 'Quick Start' section"; \ + exit 1; \ + fi; \ + if ! grep -q "Quality Assurance:" /tmp/help-output.txt; then \ + echo "$(COLOR_RED)✗$(COLOR_RESET) Help output missing 'Quality Assurance' section"; \ + exit 1; \ + fi; \ + if ! grep -q "Available Targets:" /tmp/help-output.txt; then \ + echo "$(COLOR_RED)✗$(COLOR_RESET) Help output missing 'Available Targets' section"; \ + exit 1; \ + fi; \ + rm -f /tmp/help-output.txt; \ + echo "$(COLOR_GREEN)✓$(COLOR_RESET) Help target produces expected output" + @echo "" + @echo "$(COLOR_BLUE)▶$(COLOR_RESET) Step 7/8: Verifying target documentation..." + @total_targets=$$(grep -E '^[a-zA-Z][a-zA-Z0-9_-]+:' Makefile | grep -v '^_' | wc -l); \ + documented_targets=$$(grep -E '^[a-zA-Z][a-zA-Z0-9_-]+:.*##' Makefile | wc -l); \ + if [ "$$total_targets" -gt 0 ]; then \ + percentage=$$((documented_targets * 100 / total_targets)); \ + echo "$(COLOR_GREEN)✓$(COLOR_RESET) Documentation coverage: $${percentage}% ($$documented_targets/$$total_targets targets)"; \ + if [ "$$percentage" -lt 50 ]; then \ + echo "$(COLOR_YELLOW)⚠$(COLOR_RESET) Coverage below 50%, consider adding '## comments' to more targets"; \ + fi; \ + fi + @echo "" + @echo "$(COLOR_BLUE)▶$(COLOR_RESET) Step 8/8: Running comprehensive validation..." + @$(MAKE) --no-print-directory validate-makefile + @echo "" + @echo "$(COLOR_GREEN)✅ All validation checks passed!$(COLOR_RESET)" + @echo "" + @echo "Your Makefile meets quality standards and is ready to commit." + + validate-makefile: lint-makefile check-shell ## Validate Makefile quality and syntax @echo "$(COLOR_GREEN)✓ Makefile validation passed$(COLOR_RESET)"