`docker manifest rm` command to remove manifest list draft from local storage by jennydaman · Pull Request #2449 · docker/cli

jennydaman · 2020-04-18T01:33:25Z

- What I did

Made a typo and wanted to delete my manifest from local storage, yet there was no easy way to do so without using docker manifest push --purge.

I implemented a new sub-command docker manifest rm

$ docker manifest rm
"docker manifest rm" requires exactly 1 argument.
See 'docker manifest rm --help'.

Usage:  docker manifest rm MANIFEST_LIST

Delete a manifest list from local storage

- How I did it

tbh I never programmed in Go before, so I copied and pasted push.go to rm.go and kept deleting lines until it would compile.

- How to verify it

make -f docker.Makefile binary
build/docker-linux-amd64 manifest create repo/example:latest amd64/ubuntu:latest ppc64le/ubuntu:latest
build/docker-linux-amd64 manifest inspect repo/example:latest
build/docker-linux-amd64 manifest rm repo/example:latest
build/docker-linux-amd64 manifest create repo/example:latest i386/ubuntu arm64v8/ubuntu
build/docker-linux-amd64 manifest inspect repo/example:latest

- Description for the changelog

Add sub-command docker manifest rm

- A picture of a cute animal (not mandatory but encouraged)

thaJeztah · 2020-04-21T14:47:52Z

+		Short: "Delete a manifest list from local storage",
+		Args:  cli.ExactArgs(1),
+		RunE: func(cmd *cobra.Command, args []string) error {
+			opts.target = args[0]


Should it accept multiple manifests? (i.e., docker manifest rm one two three) other rm commands support multiple manifests so it would probably make sense to accept that for this command as well

hassenius · 2020-09-09T08:44:28Z

Any view on when this might merge?

This is exactly the functionality we're looking for to ensure that old artifacts are cleaned if there is an issue with previous jobs push --purge while not needing to manually delete files (whose location could change in the future)

silvin-lubecki · 2020-09-09T11:40:41Z

+		return err
+	}
+
+	manifests, err := dockerCli.ManifestStore().GetList(targetRef)


question: I wonder why we search the manifest list, couldn't we just remove it and return Remove error?

sorry, naive copy-paste.

fixed 30466e2

actually in working on supporting multiple args it turns out that dockerCli.ManifestStore().GetList(targetRef) is useful because dockerCli.ManifestStore().Remove(targetRef) will fail silently when targetRef does not exist.

silvin-lubecki

I think the adding the rm subcommand is fine, but this PR needs some tests to be merged 👍

jennydaman · 2020-09-09T15:10:33Z

Hi everyone,

I am really not a Go dev, this PR was mostly a proof-of-concept. I can try writing some tests...

rishinair11 · 2020-09-10T05:59:14Z

Hi everyone,

I am really not a Go dev, this PR was mostly a proof-of-concept. I can try writing some tests...

Really appreciate that.

jennydaman · 2020-09-10T06:42:34Z

FYI for my own use cases I found docker buildx as an easier and faster way to build cross-platform manifest lists.

rishinair11 · 2020-09-10T07:04:44Z

FYI for my own use cases I found docker buildx as an easier and faster way to build cross-platform manifest lists.

Does buildx have a cleanup like the one you implemented above?

jennydaman · 2020-09-10T07:12:14Z

Yes. buildx is for building and manifest is for managing manifests. The ability to create a manifest list is only one of many buildx features. The manifest list is created automatically for multiplatform builds. Clean up with docker buildx prune and docker buildx rm

docker#2449 (comment)

codecov-commenter · 2020-09-13T03:49:47Z

Codecov Report

Merging #2449 into master will increase coverage by 0.02%.
The diff coverage is 77.77%.

@@            Coverage Diff             @@
##           master    #2449      +/-   ##
==========================================
+ Coverage   58.59%   58.61%   +0.02%     
==========================================
  Files         296      297       +1     
  Lines       21360    21387      +27     
==========================================
+ Hits        12516    12537      +21     
- Misses       7931     7935       +4     
- Partials      913      915       +2

docker#2449 (comment) Signed-off-by: Jennings Zhang <[email protected]>

jennydaman · 2020-09-13T05:59:54Z

Happy to say that the tooling was surprisingly easy to pick up!

docker manifest rm supports multiple arguments
added tests

silvin-lubecki

LGTM, thanks @jennydaman 👍

thaJeztah

code changes LGTM, but could you squash the commits? Let me know if you need instructions on doing so 🤗

albers

I found problems in bash completion, please fix.

docker#2449 (review) Signed-off-by: Jennings Zhang <[email protected]>

Squashed commit of the following: commit b9ef85e Author: Jennings Zhang <[email protected]> Date: Mon Sep 14 21:39:57 2020 -0400 Fix bash completion docker#2449 (review) Signed-off-by: Jennings Zhang <[email protected]> commit 8c46bd6 Author: Jennings Zhang <[email protected]> Date: Sun Sep 13 01:48:12 2020 -0400 Add tests for docker manifest rm Signed-off-by: Jennings Zhang <[email protected]> commit 7e3d9a9 Author: Jennings Zhang <[email protected]> Date: Sun Sep 13 00:55:37 2020 -0400 docker manifest rm multiple args Signed-off-by: Jennings Zhang <[email protected]> commit 30466e2 Author: Jennings Zhang <[email protected]> Date: Sun Sep 13 00:01:20 2020 -0400 No need to search before Remove docker#2449 (comment) Signed-off-by: Jennings Zhang <[email protected]> commit ccdc4ed Author: Jennings Zhang <[email protected]> Date: Sat Sep 12 23:42:41 2020 -0400 Completion should also handle --help docker#2449 (comment) Signed-off-by: Jennings Zhang <[email protected]> commit ed260af Merge: 46c61d8 2955ece Author: Jennings Zhang <[email protected]> Date: Sat Sep 12 23:31:54 2020 -0400 Merge branch 'master' into manifest-rm commit 46c61d8 Author: Jennings Zhang <[email protected]> Date: Fri Apr 17 21:53:33 2020 -0400 Remove extra space Signed-off-by: Jennings Zhang <[email protected]> commit 6d31d26 Author: Jennings Zhang <[email protected]> Date: Fri Apr 17 21:15:21 2020 -0400 Bash completion for `docker manifest rm` Signed-off-by: Jennings Zhang <[email protected]> commit 3c8c843 Author: Jennings Zhang <[email protected]> Date: Fri Apr 17 21:05:50 2020 -0400 Frankenstein a `docker manifest rm` command Signed-off-by: Jennings Zhang <[email protected]> Signed-off-by: Jennings Zhang <[email protected]>

jennydaman · 2020-09-15T02:01:23Z

@albers bash completion fixed

@thaJeztah I tried squash, I think it's correct-ish?

thaJeztah · 2020-09-15T11:01:18Z

#2449 (comment)

actually in working on supporting multiple args it turns out that dockerCli.ManifestStore().GetList(targetRef) is useful because dockerCli.ManifestStore().Remove(targetRef) will fail silently when targetRef does not exist.

Looks like something we should fix in a follow-up. I think it would be good to have dockerCli.ManifestStore().Remove(targetRef) return an "NotExist" error, and leave it to the caller of that function to decide wether or not that error is ok to ignore.

thaJeztah

LGTM, thanks!

Left some suggestions for follow-up improvements

@silvin-lubecki @albers PTAL

thaJeztah · 2020-09-15T10:58:19Z

+		targetRef, refErr := normalizeReference(target)
+		if refErr != nil {
+			errs = append(errs, refErr.Error())
+			continue


We could probably add a -f / --force option to switch between "failing immediately" if any of the references is invalid and/or doesn't exist, or only printing the error, but still completing the command successfully (see #2678 and #2677)

Not a blocker; this command is still experimental, so I'm ok on improving that in a follow-up

this command is still experimental

--force isn't ubiquitous either way

docker network rm --help Usage: docker network rm NETWORK [NETWORK...] Remove one or more networks Aliases: rm, remove

I think for network, there had been some discussion, but the problem was that --force would also imply "if it's in use", which could mean that containers would have to be (automatically) detached from the network.

I agree with your proposal @thaJeztah, and also ok to improve that in a follow-up 👍

silvin-lubecki

LGTM

Squashed commit of the following: commit b9ef85e Author: Jennings Zhang <[email protected]> Date: Mon Sep 14 21:39:57 2020 -0400 Fix bash completion docker#2449 (review) Signed-off-by: Jennings Zhang <[email protected]> commit 8c46bd6 Author: Jennings Zhang <[email protected]> Date: Sun Sep 13 01:48:12 2020 -0400 Add tests for docker manifest rm Signed-off-by: Jennings Zhang <[email protected]> commit 7e3d9a9 Author: Jennings Zhang <[email protected]> Date: Sun Sep 13 00:55:37 2020 -0400 docker manifest rm multiple args Signed-off-by: Jennings Zhang <[email protected]> commit 30466e2 Author: Jennings Zhang <[email protected]> Date: Sun Sep 13 00:01:20 2020 -0400 No need to search before Remove docker#2449 (comment) Signed-off-by: Jennings Zhang <[email protected]> commit ccdc4ed Author: Jennings Zhang <[email protected]> Date: Sat Sep 12 23:42:41 2020 -0400 Completion should also handle --help docker#2449 (comment) Signed-off-by: Jennings Zhang <[email protected]> commit ed260af Merge: 46c61d8 2955ece Author: Jennings Zhang <[email protected]> Date: Sat Sep 12 23:31:54 2020 -0400 Merge branch 'master' into manifest-rm commit 46c61d8 Author: Jennings Zhang <[email protected]> Date: Fri Apr 17 21:53:33 2020 -0400 Remove extra space Signed-off-by: Jennings Zhang <[email protected]> commit 6d31d26 Author: Jennings Zhang <[email protected]> Date: Fri Apr 17 21:15:21 2020 -0400 Bash completion for `docker manifest rm` Signed-off-by: Jennings Zhang <[email protected]> commit 3c8c843 Author: Jennings Zhang <[email protected]> Date: Fri Apr 17 21:05:50 2020 -0400 Frankenstein a `docker manifest rm` command Signed-off-by: Jennings Zhang <[email protected]> Signed-off-by: Jennings Zhang <[email protected]>

thaJeztah

LGTM, thanks!

Squashed commit of the following: commit b9ef85e74833ba405f68cfc20989c69d64bac4e9 Author: Jennings Zhang <[email protected]> Date: Mon Sep 14 21:39:57 2020 -0400 Fix bash completion docker/cli#2449 (review) Signed-off-by: Jennings Zhang <[email protected]> commit 8c46bd6e6ed151bb43865c8b1d79c00fd62e4345 Author: Jennings Zhang <[email protected]> Date: Sun Sep 13 01:48:12 2020 -0400 Add tests for docker manifest rm Signed-off-by: Jennings Zhang <[email protected]> commit 7e3d9a9bc60e44d96953093fa0b1bc3397ca7813 Author: Jennings Zhang <[email protected]> Date: Sun Sep 13 00:55:37 2020 -0400 docker manifest rm multiple args Signed-off-by: Jennings Zhang <[email protected]> commit 30466e28d28f6722053c5a232e99ddbae8222715 Author: Jennings Zhang <[email protected]> Date: Sun Sep 13 00:01:20 2020 -0400 No need to search before Remove docker/cli#2449 (comment) Signed-off-by: Jennings Zhang <[email protected]> commit ccdc4ed0a620cf8c9ec6ecc6804d1a45f7c61be5 Author: Jennings Zhang <[email protected]> Date: Sat Sep 12 23:42:41 2020 -0400 Completion should also handle --help docker/cli#2449 (comment) Signed-off-by: Jennings Zhang <[email protected]> commit ed260afa71a4f8feb6550f79692e47ad7430d786 Merge: 46c61d85e9 2955ece024 Author: Jennings Zhang <[email protected]> Date: Sat Sep 12 23:31:54 2020 -0400 Merge branch 'master' into manifest-rm commit 46c61d85e973cc9fdd28d42db9ecebe373e9b942 Author: Jennings Zhang <[email protected]> Date: Fri Apr 17 21:53:33 2020 -0400 Remove extra space Signed-off-by: Jennings Zhang <[email protected]> commit 6d31d26c10e8d395ab08561cdb9b29829bb4bd91 Author: Jennings Zhang <[email protected]> Date: Fri Apr 17 21:15:21 2020 -0400 Bash completion for `docker manifest rm` Signed-off-by: Jennings Zhang <[email protected]> commit 3c8c843deb2f751a5f51ee6fcaa75da2a4525d99 Author: Jennings Zhang <[email protected]> Date: Fri Apr 17 21:05:50 2020 -0400 Frankenstein a `docker manifest rm` command Signed-off-by: Jennings Zhang <[email protected]> Signed-off-by: Jennings Zhang <[email protected]> Upstream-commit: 185d71262a56dfb2d5c11d49441f6acb729055eb Component: cli

Squashed commit of the following: commit b9ef85e Author: Jennings Zhang <[email protected]> Date: Mon Sep 14 21:39:57 2020 -0400 Fix bash completion docker#2449 (review) Signed-off-by: Jennings Zhang <[email protected]> commit 8c46bd6 Author: Jennings Zhang <[email protected]> Date: Sun Sep 13 01:48:12 2020 -0400 Add tests for docker manifest rm Signed-off-by: Jennings Zhang <[email protected]> commit 7e3d9a9 Author: Jennings Zhang <[email protected]> Date: Sun Sep 13 00:55:37 2020 -0400 docker manifest rm multiple args Signed-off-by: Jennings Zhang <[email protected]> commit 30466e2 Author: Jennings Zhang <[email protected]> Date: Sun Sep 13 00:01:20 2020 -0400 No need to search before Remove docker#2449 (comment) Signed-off-by: Jennings Zhang <[email protected]> commit ccdc4ed Author: Jennings Zhang <[email protected]> Date: Sat Sep 12 23:42:41 2020 -0400 Completion should also handle --help docker#2449 (comment) Signed-off-by: Jennings Zhang <[email protected]> commit ed260af Merge: 46c61d8 2955ece Author: Jennings Zhang <[email protected]> Date: Sat Sep 12 23:31:54 2020 -0400 Merge branch 'master' into manifest-rm commit 46c61d8 Author: Jennings Zhang <[email protected]> Date: Fri Apr 17 21:53:33 2020 -0400 Remove extra space Signed-off-by: Jennings Zhang <[email protected]> commit 6d31d26 Author: Jennings Zhang <[email protected]> Date: Fri Apr 17 21:15:21 2020 -0400 Bash completion for `docker manifest rm` Signed-off-by: Jennings Zhang <[email protected]> commit 3c8c843 Author: Jennings Zhang <[email protected]> Date: Fri Apr 17 21:05:50 2020 -0400 Frankenstein a `docker manifest rm` command Signed-off-by: Jennings Zhang <[email protected]> Signed-off-by: Jennings Zhang <[email protected]>

jennydaman requested a review from albers as a code owner April 18, 2020 01:33

thaJeztah added kind/experimental kind/feature status/2-code-review labels Apr 21, 2020

thaJeztah reviewed Apr 21, 2020

View reviewed changes

albers requested changes Jun 20, 2020

View reviewed changes

Comment thread contrib/completion/bash/docker Outdated

silvin-lubecki reviewed Sep 9, 2020

View reviewed changes

silvin-lubecki requested changes Sep 9, 2020

View reviewed changes

jennydaman pushed a commit to jennydaman/cli that referenced this pull request Sep 13, 2020

Completion should also handle --help

0e7c04f

docker#2449 (comment)

jennydaman pushed a commit to jennydaman/cli that referenced this pull request Sep 13, 2020

Completion should also handle --help

ccdc4ed

docker#2449 (comment) Signed-off-by: Jennings Zhang <[email protected]>

jennydaman force-pushed the manifest-rm branch from 0e7c04f to ccdc4ed Compare September 13, 2020 03:52

jennydaman pushed a commit to jennydaman/cli that referenced this pull request Sep 13, 2020

No need to search before Remove

30466e2

docker#2449 (comment) Signed-off-by: Jennings Zhang <[email protected]>

jennydaman requested review from albers, silvin-lubecki and thaJeztah September 13, 2020 05:57

silvin-lubecki approved these changes Sep 14, 2020

View reviewed changes

thaJeztah requested changes Sep 14, 2020

View reviewed changes

thaJeztah added the impact/changelog label Sep 14, 2020

thaJeztah added this to the 20.03.0 milestone Sep 14, 2020

albers requested changes Sep 14, 2020

View reviewed changes

Comment thread contrib/completion/bash/docker Outdated

Comment thread contrib/completion/bash/docker Outdated

Comment thread contrib/completion/bash/docker Outdated

jennydaman pushed a commit to jennydaman/cli that referenced this pull request Sep 15, 2020

Fix bash completion

b9ef85e

docker#2449 (review) Signed-off-by: Jennings Zhang <[email protected]>

jennydaman force-pushed the manifest-rm branch from 8c46bd6 to 29dcc37 Compare September 15, 2020 01:58

jennydaman requested review from albers and thaJeztah September 15, 2020 01:59

thaJeztah approved these changes Sep 15, 2020

View reviewed changes

silvin-lubecki approved these changes Sep 15, 2020

View reviewed changes

albers requested changes Sep 15, 2020

View reviewed changes

Comment thread contrib/completion/bash/docker

jennydaman force-pushed the manifest-rm branch from 29dcc37 to 185d712 Compare September 15, 2020 20:26

jennydaman requested a review from albers September 15, 2020 20:41

thaJeztah approved these changes Sep 17, 2020

View reviewed changes

thaJeztah merged commit b747821 into docker:master Sep 17, 2020

jennydaman mentioned this pull request Sep 17, 2020

Fix bash completion typo docker manifest rm #2728

Merged

Uh oh!

Conversation

jennydaman commented Apr 18, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

hassenius commented Sep 9, 2020

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silvin-lubecki left a comment

Choose a reason for hiding this comment

Uh oh!

jennydaman commented Sep 9, 2020

Uh oh!

rishinair11 commented Sep 10, 2020

Uh oh!

jennydaman commented Sep 10, 2020

Uh oh!

rishinair11 commented Sep 10, 2020

Uh oh!

jennydaman commented Sep 10, 2020

Uh oh!

codecov-commenter commented Sep 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

jennydaman commented Sep 13, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silvin-lubecki left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

albers left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

jennydaman commented Sep 15, 2020

Uh oh!

thaJeztah commented Sep 15, 2020

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

silvin-lubecki left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

thaJeztah left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-commenter commented Sep 13, 2020 •

edited

Loading

jennydaman commented Sep 13, 2020 •

edited

Loading