feat: explicit optional compression in dropgz #3648
Conversation
Signed-off-by: Evan Baker <[email protected]>
rbtr
requested review from
camrynl,
paulyufan2,
ashvindeodhar and
thatmattlong
as code owners
There was a problem hiding this comment.
Pull Request Overview
This PR introduces an optional decompression mechanism for embedded blobs in dropgz by adding a new compression flag. Key changes include:
- Introducing a new Compression type with supported constants (None and Gzip) in payload.go.
- Updating the Extract, deploy, and Deploy functions to accept and propagate the compression parameter.
- Adding a new CLI flag for compression in the payload command and removing outdated local-run instructions from the README.
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| dropgz/pkg/embed/payload.go | Added Compression type and integrated conditional decompression. |
| dropgz/cmd/payload.go | Updated command definitions to pass the compression parameter. |
| dropgz/README.md | Removed outdated instructions that enforced gzip compression. |
Comments suppressed due to low confidence (1)
dropgz/README.md:1
- [nitpick] The removal of local run instructions may leave users without guidance on how to use the new '--compression' flag; please update or provide alternative documentation reflecting the new behavior.
-### Running the dropgz locally
|
/azp run Azure Container Networking PR |
|
Azure Pipelines successfully started running 1 pipeline(s). |
There was a problem hiding this comment.
If dropgz's default behavior now assumes uncompressed files, and previously we placed compressed files into the dropgz image (like inside azure-cni), do we also need to update these other dockerfiles (to place uncompressed files), or make it so that dropgz runs with gzip compression (your new flag) in pipelines etc.? Or does this compression on pertain to how the files are transferred out?
There was a problem hiding this comment.
Would it be beneficial to add a new README with updated instructions for running dropgz?
Yes, but dropgz is a separate go mod and is used as a library in those dockerfiles. It must be released before they can be updated, and if we make no change to them they just keep using the older version with compression. |
So let's say this PR gets merged and then we build a new cni/cns image right after-- will this image be broken because we place compressed images inside when we build dropgz, and then try to extract with the default compression as none? |
|
nothing will break or change until we release a dropgz 0.0.13, and update the CNS/CNI dockerfiles to use it |
There was a problem hiding this comment.
Pull Request Overview
Make decompression of embedded blobs optional by introducing a --compression flag and defaulting to no decompression.
- Define a
Compressionenum (None,Gzip) and updateExtract,deploy, andDeployto accept it. - Add a
--compressionCLI flag inpayload.goand pass it through to the embed API. - Remove outdated local usage instructions from the README (needs replacement).
Reviewed Changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| dropgz/pkg/embed/payload.go | Add Compression type/constants and update Extract/deploy to use it |
| dropgz/cmd/payload.go | Introduce --compression flag, propagate to extract/deploy |
| dropgz/README.md | Remove old local usage steps (no replacement for new flag usage) |
Comments suppressed due to low confidence (2)
dropgz/cmd/payload.go:10
- The code references
embed.Compressionandembed.Deploybut does not import theembedpackage. Add"dropgz/pkg/embed"to the import block.
import (
dropgz/README.md:1
- The README content was removed but no new instructions were added for the
--compressionflag. Add updated usage examples covering compression modes.
-### Running the dropgz locally
| if err != nil { | ||
| return nil, errors.Wrap(err, "failed to build reader") | ||
| var rc io.ReadCloser = f | ||
| switch compression { |
There was a problem hiding this comment.
The switch ignores unsupported compression values and silently defaults to no decompression. Consider adding a default case that returns an error for unknown Compression values.
Copilot uses AI. Check for mistakes.
| @@ -10,6 +10,12 @@ import ( | |||
| "go.uber.org/zap" | |||
| ) | |||
|
|
|||
| var ( | |||
| compression embed.Compression | |||
| skipVerify bool | |||
There was a problem hiding this comment.
The skipVerify flag is declared and bound but never used in the deploy command. Remove it or implement its logic to skip checksum validation.
Copilot uses AI. Check for mistakes.
| @@ -120,6 +121,7 @@ func init() { | |||
| root.AddCommand(verify) | |||
|
|
|||
| deploy.ValidArgs, _ = embed.Contents() // setting this after the command is initialized is required | |||
| deploy.Flags().StringVarP((*string)(&compression), "compression", "c", "none", "compression type (default none)") | |||
There was a problem hiding this comment.
[nitpick] Casting an alias type pointer to *string is error-prone. Consider implementing flag.Value on Compression and using Flags().VarP() for stronger type safety.
| deploy.Flags().StringVarP((*string)(&compression), "compression", "c", "none", "compression type (default none)") | |
| deploy.Flags().VarP(&compression, "compression", "c", "compression type (default none)") |
Copilot uses AI. Check for mistakes.
Previously, dropgz has expected and required that embedded blobs be gzip compressed.
This change makes decompressing the stream optional and defaults to no decompression, by adding a
--compressionflag and simply streaming the embed out if compression is unset.dropgz deploynow defaults to assuming uncompressed embeds. Only gzip compression is supported and may be set.