`docker manifest rm` command to remove manifest list draft from local storage #2449

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

cli/command/manifest/rm.go

+		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

contrib/completion/bash/docker

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

cli/command/manifest/rm.go

+		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 <jenni_zh@protonmail.com>

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.

contrib/completion/bash/docker

docker#2449 (review) Signed-off-by: Jennings Zhang <jenni_zh@protonmail.com>

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

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

cli/command/manifest/rm.go

+		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

contrib/completion/bash/docker

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

thaJeztah

LGTM, thanks!

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

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

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

contrib/completion/bash/docker Outdated Show resolved Hide resolved

silvin-lubecki reviewed Sep 9, 2020

View reviewed changes

silvin-lubecki requested changes Sep 9, 2020

View reviewed changes

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

Completion should also handle --help

0e7c04f

docker#2449 (comment)

jennydaman added 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 <jenni_zh@protonmail.com>

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

jennydaman added 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 <jenni_zh@protonmail.com>

jennydaman requested review from silvin-lubecki, albers 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

contrib/completion/bash/docker Outdated Show resolved Hide resolved

contrib/completion/bash/docker Outdated Show resolved Hide resolved

contrib/completion/bash/docker Outdated Show resolved Hide resolved

jennydaman added 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 <jenni_zh@protonmail.com>

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

contrib/completion/bash/docker Show resolved Hide resolved

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

`docker manifest rm` command to remove manifest list draft from local storage #2449

`docker manifest rm` command to remove manifest list draft from local storage #2449

jennydaman commented Apr 18, 2020

thaJeztah Apr 21, 2020

hassenius commented Sep 9, 2020

silvin-lubecki Sep 9, 2020

jennydaman Sep 13, 2020

jennydaman Sep 13, 2020

silvin-lubecki left a comment

jennydaman commented Sep 9, 2020

rishinair11 commented Sep 10, 2020

jennydaman commented Sep 10, 2020

rishinair11 commented Sep 10, 2020

jennydaman commented Sep 10, 2020

codecov-commenter commented Sep 13, 2020 •

edited

jennydaman commented Sep 13, 2020 •

edited

silvin-lubecki left a comment

thaJeztah left a comment

albers left a comment

jennydaman commented Sep 15, 2020

thaJeztah commented Sep 15, 2020

thaJeztah left a comment

thaJeztah Sep 15, 2020

jennydaman Sep 15, 2020

thaJeztah Sep 15, 2020

silvin-lubecki Sep 15, 2020

silvin-lubecki left a comment

thaJeztah left a comment

docker manifest rm command to remove manifest list draft from local storage #2449

docker manifest rm command to remove manifest list draft from local storage #2449

Conversation

jennydaman commented Apr 18, 2020

Choose a reason for hiding this comment

hassenius commented Sep 9, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silvin-lubecki left a comment

Choose a reason for hiding this comment

jennydaman commented Sep 9, 2020

rishinair11 commented Sep 10, 2020

jennydaman commented Sep 10, 2020

rishinair11 commented Sep 10, 2020

jennydaman commented Sep 10, 2020

codecov-commenter commented Sep 13, 2020 • edited

Codecov Report

jennydaman commented Sep 13, 2020 • edited

silvin-lubecki left a comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

albers left a comment

Choose a reason for hiding this comment

jennydaman commented Sep 15, 2020

thaJeztah commented Sep 15, 2020

thaJeztah left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

silvin-lubecki left a comment

Choose a reason for hiding this comment

thaJeztah left a comment

Choose a reason for hiding this comment

`docker manifest rm` command to remove manifest list draft from local storage #2449

`docker manifest rm` command to remove manifest list draft from local storage #2449

codecov-commenter commented Sep 13, 2020 •

edited

jennydaman commented Sep 13, 2020 •

edited