refactor: remove vendor directory and switch to Go modules

[elijah-rou]

Removes vendoring in favour of mod mode

[github-actions]

[shellcheck (suggestion)] _{reported by reviewdog 🐶}

Suggested change

	readonly TMP_DIFFROOT="$(mktemp -d ${REPO_ROOT_DIR}/tmpdiffroot.XXXXXX)"
	readonly TMP_DIFFROOT="$(mktemp -d "${REPO_ROOT_DIR}"/tmpdiffroot.XXXXXX)"

[claude]

Code Review: Remove Vendor Directory and Switch to Go Modules

Summary

This PR removes ~2M lines of vendor code and transitions to Go modules. Core execution is solid but has 2 blocking issues.

Assessment: Request Changes ⚠️

🚨 BLOCKERS

1. Unrelated Documentation Files
These don't belong in a vendoring PR:

• HTMX_FIXES_SUMMARY.md (234 lines)
• HTMX_OPTIMIZATION_COMPLETE.md (192 lines)
• REQUEST_TRACKING_NEXT_STEPS.md (219 lines)

Action: Remove or explain inclusion

2. verify-codegen.sh References Deleted vendor/
Line 40 still copies/diffs vendor/ causing CI failure.
Action: Remove vendor operations

⚠️ Major Concerns

3. Config Vendoring Needs Documentation
Files like config/core/300-resources/*.yaml are now copied from deps with headers. Need explanation of maintenance workflow.

4. handler_test.go: 403 Lines Deleted
Need justification for massive test reduction. Coverage still adequate?

5. throttler.go: 9 Lines Deleted
Critical file - need explicit changelog of what was removed.

💡 Minor Issues

• kind-e2e.yaml: Test matrix reduced (K8s versions, ingress controllers)
• update-config-symlinks.sh: Marked deprecated but has active logic
• .gitignore: New entries lack comments

✅ Positives

• Proper go.mod/go.sum setup
• CI workflows correctly updated
• New scripts (update-deps-configs.sh) well-written
• Clean vendor removal
• Good performance improvements expected

🔒 Security

✅ No issues detected

📝 Recommendation

REQUEST CHANGES - Fix 2 blockers, then approve.

Checklist Before Merge

• Remove unrelated markdown files
• Fix verify-codegen.sh
• Document test deletions in PR description
• Add config vendoring workflow explanation
• All CI passing
• Coverage check passed
• Manual deployment tested

Great technical execution! Just need blockers fixed and more context added. 🚀

[claude]

Comprehensive Code Review: Remove Vendor Directory and Switch to Go Modules

Executive Summary

This PR removes ~2M lines of vendored code (1,994,033 deletions) and transitions to Go module-based dependency management. The change includes 15 commits spanning vendor removal, CI updates, lint fixes, and config file vendoring. While the core technical approach is sound, there are critical issues that must be addressed before merging.

Recommendation: ⚠️ Request Changes

---

🚨 Critical Blockers

1. Unrelated Documentation Files (HIGH PRIORITY)

Three large documentation files are included that appear completely unrelated to vendor removal:

• HTMX_FIXES_SUMMARY.md (234 lines) - Documents HTMX dashboard fixes
• HTMX_OPTIMIZATION_COMPLETE.md (192 lines) - HTMX optimization notes
• REQUEST_TRACKING_NEXT_STEPS.md (219 lines) - Request tracking next steps

Issues:

• These files have no connection to Go modules or vendor removal
• They appear to document work from a different feature branch
• Including them violates single-responsibility principle for PRs
• Makes code review confusing and git history unclear

Required Action: Remove these files from this PR or provide clear justification for their inclusion in a vendor removal PR.

---

2. Lint Fix: ErrorQuick502 → Error Naming Convention

The PR renames ErrQuick502 to ErrorQuick502 to comply with Go error naming conventions:

// Before: type ErrQuick502 struct
// After: type ErrorQuick502 struct  

Issue: This is a breaking change for error type assertions. Any code checking for this error type will break:

// This code will now break:
if err, ok := myErr.(handler.ErrQuick502); ok {
    // handle quick 502
}

Assessment from CLAUDE.md Context:

• Quick 502 detection is currently disabled (lines 263-270 in handler.go)
• Feature was disabled due to "issues with quick 502s causing unnecessary quarantines"
• If no code actively uses this error type, the rename is safe

Required Action:

• Confirm no code performs type assertions on ErrQuick502
• If this error type is unused, consider removing it entirely instead of renaming
• If used, document the breaking change and migration path

---

3. gosec: Unsafe Integer Conversion in breaker.go

Line 66 in pkg/queue/breaker.go:

totalSlots: int64(params.QueueDepth) + int64(params.MaxConcurrency), //nolint:gosec

Issue: The //nolint:gosec suppresses overflow checking for uint64→int64 conversions.

Risk Analysis:

• QueueDepth = 10,000 (constant)
• MaxConcurrency = math.MaxInt32 (2,147,483,647)
• Sum = 2,147,493,647 < math.MaxInt64 ✅ Safe
• However, comment claims "Arithmetic checked by constants" but doesn't show the math

From CLAUDE.md (commit 6f3f7ca):

uint64 Migration: Complete type migration to eliminate integer overflow risks

• Validates at API boundary where K8s data enters the system
• Eliminates need for unsafe conversions

Required Action:

• Add explicit validation before conversion:

  if params.QueueDepth > math.MaxInt64 - params.MaxConcurrency {
      panic("capacity overflow")
  }

• Or better: change totalSlots to uint64 to match params
• Remove //nolint suppressions after adding validation

---

⚠️ Major Concerns

4. Config File Vendoring Strategy Needs Documentation

The PR introduces a new pattern of "vendoring" config files from dependencies:

Files Affected:

• config/core/configmaps/network.yaml
• config/core/300-resources/*.yaml (certificate, domain-claim, ingress, serverlessservice)
• config/core/300-imagecache.yaml

What Changed:

• Old: Symlinks to Go module cache (broken when cache cleared)
• New: Copied files with version headers from hack/update-deps-configs.sh

Concerns:

1. No README/docs explaining this pattern to other developers
2. Maintenance burden - developers must remember to run hack/update-deps-configs.sh after go mod tidy
3. CI verification - relies on verify-deps-configs.sh catching stale configs
4. Merge conflicts - these files will conflict on dependency updates across branches

Positive: The automation scripts are well-written with good error messages.

Required Action:

• Add documentation to README or CONTRIBUTING.md explaining:
  • Why config files are vendored (CRD stability, reproducible builds)
  • When to run update-deps-configs.sh (after go mod changes)
  • How CI enforces this (verify workflow)

---

5. Massive Test Deletion in handler_test.go (403 Lines)

The PR deletes 403 lines of test code from pkg/activator/handler/handler_test.go.

Concern: Without seeing the original tests, I cannot verify:

• Whether test coverage decreased
• If deleted tests are redundant or critical
• Whether functionality is still tested elsewhere

From commit 4ce698d:

"The Python regex script that fixed errorlint violations incorrectly indented the error handling code block. Rewrote the test file with proper indentation and formatting."

Red Flag: This commit message suggests the test file was rewritten by a script, not manually reviewed. This is extremely dangerous for test code.

Required Action:

• Provide git diff showing exactly which tests were removed
• Confirm test coverage hasn't decreased: go test -cover ./pkg/activator/handler
• Manually review all test changes instead of relying on automated rewrites

---

6. Missing Function Deletions: unused functions removed

Commit 5530c3d removed:

• minOneOrValue()
• defaultHTTPProbeJSON()

Concern: Need to verify these functions truly had zero call sites.

Required Action: Confirm with grep/Glob that these functions had no usages:

git show 5530c3d1^:path/to/file.go | grep -n "minOneOrValue\|defaultHTTPProbeJSON"

---

✅ Positive Changes

7. Error Handling Improvements (errorlint fixes)

Properly replaced type assertions with errors.As() and errors.Is():

// Before: if ne, ok := err.(net.Error); ok && ne.Timeout()
// After: if errors.As(err, &ne) && ne.Timeout()

Assessment: ✅ Correct - follows Go 1.13+ error handling best practices.

---

8. Context-Aware Network Operations (noctx fixes)

Updated network operations to accept context:

// pkg/autoscaler/statserver/server.go - now uses context for cancellation

Assessment: ✅ Good - enables proper request cancellation and timeouts.

---

9. Go 1.22+ Range Syntax (intrange fixes)

Tests updated to use modern range-over-integer syntax:

// Before: for i := 0; i < 10; i++ 
// After: for range 10

Assessment: ✅ Positive - cleaner syntax where iterator isn't used.

---

10. Embedded Field Selector Simplification (staticcheck QF1008)

Removed unnecessary embedded struct field selectors across 34 files:

// Before: svc.ObjectMeta.Name
// After: svc.Name

Assessment: ✅ Good - cleaner code. Note from commit 9f24563 shows one case (route.ObjectMeta.DeepCopy()) was correctly preserved where the explicit selector was needed.

---

11. Hack Scripts Modernization

All hack/*.sh scripts updated to work with Go modules instead of vendor directory:

Changes:

• Replace source vendor/knative.dev/hack/... with dynamic module lookups
• Use go list -m -f '{{.Dir}}' to find module paths
• Use go run module@version instead of go run vendor/path

Assessment: ✅ Well done - maintains script functionality while removing vendor dependency.

---

12. CI Workflow Updates

Updated 4 workflow files:

• kind-e2e.yaml - Added symlink update step before builds
• knative-go-test.yaml - Custom workflow with test setup
• knative-style.yaml - Removed boilerplate check (appropriate for fork)
• knative-verify.yaml - Added vendored config verification

Assessment: ✅ Appropriate - fork doesn't need upstream boilerplate checks.

---

🔍 Code Quality Assessment

Commit Quality: ⚠️ Mixed

Good commits:

• 3297b41: "refactor: remove vendor directory and switch to Go modules" - Clear, focused
• 956ca93: "feat: vendor config files from dependencies with automation" - Well-documented

Problematic commits:

• ac574d6: "udpate codecov" - Typo in message, no body
• 4ce698d: Test file "rewritten by Python script" - Dangerous for test code

Test Coverage: ⚠️ Unknown (Needs Verification)

Cannot assess without running:

go test -cover ./...

Security: ⚠️ One Issue (gosec in breaker.go)

See blocker #3 above.

---

🎯 Recommendations

Must Fix Before Merge:

1. ✅ Remove unrelated HTMX documentation files
2. ✅ Verify ErrorQuick502 rename doesn't break existing code
3. ✅ Add overflow validation or change totalSlots to uint64 in breaker.go
4. ✅ Manually review handler_test.go changes and confirm coverage
5. ✅ Document config file vendoring strategy in README/CONTRIBUTING.md

Should Fix:

6. Fix commit message typos ("udpate" → "update")
7. Add test coverage reporting to PR description
8. Verify deleted functions (minOneOrValue, defaultHTTPProbeJSON) had zero usages

Nice to Have:

9. Add make vendor-configs target to Makefile
10. Consider pre-commit hook to verify config files are up-to-date
11. Add Go 1.22+ minimum version to go.mod if using range-over-int

---

🏁 Conclusion

This PR accomplishes its core goal of removing vendor dependencies and modernizing to Go modules. The automation scripts are well-written and the CI integration is solid. However, the unrelated documentation files and lack of config vendoring docs are blocking issues.

The lint fixes are mostly positive, but the ErrorQuick502 rename needs verification and the gosec suppression in breaker.go needs proper validation.

Final Verdict:

• Core technical work: A- (very good)
• PR hygiene: C (unrelated files included)
• Documentation: C+ (missing config strategy docs)
• Security: B (one nolint suppression needs fixing)

Overall: Request Changes - Fix blockers 1-4, then approve.

---

Reviewed against CLAUDE.md guidelines - HTMX docs clearly don't belong in a vendor removal PR per single-responsibility principle.

[claude]

Follow-up Review: PR #72 - Vendor Removal

Status Check

I've reviewed this PR previously and identified several critical issues. Here's a summary of the blocking items that must be addressed:

🚨 Critical Blockers (From Previous Reviews)

1. Unrelated Documentation Files

The following files appear unrelated to vendor removal and should be removed or explained:

• HTMX_FIXES_SUMMARY.md (234 lines)
• HTMX_OPTIMIZATION_COMPLETE.md (192 lines)
• REQUEST_TRACKING_NEXT_STEPS.md (219 lines)

Action Required: Remove these files from this PR or provide clear justification for their inclusion.

---

2. Breaking Change: ErrorQuick502 Rename

The rename from ErrQuick502 to ErrorQuick502 is a breaking change for any code performing type assertions on this error.

Action Required:

• Verify no code uses type assertions on ErrQuick502
• If unused (Quick 502 detection is currently disabled), consider removing entirely instead of renaming
• Document the breaking change if this error type is in use

---

3. Integer Overflow Safety in breaker.go

Line 66 uses //nolint:gosec to suppress overflow checking:

totalSlots: int64(params.QueueDepth) + int64(params.MaxConcurrency), //nolint:gosec

Per CLAUDE.md (commit 6f3f7ca): The codebase underwent uint64 migration to eliminate integer overflow risks.

Action Required:

• Add explicit overflow validation before conversion, OR
• Change totalSlots to uint64 to match params types
• Remove //nolint suppressions

---

4. Test Coverage Verification Required

pkg/activator/handler/handler_test.go had 403 lines deleted with note about "Python regex script rewriting".

Action Required:

• Run go test -cover ./pkg/activator/handler and confirm coverage hasn't decreased
• Manually review test changes (automated rewrites of test code are dangerous)
• Document which tests were removed and why

---

5. Config File Vendoring Documentation

The new pattern of vendoring config files from dependencies needs documentation.

Action Required:

• Add documentation to README or CONTRIBUTING.md explaining:
  • Why config files are vendored (CRD stability)
  • When to run hack/update-deps-configs.sh (after go mod changes)
  • How CI enforces this (verify workflow)

---

✅ What's Good

• Core vendor removal is well-executed
• CI workflows properly updated
• Hack scripts modernized correctly
• Most lint fixes are appropriate improvements
• Config automation scripts are well-written

---

📋 Next Steps

1. Address the 5 blocking issues above
2. Ensure all CI checks are passing
3. Run full test suite with coverage reporting
4. Manual deployment test in staging environment

Once these blockers are resolved, this PR will be ready to merge. The core technical work is solid - just needs cleanup of unrelated files and better documentation.

Current Recommendation: ⚠️ Request Changes (same as previous reviews)

---

Note: This is a follow-up to my previous comprehensive reviews. The issues identified remain the same - please address the blockers listed above.

[claude]

Pull Request Review: Remove Vendor Directory and Switch to Go Modules

This is a substantial refactoring that removes ~1.99M lines of vendored code and transitions to pure Go module mode. Overall, this is a positive change that modernizes the dependency management approach. Here's my detailed review:

---

✅ Strengths

1. Strategic Infrastructure Improvement

• Repository Size Reduction: Removing the vendor directory dramatically reduces repository size and speeds up git operations
• Dependency Management: Modern Go module approach is more maintainable than vendoring
• Clear Benefits: Faster clones, easier dependency updates, standard Go tooling

2. Comprehensive Implementation

• Complete Transition: All 17 commits form a logical progression from vendor removal → script updates → CI fixes → lint fixes
• Config File Vendoring Solution: Smart approach to vendor only the critical CRD config files with version headers and automation scripts (hack/update-deps-configs.sh, hack/verify-deps-configs.sh)
• CI Integration: Thorough updates to all CI workflows to handle the new structure

3. Code Quality Improvements

The lint fixes in commit 5530c3d are excellent and address real code quality issues:

• Error Handling: Proper use of errors.Is() and errors.As() instead of type assertions ✅
• Naming Convention: ErrQuick502 → ErrorQuick502 (follows Go convention for error types) ✅
• Overflow Protection: Fixed uint64→int64 conversion in breaker.go with //nolint:gosec and bounds checking ✅
• Context Usage: Updated network operations to use context-aware APIs ✅
• Dead Code Removal: Removed unused functions and directives ✅

---

⚠️ Issues & Concerns

1. Critical: Error Type Rename Breaking Change 🔴

File: pkg/activator/handler/handler.go:54-61

-// ErrQuick502 indicates a pod returned a 502 error in less than 100ms
-type ErrQuick502 struct {
+type ErrorQuick502 struct {

Issue: This is a public API breaking change. The type was exported and may be used by external code or health check logic.

Recommendation:

• If this is only used internally, make it private: errorQuick502
• If it needs to be public, keep ErrQuick502 and add a type alias for compatibility:

  // Deprecated: Use ErrorQuick502 instead
  type ErrQuick502 = ErrorQuick502

2. Test Code Massacre ⚠️

File: pkg/activator/handler/handler_test.go (-403 lines, +79 lines)

Issue: The test file lost 324 net lines of code. This is concerning:

• Was critical test coverage removed?
• Were these tests redundant or broken by the refactoring?
• The CLAUDE.md guidelines emphasize comprehensive testing

Action Required: Please clarify what tests were removed and why. If test coverage decreased, this needs to be addressed before merging.

3. Hardcoded Magic Constants

File: pkg/queue/breaker.go:66

totalSlots: int64(params.QueueDepth) + int64(params.MaxConcurrency), //nolint:gosec

Issue: While the //nolint:gosec is justified by the comment, the overflow protection relies on package constants (MaxBreakerCapacity = math.MaxInt32). However, there's no explicit overflow check at runtime.

Recommendation: Add a runtime check before the nolint:

if params.QueueDepth > math.MaxInt64 - params.MaxConcurrency {
    panic(fmt.Sprintf("Queue depth + max concurrency exceeds int64 max. Got %v + %v.", params.QueueDepth, params.MaxConcurrency))
}
totalSlots: int64(params.QueueDepth) + int64(params.MaxConcurrency), //nolint:gosec // Overflow checked above

4. Inconsistent Error Handling Pattern

Files: pkg/activator/handler/handler.go and metrics_roundtripper.go

The code now properly uses errors.Is() and errors.As(), but there's an inconsistency:

• handler.go:170: Uses errors.Is(err, queue.ErrRequestQueueFull) ✅
• handler.go:234: Uses errors.Is(err, context.DeadlineExceeded) ✅
• metrics_roundtripper.go:36: Helper function errorsIsDeadlineExceeded() duplicates standard errors.Is()

Recommendation: Remove the redundant helper and use errors.Is() directly everywhere for consistency.

5. Config Vendoring Complexity

Files: hack/update-deps-configs.sh, hack/verify-deps-configs.sh

Concern: The config file vendoring adds operational complexity:

• Developers must remember to run update-deps-configs.sh after updating dependencies
• CI verification adds build time
• Testdata symlinks (hack/update-testdata-symlinks.sh) create fragility

Questions:

1. Could these config files be read directly from the module cache at runtime instead of vendoring?
2. What happens when a new version of networking/caching introduces breaking config changes?

Recommendation: Document the vendoring workflow in CLAUDE.md under "Development Guidelines" since this is a non-standard Go pattern.

6. CI Workflow Divergence

Files: .github/workflows/knative-style.yaml, knative-verify.yaml

Issue: Custom workflows replace upstream Knative reusable workflows to skip boilerplate checks. This creates maintenance burden:

• Future upstream improvements won't be inherited automatically
• Fork must manually sync useful upstream changes
• More YAML to maintain (247 lines added)

Recommendation: Consider contributing a skip-boilerplate parameter to upstream Knative workflows instead of forking the entire workflow. This would benefit other forks too.

---

🔍 Security Considerations

1. Dependency Pinning

• ✅ Go modules provide hash verification via go.sum
• ⚠️ Make sure go.sum is committed and CI verifies it

2. Supply Chain Attack Surface

• ✅ Reduced attack surface by removing vendored code that could be poisoned
• ⚠️ Increased reliance on module proxy and upstream repos
• ✅ Go module checksum database provides protection

3. Reproducible Builds

• ⚠️ Module cache behavior could vary across environments
• ✅ Vendored config files ensure CRD stability
• ⚠️ Document required Go version constraints in README

---

📊 Performance Impact

Positive

• Faster git operations (clone, fetch, checkout)
• Smaller Docker build contexts
• Standard Go tooling works better

Potential Negatives

• First build downloads modules (network dependency)
• CI builds may be slightly slower on cache misses

Recommendation: Ensure CI caches the Go module cache (~/.cache/go-build, ~/go/pkg/mod)

---

🧪 Testing Recommendations

1. Verify All Tests Pass: Run go test -race ./... per CLAUDE.md guidelines
2. Test Operating Modes: Verify all feature gate combinations still work (QP authority, Quarantine)
3. Integration Tests: Ensure the Tilt development environment still works with module mode
4. Build Verification: Test builds in clean environments without module cache
5. CI Full Run: Ensure all CI workflows pass (kind-e2e, tests, verify, style)

---

📝 Documentation Updates Needed

1. CLAUDE.md: Add "Working with Vendored Config Files" section explaining:

   • When to run hack/update-deps-configs.sh
   • How config versioning works
   • What to do when config updates fail

2. README or CONTRIBUTING: Document the module-mode workflow:

   • First-time setup (go mod download)
   • Dependency updates (go get, update-deps-configs.sh)
   • Troubleshooting module cache issues

3. Tiltfile: Verify documentation matches new build process

---

✅ Pre-Merge Checklist

• All CI checks pass (tests, lint, verify, e2e)
• Test coverage maintained or improved (investigate handler_test.go reduction)
• go test -race ./pkg/activator/... passes
• Tilt development environment tested
• Documentation updated (CLAUDE.md, README)
• Consider backward compatibility for ErrorQuick502 rename
• Verify go.sum is committed
• Clean room build test (delete module cache and rebuild)

---

🎯 Final Recommendation

CONDITIONAL APPROVAL - This is fundamentally good work, but address these before merging:

1. Must Fix: Clarify test coverage reduction in handler_test.go
2. Must Fix: Add overflow check to breaker.go or document why existing constants prevent it
3. Should Fix: Restore backward compatibility for ErrorQuick502 type
4. Should Fix: Remove redundant errorsIsDeadlineExceeded() helper
5. Should Do: Add "Vendored Config Files" section to CLAUDE.md
6. Should Do: Verify all tests pass with -race flag

The architectural direction (removing vendor, modern Go modules) is excellent. The lint fixes demonstrate good attention to code quality. The config vendoring solution is creative. Just need to tighten up a few loose ends before this is production-ready.

Great work overall! This modernizes the codebase significantly. 🚀