Conversation
b7756fe to
9599d0a
Compare
cmd/nerdctl/image/image_squash.go
Outdated
| cmd.Flags().StringP("message", "m", "", "Commit message") | ||
| } | ||
|
|
||
| func NewSquashCommand() *cobra.Command { |
There was a problem hiding this comment.
https://github.com/containerd/nerdctl/blob/main/docs/command-reference.md needs to be updated
cmd/nerdctl/main.go
Outdated
| image.NewTagCommand(), | ||
| image.NewRmiCommand(), | ||
| image.NewHistoryCommand(), | ||
| image.NewSquashCommand(), |
There was a problem hiding this comment.
Not a fan of inflating the number of the top-level commands
pkg/cmd/image/squash.go
Outdated
| } | ||
| } | ||
|
|
||
| // copied from github.com/containerd/containerd/rootfs/apply.go |
There was a problem hiding this comment.
Should be a permanent URL with the git tag (or the commit hash)
There was a problem hiding this comment.
The commit hash has been added.
There was a problem hiding this comment.
Suggest to use a full URL directly, e.g., https://github.com/containerd/containerd/blob/v2.0.1/pkg/rootfs/apply.go#L180
df967a2 to
b6077c1
Compare
cmd/nerdctl/image/image_squash.go
Outdated
| // NewSquashCommand returns a new `squash` command to compress the number of layers of the image | ||
| func NewSquashCommand() *cobra.Command { | ||
| var squashCommand = &cobra.Command{ | ||
| Use: "squash [flags] SOURCE_IMAGE TAG_IMAGE", |
There was a problem hiding this comment.
nit: TAG_IMAGE -> TARGET_IMAGE
| testCase := nerdtest.Setup() | ||
|
|
||
| require := test.Require( | ||
| test.Linux, |
cmd/nerdctl/image/image_squash.go
Outdated
| ) | ||
|
|
||
| func addSquashFlags(cmd *cobra.Command) { | ||
| cmd.Flags().IntP("layer-count", "c", 0, "The number of layers that can be compressed") |
There was a problem hiding this comment.
does this flag specify squashing the last N (N=layer-count) layers? If so I suggest renaming it to last-n-layer.
Or rename it to from-layer, meaning squashing all layers starting from from-layer (and --from-layer=0 means squash all layers)
There was a problem hiding this comment.
Thank you, I have changed it to last-n-layer.
cmd/nerdctl/image/image_squash.go
Outdated
|
|
||
| func addSquashFlags(cmd *cobra.Command) { | ||
| cmd.Flags().IntP("layer-count", "c", 0, "The number of layers that can be compressed") | ||
| cmd.Flags().StringP("layer-digest", "d", "", "The digest of the layer to be compressed") |
There was a problem hiding this comment.
It is indeed unnecessary, so I have removed this flag.
cmd/nerdctl/image/image_squash.go
Outdated
| cmd.Flags().StringP("author", "a", "", `Author (e.g., "nerdctl contributor <[email protected]>")`) | ||
| cmd.Flags().StringP("message", "m", "", "Commit message") |
There was a problem hiding this comment.
nit: maybe use something like author="nerdctl" and message="generated by nerdctl squash" as default values.
docs/command-reference.md
Outdated
|
|
||
| ### :nerd_face: nerdctl image squash | ||
|
|
||
| Squash an image layers. |
There was a problem hiding this comment.
suggest to add some details, e.g., Squash all layers starting from from-layer into a single layer.
pkg/cmd/image/squash.go
Outdated
| } | ||
| } | ||
|
|
||
| // copied from github.com/containerd/containerd/rootfs/apply.go |
There was a problem hiding this comment.
Suggest to use a full URL directly, e.g., https://github.com/containerd/containerd/blob/v2.0.1/pkg/rootfs/apply.go#L180
| layerN, err := cmd.Flags().GetInt("last-n-layer") | ||
| if err != nil { | ||
| return options, err | ||
| } |
There was a problem hiding this comment.
can we add a check to make sure layerN > 0?
| if matchCount < 1 { | ||
| return fmt.Errorf("%s: not found", option.SourceImageRef) | ||
| } |
There was a problem hiding this comment.
what's the behavior if matchCount > 1?
There was a problem hiding this comment.
I'm not sure. Maybe just return an error.
| return err | ||
| } | ||
| remainingLayerCount := len(img.manifest.Layers) - len(sLayers) | ||
| // Don't gc me and clean the dirty data after 1 hour! |
There was a problem hiding this comment.
why 1 hour? after squash we should just remove the temp data and keep the final new data forever?
There was a problem hiding this comment.
I'm not sure if this "leases" part is necessary. This code is copy from the "commit" command (pkg/imgutil/commit/commit.go). Could you help me take a look?
| } | ||
|
|
||
| clientImage := containerd.NewImage(sr.client, containerImage) | ||
| manifest, _, err := imgutil.ReadManifest(ctx, clientImage) |
There was a problem hiding this comment.
what if we pass in an image index instead of manifest? seems index is not supported? can we add a check to return an error if the image is not a manifest.
|
@weapons97 Could you check the comments from djdongjin? |
|
Is there any new progress? I might be able to provide assistance, as the code in this PR is derived from one of my repositories https://github.com/lingdie/squash/blob/main/pkg/squash/runtime.go, provided that this PR allows me to make modifications and the nerdctl open source community permits it. |
|
@lingdie Thanks, would you be interested in opening a new PR? Alternatively @weapons97 may invite @lingdie to allow pushing to https://github.com/weapons97/nerdctl/tree/main (if @weapons97 would like to do so) |
Sorry for the late reply. I've added you as a collaborator to the project, so you should have permission to push to the main branch. |
a68839b to
8f4449e
Compare
Signed-off-by: weapons97 <[email protected]>
This pr adds the squash image command.
The related issue is #3252.