Review comments from #21
## Summary
Refactors Git authentication resolution logic into a new reusable
`gitauth` package, eliminating code duplication across multiple
reconcilers. Also includes workspace management improvements and API
cleanup.
## Key Changes
### Git Auth Refactoring
- Created new `pkg/gitauth` package with `ResolveAuth()` function that
handles Kubernetes Secret-based Git authentication
- Removed duplicate `resolveAuth()` methods from three reconcilers:
- `pkg/reconciler/push/push.go`
- `pkg/reconciler/resolver/resolver.go`
- `pkg/reconciler/sync/sync.go`
- All reconcilers now use the centralized `gitauth.ResolveAuth()`
function
- Added comprehensive test coverage in `pkg/gitauth/gitauth_test.go`
covering:
- No auth configuration
- Valid secret resolution
- Missing username/password keys
- Secret not found scenarios
### Workspace Management Improvements
- Fixed `Release()` method to properly clean up active references from
the map when ref count reaches zero
- Improved `GC()` to skip directories with active in-process references,
preventing race conditions during ongoing reconciles
- Added tests for ref count cleanup and garbage collection behavior
### API Cleanup
- Removed unused `CacheStatus` type from `GitRepositoryStatus` in the
API
- Updated CRD schema to remove cache-related fields
- Cleaned up generated deepcopy code
### Minor Updates
- Updated push controller comment to clarify that full history is needed
(not shallow clones)
- Updated test imports to use the new `gitauth` package
## Implementation Details
- The `ResolveAuth()` function uses the Kubernetes dynamic client to
fetch Secrets
- Handles base64 decoding of Secret data fields (username and password)
- Returns `*http.BasicAuth` for use with go-git library
- Proper error handling with descriptive messages for missing keys or
secrets
https://claude.ai/code/session_01WJE6ozYXZMn1CD6d4vy8b6
- Protect r.count decrement and map deletion under a single m.mu lock
hold in Release() to eliminate race with GC()
- Keep GC's r.count > 0 check inside the m.mu critical section
- Replace hand-rolled contains/containsSubstring with strings.Contains
https://claude.ai/code/session_01WJE6ozYXZMn1CD6d4vy8b6
- Fix Release() ref counting bug: avoid calling getRef() which increments
count, instead look up ref directly and clean up when count reaches zero
- Remove unpopulated CacheStatus struct and fields from types, deepcopy,
and CRD manifest since nothing writes to them
- Extract duplicated resolveAuth into shared pkg/gitauth package with
explicit key validation for missing "username"/"password" keys
- Fix GC() race condition by skipping paths with active in-process refs
- Remove misleading WorkspaceCacheMiss metric increment for in-memory
(non-cache) code paths
- Change push controller from shallow to full clones to avoid silent
failures when transactions reference historical commits
- Add tests for ref count cleanup, ref leak prevention, GC active ref
skipping, and gitauth key validation
https://claude.ai/code/session_01WJE6ozYXZMn1CD6d4vy8b6
## Summary
This PR introduces a new `workspace` package that provides PVC-backed
caching for Git repositories with automatic fallback to in-memory
cloning. It enables controllers to optionally cache bare Git clones on
disk for improved performance while maintaining backward compatibility
with the existing in-memory-only behavior.
## Key Changes
- **New `workspace` package** (`pkg/workspace/workspace.go`):
- `Manager` type that handles both disk-backed and in-memory Git
repository acquisition
- `Acquire()` method that clones repos to disk (with SHA256-based path
hashing) or memory based on configuration
- `Release()` method for reference counting on disk-backed repos
- `GC()` method for garbage collecting stale cached repositories
- `Deepen()` method to convert shallow clones to full history when
needed
- Automatic fallback to in-memory if disk clone fails or caching is
disabled
- Context injection helpers (`WithManager`, `GetManager`) for dependency
injection
- **API extensions** (`pkg/apis/git/v1alpha1/types.go`):
- New `CacheConfig` struct with `Enabled` boolean field
- Added `Cache` field to `GitRepositorySpec` for opt-in caching per
repository
- Added `CacheStatus` struct to track cache health (clone/fetch times)
- Updated generated deepcopy code
- **Reconciler updates** (sync, resolver, push controllers):
- Integrated `workspace.Manager` into all three reconcilers
- Updated to use workspace manager for Git operations instead of direct
in-memory clones
- Added auth resolution from Kubernetes secrets
- Pass `cacheEnabled` flag based on repository configuration
- **Metrics enhancements** (`pkg/metrics/metrics.go`):
- `WorkspaceAcquireDuration` histogram tracking acquire time by mode
(disk/memory)
- `WorkspaceCacheHit` and `WorkspaceCacheMiss` counters for cache
effectiveness monitoring
- **Deployment configuration**:
- Added commented-out examples for enabling PVC-backed caching via
`GIT_CACHE_DIR` environment variable and volume mounts in all three
controller deployments
- Updated CRD schema to include cache configuration
- **Comprehensive test coverage** (`pkg/workspace/workspace_test.go`):
- 426 lines of unit tests covering all Manager functionality
- Tests for cache hits/misses, shallow clones, GC, context injection,
and error handling
- **E2E tests** (`test/e2e/workspace_test.go`):
- Tests verifying push transactions work with and without caching
- Tests for cache serialization and multiple sequential operations
## Implementation Details
- **Backward compatible**: Empty `basePath` or `cacheEnabled=false`
triggers in-memory fallback
- **Per-repo opt-in**: Caching is disabled by default; must be
explicitly enabled via `Cache.Enabled` in GitRepository spec
- **Graceful degradation**: If disk clone fails, automatically falls
back to in-memory without failing the operation
- **Reference counting**: Disk-backed workspaces use mutex-protected
reference counting to track active uses
- **Deterministic paths**: Repository cache paths are derived from
SHA256 hash of the URL for consistency
- **Shallow clone support**: Initial clones can use `--depth=1` when
`shallow=true`, with `Deepen()` to fetch full history later
https://claude.ai/code/session_01NULGxaCLPMT1yDVEAsffc5
Add pkg/workspace.Manager that provides Acquire/Release semantics for Git
repos. When GIT_CACHE_DIR is set and a GitRepository has spec.cache.enabled,
controllers use on-disk bare clones with incremental fetch; otherwise they
fall back to the existing in-memory clone behavior.
Key changes:
- New pkg/workspace package with Manager, Workspace, GC, Deepen, and
context-based injection (WithManager/GetManager)
- GitRepository API gains spec.cache (CacheConfig) and status.cache
(CacheStatus) with deepcopy and CRD schema updates
- Push, sync, and resolver reconcilers now use workspace.Acquire/Release
instead of inline memory.NewStorage + git.CloneContext
- Shallow clone by default; sync/resolver deepen as needed for merge-base
- New Prometheus metrics: workspace_acquire_duration_seconds,
workspace_cache_hit_total, workspace_cache_miss_total
- Deployment manifests include commented-out PVC configuration
- Comprehensive unit tests for workspace package (21 tests)
- E2E tests for cache-free and cache-enabled push flows
https://claude.ai/code/session_01NULGxaCLPMT1yDVEAsffc5
## Summary
- Prioritized roadmap in 4 tiers:
- **Tier 1**: Metrics, health checks, unit tests, Git timeouts
(production blockers)
- **Tier 2**: Exponential backoff, SSH auth, transaction GC, label
selectors
- **Tier 3**: JSON logging, NetworkPolicy, CRD validation, configurable
merge author
- **Tier 4**: Shallow clones, HPA, poll jitter, webhook-driven sync
- Production readiness checklist
## Notable Details
- The assessment identifies two production blockers: missing Prometheus
metrics and missing health check endpoints
- Provides specific code locations for each issue (e.g.,
`pkg/reconciler/push/push.go:139-172` for SSH key support)
- Includes a security assessment matrix showing which areas are
production-ready vs. missing
- Offers concrete implementation guidance (e.g., "Use Knative's built-in
metrics support or `prometheus/client_golang`")
- Acknowledges existing strengths (distroless images, non-root
containers, proper RBAC, fresh dependencies) while highlighting gaps
This document serves as a reference for contributors and operators to
understand the project's current state and prioritize improvements
toward production deployment.
I deleted ASSSESSMENT.md after Claude fixed the findings. I also added a
design for PVC-backed git workspaces, which we'll implement next.
https://claude.ai/code/session_01PaXbaSqhVEqj97kpY4v6rt
Describes a pkg/workspace.Manager that replaces per-reconcile full clones
with persistent bare repos on a PVC, falling back to in-memory when no
PVC is mounted. Covers API changes (spec.cache on GitRepository),
deployment changes (optional PVC per controller), concurrency/locking,
cache lifecycle, and migration path.
https://claude.ai/code/session_01PaXbaSqhVEqj97kpY4v6rt
Critical production-readiness improvements:
- Prometheus metrics: reconcile count/latency per controller, Git operation
duration (clone/push/ls-remote) via pkg/metrics with /metrics on :9090
- Health probes: leverage Knative's built-in health server on :8080, add
readiness/liveness probes to all four deployment manifests
- Git operation timeouts: 5-minute context deadline on all git.CloneContext
and PushContext calls to prevent indefinite blocking on large repos
- Unit tests: new ReconcileKind-level tests for push (pending→failed on
missing repo, auth error paths), sync (branch-not-found, LastSyncTime),
resolver (missing repo, partial hashes, empty phase), and repowatcher
(ls-remote errors, empty remote, unchanged branches)
https://claude.ai/code/session_01PaXbaSqhVEqj97kpY4v6rt
Comprehensive review of reliability, scalability, security, and performance.
Key findings: strong architecture and security fundamentals, but missing
metrics, health checks, unit tests, and Git operation timeouts for production.
https://claude.ai/code/session_01PaXbaSqhVEqj97kpY4v6rt
## Summary
This PR significantly expands test coverage across the git-k8s
reconcilers and client packages. The changes add integration-style tests
using fake Kubernetes clients to verify the core business logic of push
transactions, two-way repo sync, conflict resolution, remote polling,
and the git client wrapper.
## Key Changes
- **Push Reconciler Tests** (`pkg/reconciler/push/push_test.go`):
- Added `TestReconcileKind_SkipsTerminalPhase` to verify terminal phases
are skipped
- Added `TestFailTransaction` to test transaction failure handling
- Added `TestUpdateBranches` to verify branch status updates after
successful pushes
- Added `TestUpdateBranches_NoMatchingBranch` to test branch filtering
- **Sync Reconciler Tests** (`pkg/reconciler/sync/sync_test.go`):
- Added `TestFindBranch_Found`, `TestFindBranch_NotFound`,
`TestFindBranch_WrongRepo` to test branch lookup
- Added `TestUpdateSyncStatus` with multiple phase transitions (InSync,
Syncing, Conflicted, ManualIntervention)
- Added `TestCreatePushTransaction` to verify transaction creation with
proper owner references
- **Resolver Reconciler Tests**
(`pkg/reconciler/resolver/resolver_test.go`):
- Added `TestReconcileKind_SkipsNonConflicted` to verify non-conflicted
syncs are skipped
- Added `TestReconcileKind_MissingCommitHashes` to test fallback to
manual intervention
- Added `TestMarkManualIntervention` to verify manual intervention state
transitions
- **RepoWatcher Reconciler Tests**
(`pkg/reconciler/repowatcher/reconciler_test.go`):
- Added `TestEnqueueAfter_NilImpl` to verify nil impl doesn't panic
- Added `TestReconcileKind_CreatesNewBranches` to test branch creation
from remote refs
- Added `TestReconcileKind_UpdatesExistingBranch` to test branch status
updates
- Added `TestReconcileKind_DeletesStaleBranch` to test cleanup of
deleted remote branches
- **Git Client Tests** (`pkg/client/clientset_test.go`):
- Added `TestNewFromDynamic` to verify client initialization
- Added `TestGitRepositories_CRUD` for full CRUD operations on
GitRepository
- Added `TestGitBranches_CRUD` for full CRUD operations on GitBranch
- Added `TestGitPushTransactions_CRUD` for full CRUD operations on
GitPushTransaction
- Added `TestGitRepoSyncs_CRUD` for full CRUD operations on GitRepoSync
- **Internal Reconciler Tests**
(`pkg/reconciler/internal/reconcile_test.go`):
- Added comprehensive tests for the generic reconciler framework
- Tests cover success paths, not-found handling, get errors, and
reconcile errors
- **API Defaults Tests** (`pkg/apis/git/v1alpha1/defaults_test.go`):
- Added `TestRegisterDefaults` to verify scheme registration
- **Test Infrastructure**:
- Added reusable `newFakeScheme()` helper across multiple test files
- Added `newFakeReconciler()` factory functions for consistent test
setup
- Added `gvrForKind()` mapping functions for GroupVersionResource
lookups
- Configured fake dynamic clients with custom list kinds for proper API
behavior
## Implementation Details
- Tests use `fakedynamic.NewSimpleDynamicClientWithCustomListKinds` to
simulate Kubernetes API behavior
- Each test file includes helper functions to create properly configured
fake reconcilers
- Tests verify both happy paths and error conditions
- Owner references and labels are validated to ensure proper resource
relationships
- Status updates are verified through the fake client to ensure
persistence
- Added a differential coverage report documenting baseline coverage and
feature-to-test mapping
https://claude.ai/code/session_016Yeyfoocn7SN2HuMsxRD6f
The compile-workflows job was firing on both `pull_request` and `push`
to `main`, but compiling `.md` workflows on merge is redundant — the PR
run already commits the lock files back to the branch.
## Changes
- **Removed `push` trigger** — workflow now only runs on `pull_request`
events targeting `main`
- **Simplified checkout** — two conditional checkout steps (push vs PR)
collapsed into a single step using `github.head_ref`
- **Simplified `if:` guard** — no longer needs to check `event_name`,
just `head.repo.fork == false`
<!-- START COPILOT CODING AGENT TIPS -->
---
💬 We'd love your input! Share your thoughts on Copilot coding agent in
our [2 minute survey](https://gh.io/copilot-coding-agent-survey).
The workflows: write permission in the YAML permissions block is not
valid for GITHUB_TOKEN, which prevented the workflow from starting.
Pushing files under .github/workflows/ requires a PAT with the
workflows scope. The workflow now uses a GH_PAT secret (falling back
to GITHUB_TOKEN) for checkout and push. To set this up:
Create a fine-grained PAT with Contents + Workflows read/write,
then: gh secret set GH_PAT --body "<your-pat>"
https://claude.ai/code/session_01JTYNSeJrGzdW6wfAUbdjjw
The GITHUB_TOKEN needs the workflows permission to push changes to
files under .github/workflows/. Also add || true to git add glob
in case no lock files exist yet on first compile.
https://claude.ai/code/session_01JTYNSeJrGzdW6wfAUbdjjw
Instead of just failing when lock files are outdated, the workflow now
compiles the agentic workflow markdown files and commits+pushes the
resulting lock files back. On PRs, checks out the head branch directly
(not the merge ref) so it can push back to the PR.
https://claude.ai/code/session_01JTYNSeJrGzdW6wfAUbdjjw
Lockdown mode requires a custom PAT (GH_AW_GITHUB_TOKEN) which isn't
configured. The toolsets restriction already limits available GitHub
API tools to pull_requests and repos, so lockdown isn't needed.
https://claude.ai/code/session_01JTYNSeJrGzdW6wfAUbdjjw
Runs on both PRs and pushes to main. Compiles all agentic workflow
markdown files and fails if the resulting lock files differ from what's
committed. This ensures contributors run `gh aw compile` before merging.
https://claude.ai/code/session_01JTYNSeJrGzdW6wfAUbdjjw
Triggers on pushes to main that touch .github/workflows/*.md files.
Installs gh-aw, compiles all agentic workflow markdown files, and
commits the resulting .lock.yml files back to the repo.
https://claude.ai/code/session_01JTYNSeJrGzdW6wfAUbdjjw
- Add engine: claude to all agentic workflows (uses ANTHROPIC_API_KEY)
- Fix dependency-update.md network field: use object format instead of
array to match the oneOf schema (string | object)
- Remove pr-fix.md since ci-doctor.md already handles CI failure
investigation and auto-fixing
https://claude.ai/code/session_01JTYNSeJrGzdW6wfAUbdjjw
Auto-merge workflow was using squash merges; change it to use standard
merge commits.
- **`auto-merge.yaml`**: Set `merge_method: 'merge'` (was `'squash'`)
<!-- START COPILOT CODING AGENT TIPS -->
---
💬 We'd love your input! Share your thoughts on Copilot coding agent in
our [2 minute survey](https://gh.io/copilot-coding-agent-survey).
Set up five workflows to automate repository maintenance:
- code-review.md: Automated PR review on open/sync, checks for bugs,
security issues, and Go/K8s anti-patterns. Auto-fixes formatting and
linting. Escalates to maintainer only for CRD/architecture changes.
- ci-doctor.md: Triggers on CI failure, investigates root cause from logs,
auto-fixes mechanical issues (mod tidy, fmt, simple compilation errors),
and creates detailed issues for complex failures.
- pr-fix.md: On-demand /pr-fix command to analyze and fix failing CI checks
on any PR.
- dependency-update.md: Weekly scheduled check for Go module updates.
Groups updates logically (k8s, knative, go-git), fixes breaking changes,
and creates per-batch PRs. Escalates only when fixes need design decisions.
- auto-merge.yaml: Standard GitHub Actions workflow that auto-merges PRs
labeled "automation" once all CI checks pass (squash merge).
All agentic workflows assign @imjasonh only when human judgment is needed,
with specific questions rather than generic notifications.
https://claude.ai/code/session_01JTYNSeJrGzdW6wfAUbdjjw
The repo-watcher's delete loop was unconditionally removing any
GitBranch whose branch name didn't appear on the remote. This caused
a race with the push controller: when a GitBranch is created for a
branch that doesn't yet exist (e.g., before a push transaction),
the watcher would delete it before the push could complete.
Fix: only delete GitBranch CRDs that the repo-watcher itself created,
identified by having an owner reference pointing to the GitRepository.
Branches created manually or by other controllers are left alone.
This fixes the flaky TestPushTransaction e2e test, where the watcher
would delete "e2e-push-feature" before the push controller could
update its headCommit.
https://claude.ai/code/session_01P7xfBqARU5DFS8QJ4uDPVj
Tests cover the key behaviors of the repo-watcher:
- Branch discovery: auto-creates GitBranch CRDs from remote refs
- Owner references: created branches correctly reference GitRepository
- New branch detection: branches added on Gitea are picked up
- Commit update detection: new pushes to Gitea update headCommit
- Branch deletion: branches removed from Gitea trigger CRD deletion
- Multiple branches: all branches on a repo are discovered
- LastFetchTime: status is updated and advances on each poll
- Labels: repository label is set on created branches
- Slash branch names: feature/foo naming handled correctly
Also adds shared e2e helpers for Gitea branch/file operations and
updates CI workflow to deploy and monitor the repo-watcher-controller.
https://claude.ai/code/session_01P7xfBqARU5DFS8QJ4uDPVj
Introduces a fourth controller that watches GitRepository resources and
polls their remotes via git ls-remote on a configurable per-repo interval
(spec.pollInterval, defaulting to 30s). It auto-creates, updates, and
deletes GitBranch CRDs to reflect the actual state of the remote,
enabling fast detection of external pushes to trigger downstream sync.
Key changes:
- Add PollInterval field to GitRepositorySpec
- New pkg/reconciler/repowatcher with controller and reconciler
- New cmd/repo-watcher-controller binary
- CRD manifest updated for pollInterval
- Deployment manifest for the new controller
- Unit tests for ref filtering, naming, and poll interval logic
- CLAUDE.md updated with new controller docs and Go module workaround
https://claude.ai/code/session_01P7xfBqARU5DFS8QJ4uDPVj
Document the project architecture (three-controller design, CRDs),
directory structure, build commands, testing workflows, key dependencies,
and development conventions for AI assistants working on this codebase.
https://claude.ai/code/session_016QCVChhEZLuYZbhcfxyCzw
Move the key-splitting, resource-fetching, and not-found handling into a
generic internal.NewReconciler wrapper so that each reconciler only
implements ReconcileKind(ctx, *TypedResource) — matching the Knative
generated reconciler convention. The controller setup wraps the
reconciler with internal.NewReconciler, which bridges Reconcile(ctx, key)
to ReconcileKind(ctx, *T).
This removes duplicated boilerplate from all three reconcilers (push,
sync, resolver), eliminates their Reconcile/LeaderAware methods, and
makes splitKey an unexported implementation detail.
https://claude.ai/code/session_01Eu3LyxX3G1KCcw4aDzmHvp
