chore: updates CI runners and handles failing Windows test#101
chore: updates CI runners and handles failing Windows test#101rosygmiki wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Code Review
This pull request fixes a bug in pkg/tar-patch/apply.go where an error encountered while closing the current file was being ignored and nil was returned instead of the actual error. The code has been corrected to return the err. There are no review comments to address.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #101 +/- ##
==========================================
+ Coverage 81.43% 81.61% +0.17%
==========================================
Files 10 10
Lines 1115 1115
==========================================
+ Hits 908 910 +2
+ Misses 117 116 -1
+ Partials 90 89 -1 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
b3fbf61 to
27f0279
Compare
cdbe4e8 to
1b77c2a
Compare
Signed-off-by: Rosy-Glorious Miki <rmiki@redhat.com>
…ion for broader compatibility Signed-off-by: Rosy-Glorious Miki <rmiki@redhat.com>
4ce93ec to
2fe3ca6
Compare
Signed-off-by: Rosy-Glorious Miki <rmiki@redhat.com>
2fe3ca6 to
9810239
Compare
djach7
left a comment
There was a problem hiding this comment.
A couple of change requests, thanks for working on this!
|
|
||
| // SetCurrentFile opens the specified file for reading. | ||
| func (f *FilesystemDataSource) SetCurrentFile(file string) error { | ||
| if f.currentFile != nil { |
There was a problem hiding this comment.
Possible change, feel free to ignore if you think this is out of scope for the PR.
I think the change to err here is good, but looking at this harder there's a potential leak in the following scenario:
- the close fails on line 68
- we set currentFile to nil on line 69
- we return the err on line 71 but we now have a file that's still open without a way to reference it because we set the reference to
nil
I asked Claude about it and it fired off this suggested fix:
if f.currentFile != nil {
err := f.currentFile.Close()
if err != nil {
// Don't nil out currentFile if close fails
return fmt.Errorf("failed to close current file: %w", err)
}
f.currentFile = nil
}
currentFile, err := os.Open(filepath.Join(f.basePath, file))
if err != nil {
return err
}
f.currentFile = currentFile
return nil
}
Again, I know you didn't make any changes here other than the nil -> err one so if you want to kick this to a later PR or issue that's fine. I just spotted it and wanted to call it out
| # tar-patch: valid delta, missing source file | ||
| # Force bsdiff-sized payload + -max-bsdiff-size so the delta emits OPEN for data/only.txt; | ||
| # otherwise copyRest-only deltas can apply without that file and expect_fail would flake. | ||
| # Skips on Windows platform |
There was a problem hiding this comment.
I think it would be beneficial to add a little more of a "why" to this comment, just a line or two explaining why we're skipping windows for now
This PR temporarily skips an issue where Windows builds fails when source files are missing. It also cleans up our CI workflow by removing unsupported platforms.
Tests
niltoerrand adds a new test function for the added error path to satisfy Codecov patch % requirement.Chores
Closes #84