-
Notifications
You must be signed in to change notification settings - Fork 173
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
fix: avoid errors when --max-package-size
differs between create runs
#3398
Conversation
Signed-off-by: Austin Abro <[email protected]>
✅ Deploy Preview for zarf-docs canceled.
|
Signed-off-by: Austin Abro <[email protected]>
Codecov ReportAttention: Patch coverage is
|
--max-package-size
differs between runs--max-package-size
differs between create runs
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
Signed-off-by: Austin Abro <[email protected]>
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
func TestSplitFile(t *testing.T) { | ||
t.Parallel() | ||
|
||
tests := []struct { |
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.
Great test cases 👍
t.Run(tt.name, func(t *testing.T) { | ||
t.Parallel() | ||
|
||
dir := t.TempDir() |
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.
Nonblocking: this is more of a style thing, but usually I add some small intent comment (example below) on a series of logically similar operations. There's tradeoffs here with just making self-documenting code, so I'm curious where your opinions are at there.
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.
I think it's a reasonable thing to do. I try to break up my tests with indentation and the arrange-act-assert methodology. I don't think comments are always necessary, but when code is non intuitive I do think it helps to provide additional context.
In this case, this test case was actually from packager1 so I just copied it over, going forward I'll try to air on the side of more clarity.
err = splitFile(context.Background(), p, tt.chunkSize) | ||
require.NoError(t, err) | ||
|
||
_, err = os.Stat(p) |
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.
e.g.
// Ensure content exists
data := make([]byte, 50) | ||
err := os.WriteFile(inputFilename, data, 0644) | ||
require.NoError(t, err) | ||
// Create many fake split files |
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.
👍
Description
As seen explained in #3266 the existing split file logic doesn't replace files it writes over leading to errors with create is run with different
--max-package-size
values. Additionally, if the package is split and there are more files than Zarf is expecting (likely leftovers from a previous run) Zarf will give the below errorRelated Issue
Fixes #3266
Checklist before merging