Code Review

Code Review

tekton-pruner follows Tekton community review standards.

Review Checklist

Correctness

• Logic changes have corresponding unit tests in the relevant _test.go file
• New ConfigMap fields are covered in pkg/config/config_validation_test.go and config_validation_hierarchical_test.go
• Reconciler changes do not break idempotency (re-running reconcile must be safe)
• TTL and history-limit logic in pkg/config/ttl_handler.go and history_limiter.go correctly handles zero-value, negative, and missing fields
• No data races — verify with make test-unit (race detector is on by default)

Go Quality

• make fmt produces no diff
• go vet ./... is clean
• No use of init() for non-trivial side effects
• Errors are wrapped with fmt.Errorf("...: %w", err) — not swallowed
• Contexts are propagated, not ignored
• No direct use of os.Exit outside main

Kubernetes / Operator Patterns

• RBAC changes in config/200-clusterrole.yaml / config/200-role.yaml are least-privilege — add only the verbs actually required
• New CRDs or config resources include corresponding YAML in config/
• Informers and listers are used for reads; direct API calls for writes
• Reconcile loops return controller.NewPermanentError only for truly unrecoverable conditions
• Webhook validation in pkg/webhook/configmapvalidation.go rejects invalid pruner specs early, with clear error messages

Domain: Pruner Logic

• ConfigMap selector logic (pkg/config/config.go, pkg/config/helper.go) correctly resolves namespace-level overrides vs cluster-level defaults
• pkg/config/constants.go is the single source of truth for annotation and label keys — no hardcoded strings elsewhere
• History and TTL limits are validated against each other where both are set
• The pruner reconciler correctly handles PipelineRun and TaskRun independently; shared logic belongs in pkg/config/helper.go

Observability

• New controller actions emit metrics via pkg/metrics/
• Log statements use structured logging (zap) with appropriate levels (Info for normal operations, Error for failures, Debug for verbose paths)

Documentation

• Public functions and types have Go doc comments
• User-facing ConfigMap fields are documented in docs/ (especially docs/tutorials/ and docs/configmap-validation.md)
• ARCHITECTURE.md is updated if a new component or major design change is introduced

What to Approve

Approve when:

• All checklist items pass
• Tests cover the changed behavior
• The change is focused (one concern per PR)
• Commit messages follow Tekton commit conventions

What to Block

Block (request changes) when:

• Logic is untested or tests are trivially green
• RBAC grants broad wildcard verbs
• Errors are silently swallowed in reconcile loops
• ConfigMap validation can be bypassed