-
Notifications
You must be signed in to change notification settings - Fork 31
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
🐛 unpack cache cleanup should ignore missing files #404
🐛 unpack cache cleanup should ignore missing files #404
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #404 +/- ##
==========================================
- Coverage 37.89% 37.45% -0.45%
==========================================
Files 15 15
Lines 789 793 +4
==========================================
- Hits 299 297 -2
- Misses 446 449 +3
- Partials 44 47 +3 ☔ View full report in Codecov by Sentry. |
936ee89
to
46b0dca
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@joelanford fyi incorporated this in #379 to move that faster
if err := deleteRecursive(i.catalogPath(catalog.Name)); err != nil { | ||
return fmt.Errorf("error deleting catalog cache: %w", err) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
For now at least, deleteRecursive
already has a wrapper text:
error making catalog cache writable for deletion:
.
Do we need this?
Alternatively, if we want the nice to have error deleting catalog cache: error making catalog cache writable for deletion:..
wrappers, the next line should return nil (it's recalling deleteRecursive
again)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
+1 to the below line needing a change.
I'm not strongly opinionated on the wrapping, but generally I always try to add more context where possible to help with "tracing" the actions taken from error logs
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Oh, this is leftover from me manually testing idempotence. I'll remove.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
leaving a "change request" to prevent it from accidentally getting in.
@anik120 I want to merge this as a separate fix since it is generally unrelated to your PR. Updating now to address comments. |
46b0dca
to
11de935
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
/lgtm
Signed-off-by: Joe Lanford <joe.lanford@gmail.com>
11de935
to
c01f5ee
Compare
New changes are detected. LGTM label has been removed. |
We want to merge this separate from the PR that found the bug.
8c6625f
No description provided.