Use archive/zip for compression #3060
Conversation
|
test integrations |
|
Created or updated PR in integrations repository to test this version. Check elastic/integrations#15918 |
internal/files/compress.go
Outdated
| ) | ||
|
|
||
| // Zip function creates the .zip archive from the source path (built package content). | ||
| func Zip(ctx context.Context, sourcePath, destinationFile string) error { |
There was a problem hiding this comment.
can we add a small unit test that verifies this works as expected?
i am wondering, both sourcePath and destinationFile are absolute paths, do they have to belong to the repository?
There was a problem hiding this comment.
can we add a small unit test that verifies this works as expected?
Added in 01ad9b3
i am wondering, both sourcePath and destinationFile are absolute paths, do they have to belong to the repository?
Not really, both are intended to be used with built files. The source path is the built package, and the destination file is the built zip.
| } | ||
|
|
||
| logger.Debugf("Create work directory for archiving: %s", workDir) | ||
| err = CopyAll(sourcePath, workDir) |
There was a problem hiding this comment.
I have taken the opportunity to make a small refactor, with the new fsWithPrefix wrapper, we don't need to copy all the files only to add the root directory.
There was a problem hiding this comment.
Instead of adding a wrapper, I have followed at the end the same approach as Mario in https://github.com/elastic/package-storage-infra/pull/1069, cloning zip.AddFS to add the prefix.
| ) | ||
|
|
||
| // Zip function creates the .zip archive from the source path (built package content). | ||
| func Zip(ctx context.Context, sourcePath, destinationFile string) error { |
There was a problem hiding this comment.
Current implementation doesn't use a context, I have removed it, and from all the callers that don't use it.
There was a problem hiding this comment.
Removing >200 indirect dependencies is actually the best part of this change 🙂
Update: this is 30% of elastic-package dependencies 😮
💚 Build Succeeded
History
cc @jsoriano |
| } | ||
|
|
||
| err = files.Zip(ctx, builtPackageDir, zippedPackagePath) | ||
| logger.Debugf("Compress using archives.Zip (destination: %s)", zippedPackagePath) |
There was a problem hiding this comment.
👍
Not sure if this was added just for debugging, but it looks like it could be kept:
2025/11/11 10:53:03 DEBUG Build zipped package
2025/11/11 10:53:03 DEBUG Compress using archives.Zip (destination: /home/user/Coding/work/integrations/build/packages/nginx-2.3.2.zip)
2025/11/11 10:53:03 DEBUG Validating built .zip package (path: /home/user/Coding/work/integrations/build/packages/nginx-2.3.2.zip)
There was a problem hiding this comment.
I moved it from https://github.com/elastic/elastic-package/pull/3060/files#diff-e0a183dff782617b966d67f50da4f11ebb95719f1e73e820ef9a98c6aacbea33L21.
I think it makes more sense here.
4 participants
Use stdlib's
archive.zipfor Zip compression.The main benefits of the other library are:
We are not using any of these benefits, and on the other hand it has the following problems:
In addition to that, the stdlib includedAddFSin 1.22.0, that is quite handy to just compress a directory with good-enough defaults.AddFSis not used at the end.