Upgrade fastzip to v0.1.8
What does this MR do?
-
Updates
fastzip
so that it's compatible with newer versions ofklauspost/compress
, which updated a now deprecated call incorrectly: https://github.com/klauspost/compress/pull/502Newer versions of fastzip no longer use the deprecated function, so has fixed the issue, without requiring https://github.com/klauspost/compress/pull/502 to be merged yet.
-
Includes a test that produces an artifact and then extracts it, testing the content matches.
Why was this MR needed?
Fastzip was creating malformed zip files.
What's the best way to test this MR?
- Create a job with a large artifact and it should no longer be invalid.
- Fastzip has test coverage and the regression has been fixed: https://github.com/saracen/fastzip/actions/runs/1878454823
- Trigger failure with test using old dependency:
# works fine go test -v -run TestZipArchiveExtract ./commands/helpers # downgrade to a non-fixed fastzip + a non-fixed klauspost/compress go get -u github.com/saracen/fastzip@v0.1.6 github.com/klauspost/compress@v1.13.6 # test should now fail go test -v -run TestZipArchiveExtract ./commands/helpers
What are the relevant issue numbers?
Closes #28903 (closed)
Merge request reports
Activity
changed milestone to %14.9
assigned to @ajwalker
changed milestone to %14.8
changed milestone to %14.9
added Pick into 14.8 label
requested review from @tmaczukin
@ajwalker We should add a test that will:
- create the archive with fastzip and check if it's content is expected,
- decompress a "good" archive with fastzip and check if the content is expected.
The test should fail before the fastzip library upgrade and succeed after it. Asking in context of https://gitlab.com/gitlab-com/gl-infra/production/-/issues/6403.
Since this is feature flag driven, we probably need to cover the content validation with and without the feature flag being passed.
Is this something that can be done at the unit level @ajwalker or do you think we would need to add coverage in runner-incept? Or at an even higher level?
cc @alyubenkov
I've added a test that performs archiving with fastzip and then an extraction with fastzip.
The test has been written in such a way that it would ordinarily try different permutations of each registered archiver (legacy->fastzip, fastzip->legacy), but the legacy archiver unfortunately doesn't work as it was never updated to understand passing a directory for archiving and extracting, and always wants to use the current test directory.
Nevertheless, this test catches the regression.
@tmaczukin Back to you for review.
@tmaczukin I've updated the
What's the best way to test this MR?
steps to include running the test against problematic dependencies. The test will fail.Edited by Arran Walkerdecompress a "good" archive with fastzip and check if the content is expected.
For this, I've updated an existing test we have against all extractors to have the "good" zip file include a large file. This test won't fail for this regression, because the extractor side was working well.
removed review request for @tmaczukin
Upgrading fastzip to v0.1.8 will also fix 32bit gitlab-runners failing with exceptions when using
FF_USE_FASTZIP=true
.
See https://github.com/saracen/fastzip/pull/35mentioned in issue #28903 (closed)
requested review from @tmaczukin
mentioned in commit b826965c
removed Pick into 14.8 label
mentioned in commit 057eb691
mentioned in issue #28928 (closed)