Move shared git package from operator to pkg/git#4045
Conversation
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
3e82856 to
13536fb
Compare
|
✅ PR size has been reduced below the XL threshold. The size review has been dismissed and this PR can now proceed with normal review. Thank you for splitting this up! |
PR size has been reduced below the XL threshold. Thank you for splitting this up!
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #4045 +/- ##
==========================================
+ Coverage 68.62% 68.69% +0.06%
==========================================
Files 445 446 +1
Lines 45374 45396 +22
==========================================
+ Hits 31140 31186 +46
+ Misses 11827 11803 -24
Partials 2407 2407 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
The operator's git package (CloneConfig, Client interface, LimitedFs, DefaultGitClient) is needed by both the operator and the CLI for git-based skill installation. Move it to pkg/git/ so both can share it. Changes during move: - Replace controller-runtime/pkg/log with log/slog - Add WithAuth ClientOption for authenticated clones - Remove runtime.GC() call from Cleanup (unnecessary; Go GC handles it) - Update tests to use t.Context() instead of controller-runtime logger - Rewrite tests with testify assertions and t.TempDir() - Drop trivial field-accessibility tests and slow network-dependent tests No functional changes to the operator — the package was not yet imported by any operator code. Relates to #4015 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
13536fb to
efa601e
Compare
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
aponcedeleonch
left a comment
There was a problem hiding this comment.
Minor nits — nothing blocking, just small improvements to consider.
- Use errors.New for sentinel errors in fs.go - Add ErrNilRepository sentinel replacing duplicate fmt.Errorf strings - Add path traversal validation in GetFileContent - Add CloneConfig.validate() rejecting conflicting Branch/Tag/Commit - Log util.RemoveAll failures in Cleanup with slog.Warn - Add tag-based clone integration test - Add path traversal and validation unit tests - Improve integration test comments Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
There was a problem hiding this comment.
Large PR Detected
This PR exceeds 1000 lines of changes and requires justification before it can be reviewed.
How to unblock this PR:
Add a section to your PR description with the following format:
## Large PR Justification
[Explain why this PR must be large, such as:]
- Generated code that cannot be split
- Large refactoring that must be atomic
- Multiple related changes that would break if separated
- Migration or data transformationAlternative:
Consider splitting this PR into smaller, focused changes (< 1000 lines each) for easier review and reduced risk.
See our Contributing Guidelines for more details.
This review will be automatically dismissed once you add the justification section.
Summary
cmd/thv-operator/pkg/git/) is needed by the CLI for upcoming git-based skill installation. Moving it topkg/git/makes it a shared package available to both the operator and CLI.controller-runtime/pkg/logwithlog/slogso the package works outside the operator's controller-runtime context.t.TempDir()for isolation, dropping trivial field-accessibility tests and slow network-dependent tests in favor of deterministic local-repo integration tests.Relates to #4015
Type of change
Test plan
task test)task lint-fix)Changes
cmd/thv-operator/pkg/git/*pkg/git/pkg/git/client.gocontroller-runtime/pkg/logforlog/slog, added path validation, sentinel errors, cleanup loggingpkg/git/fs.goerrors.Newpkg/git/types.govalidate()for CloneConfig mutual exclusionpkg/git/doc.gopkg/git/client_test.got.TempDir(), added path traversal and validation testspkg/git/integration_test.goDoes this introduce a user-facing change?
No
Large PR Justification
cmd/thv-operator/pkg/git/and recreates 331 lines inpkg/git/. The deletions account for the majority of the diff size. Splitting the delete and create into separate PRs would leave the codebase in a broken state.controller-runtime/pkg/logwhich is no longer available in the new location, so they had to be rewritten in the same PR.Special notes for reviewers
Operator test files intentionally dropped during move
commit_test.go: Cloned from realgithub.com/stacklok/toolhiveover the network — slow, fragile, skipped in-shortmode. Same scenario covered by local-repo integration tests.types_test.go: Trivial field-accessibility tests ("set URL, check URL is set") that add no meaningful coverage per the project's test quality guidelines.Security hardening added during move
GetFileContent(rejects.., absolute paths, null bytes)CloneConfig.validate()enforces mutual exclusion of Branch/Tag/CommitErrNilRepository,ErrInvalidFilePath,ErrInvalidCloneConfig) for propererrors.Is()checkingThis is PR 1 of 2 — split from #4042. PR 2 (git-based skill install feature) builds on this.
Generated with Claude Code