fix(pkg/app/eventwatcher): log temporary repository cleanup errors - #7325
fix(pkg/app/eventwatcher): log temporary repository cleanup errors#7325anubhavsingh2106 wants to merge 1 commit into
Conversation
✅ Deploy Preview for pipecd-site canceled.
|
There was a problem hiding this comment.
🟢 Approval recommended
The change is small, consistent across both implementations, and correctly adds error-aware cleanup logging without altering core eventwatcher behavior.
Pull request overview
This PR improves the reliability and debuggability of PipeCD’s eventwatcher temporary-repository lifecycle by no longer suppressing cleanup errors and by logging cleanup failures with the affected repository path (in both legacy piped and pipedv1 implementations).
Changes:
- Replaced
// nolint: errcheck+defer tmpRepo.Clean()with a deferred cleanup function that checks and logstmpRepo.Clean()errors. - Included the temporary repository path in the error log fields to aid diagnosis.
- Applied the same cleanup-error logging pattern to both eventwatcher implementations.
File summaries
| File | Description |
|---|---|
| pkg/app/piped/eventwatcher/eventwatcher.go | Logs temporary repo cleanup failures instead of suppressing cleanup errors. |
| pkg/app/pipedv1/eventwatcher/eventwatcher.go | Mirrors the same deferred cleanup error handling/logging in the pipedv1 eventwatcher. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
d5bbd57 to
72d4b37
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The current cleanup logic can still leak temporary directories on CopyToModify failures and uses os.Remove (which can fail for non-empty dirs), so the cleanup behavior should be tightened before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
72d4b37 to
e76838e
Compare
There was a problem hiding this comment.
🟡 Changes recommended
The updated cleanup logic no longer calls tmpRepo.Clean(), so it doesn’t actually handle/log tmpRepo.Clean() errors as stated in the PR description and may bypass repo-specific cleanup semantics.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
Signed-off-by: Anubhav Singh <anmolkfzd@gmail.com>
e76838e to
6f24b68
Compare
There was a problem hiding this comment.
🟡 Changes recommended
Cleanup behavior and PR description are inconsistent (tmpDir removal occurs even when repo cleanup fails), and the still-invoked updateValues() path retains the old error-suppressing/leaky cleanup behavior.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (2)
pkg/app/pipedv1/eventwatcher/eventwatcher.go:335
- The deferred cleanup always calls os.RemoveAll(tmpDir) even when tmpRepo.Clean() fails. This contradicts the PR description (“remove tmpDir after successful repository cleanup”) and can delete the leftover repo/dir that would help diagnose why Clean failed.
if err := os.RemoveAll(tmpDir); err != nil {
pkg/app/piped/eventwatcher/eventwatcher.go:344
- The deferred cleanup always calls os.RemoveAll(tmpDir) even when tmpRepo.Clean() fails. This contradicts the PR description (“remove tmpDir after successful repository cleanup”) and can delete the leftover repo/dir that would help diagnose why Clean failed.
if err := os.RemoveAll(tmpDir); err != nil {
- Files reviewed: 2/2 changed files
- Comments generated: 2
- Review effort level: Lite
| var tmpRepo git.Repo | ||
| defer func() { | ||
| if tmpRepo != nil { | ||
| if err := tmpRepo.Clean(); err != nil { |
| var tmpRepo git.Repo | ||
| defer func() { | ||
| if tmpRepo != nil { | ||
| if err := tmpRepo.Clean(); err != nil { |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7325 +/- ##
==========================================
+ Coverage 29.75% 37.76% +8.01%
==========================================
Files 601 74 -527
Lines 64442 6511 -57931
==========================================
- Hits 19172 2459 -16713
+ Misses 43778 3919 -39859
+ Partials 1492 133 -1359
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Summary
tmpRepo.Clean()instead of suppressing them withnolint: errcheck.tmpDir) after successful repository cleanup to prevent empty temporary directories from accumulating during the watcher's lifetime.Files Changed
pkg/app/piped/eventwatcher/eventwatcher.gopkg/app/pipedv1/eventwatcher/eventwatcher.goTesting
go test ./pkg/app/piped/eventwatcher ./pkg/app/pipedv1/eventwatcher— passed.git diff --check— passed.gofmt— applied with no unrelated formatting changes.Note
The eventwatcher tests initially failed on Windows because the testdata files had CRLF line endings while
TestModifyTextcompares the generated content against LF-based expected content. The files were temporarily normalized locally for test execution and restored afterward; no test or testdata changes are included in this PR.