Skip to content
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.

Sign up for GitHub

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

Jump to bottom

chunked: fix generating footer #1697

Merged
merged 2 commits into from Sep 5, 2023
Merged

chunked: fix generating footer #1697

merged 2 commits into from Sep 5, 2023

Conversation

giuseppe
Copy link
Member

commit 7bbf6ed introduces an error where the generated footer is not correct.

Signed-off-by: Giuseppe Scrivano gscrivan@redhat.com

@giuseppe giuseppe mentioned this pull request Aug 30, 2023
Merged
@giuseppe giuseppe force-pushed the refactor-zstd-footer-handling branch from a06a5e2 to fd38c06 Compare August 30, 2023 14:49
mtrmac
mtrmac reviewed Aug 30, 2023
View reviewed changes
pkg/chunked/internal/compression.go
}

// ReadFooterDataFromAnnotations reads the zstd:chunked footer data from the given annotations.
func ReadFooterDataFromAnnotations(annotations map[string]string) (ZstdChunkedFooterData, error) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

(Not a review yet:) pkg/chunked/compression and pkg/chunked/internal are included in ~every single c/image build, so there would be some value in not adding code that isn’t necessary on that path to these subpackages.

OTOH keeping the read and write paths together helps maintenance…

@giuseppe giuseppe force-pushed the refactor-zstd-footer-handling branch from fd38c06 to e96dd0e Compare August 30, 2023 15:02
mtrmac
mtrmac reviewed Aug 31, 2023
View reviewed changes
Copy link
Collaborator

@mtrmac mtrmac left a comment •
edited

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for following up!

Overall LGTM — please add a EDIT test that FooterSizeSupported matches, the rest are tentative non-blocking aesthetic matters.

pkg/chunked/internal/compression.go Outdated Show resolved Hide resolved
pkg/chunked/internal/compression.go Outdated Show resolved Hide resolved
pkg/chunked/internal/compression.go Outdated Show resolved Hide resolved
pkg/chunked/internal/compression_test.go Outdated Show resolved Hide resolved
pkg/chunked/internal/compression.go Show resolved Hide resolved
pkg/chunked/internal/compression.go Show resolved Hide resolved
pkg/chunked/internal/compression_test.go Show resolved Hide resolved
pkg/chunked/internal/compression.go Outdated Show resolved Hide resolved
@giuseppe giuseppe force-pushed the refactor-zstd-footer-handling branch from e96dd0e to ec00686 Compare August 31, 2023 21:49
@giuseppe
Copy link
Member Author

pushed a new version

@giuseppe giuseppe force-pushed the refactor-zstd-footer-handling branch 2 times, most recently from c3ec176 to 68ed29f Compare September 1, 2023 08:56
@giuseppe
Copy link
Member Author

giuseppe commented Sep 4, 2023

this is ready to merge

@mtrmac
Copy link
Collaborator

mtrmac commented Sep 4, 2023

One more question I should have thought of immediately: If we are changing the footer format, why not change the magic number as well?

@giuseppe
Copy link
Member Author

giuseppe commented Sep 4, 2023

One more question I should have thought of immediately: If we are changing the footer format, why not change the magic number as well?

the format is not really stable yet, we can change the magic number but I am not sure there is any gain in that.

@giuseppe
Copy link
Member Author

giuseppe commented Sep 4, 2023

One more question I should have thought of immediately: If we are changing the footer format, why not change the magic number as well?

the format is not really stable yet, we can change the magic number but I am not sure there is any gain in that.

another point, current images would work fine if they are pulled from a registry since this information is stored in the annotations. If we change the magic number we will just break them

@mtrmac
Copy link
Collaborator

mtrmac commented Sep 5, 2023

the format is not really stable yet, we can change the magic number but I am not sure there is any gain in that.

Fair enough.

another point, current images would work fine if they are pulled from a registry since this information is stored in the annotations. If we change the magic number we will just break them

Well, not for the users (like Podman) who read the annotations, I think?

Either way, I’ll leave it entirely up to you how stable the format should be right now.

@giuseppe
chunked: refactor code in new functions
eedd976 
Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe
chunked: fix generating footer
f989a95 
commit 7bbf6ed introduces an error
where the generated footer is not correct.

Signed-off-by: Giuseppe Scrivano <gscrivan@redhat.com>
@giuseppe giuseppe force-pushed the refactor-zstd-footer-handling branch from 68ed29f to f989a95 Compare September 5, 2023 06:55
@giuseppe
Copy link
Member Author

giuseppe commented Sep 5, 2023

another point, current images would work fine if they are pulled from a registry since this information is stored in the annotations. If we change the magic number we will just break them

Well, not for the users (like Podman) who read the annotations, I think?

Either way, I’ll leave it entirely up to you how stable the format should be right now.

right, it should not really matter. I've changed the magic number and pushed a new version

mtrmac
mtrmac approved these changes Sep 5, 2023
View reviewed changes
Copy link
Collaborator

@mtrmac mtrmac left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mtrmac mtrmac merged commit a46ccc0 into containers:main Sep 5, 2023
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mtrmac mtrmac mtrmac approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
2 participants
@giuseppe @mtrmac