Skip to content

Feature: Add local make self-validate target (#372) #553

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
jeremyeder wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Feature: Add local make self-validate target (#372) #553

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
235 changes: 3 additions & 232 deletions .github/workflows/makefile-quality.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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()
Expand Down
109 changes: 108 additions & 1 deletion Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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)"

Expand Down
Loading