trust sign: add --local flag #575

eiais · 2017-09-29T17:05:39Z

The --local flag will force the signing of a local image.

Signed-off-by: Kyle Spiers [email protected]

codecov-io · 2017-09-29T17:20:35Z

Codecov Report

Merging #575 into master will decrease coverage by 0.66%.
The diff coverage is 90%.

@@            Coverage Diff             @@
##           master     #575      +/-   ##
==========================================
- Coverage   50.09%   49.43%   -0.67%     
==========================================
  Files         216      208       -8     
  Lines       17696    17170     -526     
==========================================
- Hits         8865     8488     -377     
+ Misses       8387     8249     -138     
+ Partials      444      433      -11

mdlinville

One question and a small nit

mdlinville · 2017-09-29T17:51:48Z

docs/reference/commandline/trust_sign.md

@@ -16,10 +16,14 @@ keywords: "sign, notary, trust"
 # trust sign

 ```markdown
-Usage:  docker trust sign IMAGE:TAG
+Usage:  docker trust sign [OPTIONS] IMAGE:TAG


I think @riyazdf pulled [OPTIONS] out of a bunch of the other commands. Is it OK here?

Yes, we pulled it out because they didn't have options yet. This one now does.

vdemeester · 2017-10-02T08:59:27Z

cli/command/trust/sign_test.go

@@ -295,3 +295,13 @@ func TestSignCommandChangeListIsCleanedOnError(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, len(cl.List()), 0)
 }
+
+func TestLocalFlag(t *testing.T) {


@eiais Not sure I clearly understand that test 🤔
Shouldn't we also test the happy path (i.e. "Signing and pushing trust data for local image" […]).

thaJeztah · 2017-10-02T11:10:35Z

I also left a similar proposal in the original PR: #472 (review)

Wondering if it's always desirable to push the image, or if there should be an option to either make pushing optional, or to disable pushing.

An alternative could be to have a --push flag, but @riyazdf is more familiar with the common workflow / expectations than I am 😅

riyazdf · 2017-10-02T17:01:29Z

@thaJeztah: yup - this is a followup PR that addresses your feedback, though in a slightly different fashion.

In the context of docker, it doesn't really make sense to have a signature without a pushed image - so we've deliberately made the workflows such that if you sign an image there should always be an associated image in the registry unless you manually delete images in the registry or the signatures with notary.

Taking this into account on docker trust sign: if the image doesn't exist in a signed repository then it will push and sign. However, if the image does exist with signatures docker trust sign will multi-sign over the same digest/tag without pushing anything to the registry.

This PR adds a --local flag to always attempt pushing and signing a locally tagged image, even if signatures already exist, potentially clobbering the digest for the tag if it was previously signed.

dnephin · 2017-10-02T18:08:42Z

cli/command/trust/sign.go

 	cmd := &cobra.Command{
 		Use:   "sign IMAGE:TAG",
 		Short: "Sign an image",
 		Args:  cli.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			return runSignImage(dockerCli, args[0])
+			return signImage(dockerCli, args[0], options)


Small thing, but would you mind making the args[0] part of options struct by adding an imageName field? We do this for most commands.

dnephin · 2017-10-02T18:12:02Z

cli/command/trust/sign_test.go

@@ -295,3 +295,13 @@ func TestSignCommandChangeListIsCleanedOnError(t *testing.T) {
 	require.NoError(t, err)
 	assert.Equal(t, len(cl.List()), 0)
 }
+
+func TestLocalFlag(t *testing.T) {


The test name should reflect the function that is being tested, so the name should be something like TestSignCommandLocalFlag

dnephin · 2017-10-02T18:15:41Z

cli/command/trust/sign.go

 	return cmd
 }

-func runSignImage(cli command.Cli, imageName string) error {
+func signImage(cli command.Cli, imageName string, options signOptions) error {


I think this should be runSignImage() (or runSign would be fine too). This is another convention we use in the cli. The run prefix refers to the fact this is the function called for RunE of the Command.

thaJeztah · 2017-10-30T16:08:25Z

cli/command/trust/sign.go

 		},
 	}
+	flags := cmd.Flags()
+	flags.BoolVarP(&options.local, "local", "l", false, "Sign a locally tagged image")


Can you remove the shorthand -l for now? Trying to be a bit conservative adding shorthands, unless a) frequently used, and b) we know it's not going to conflict with a possibly more-frequently-used option.

It's easier to add them later then removing/deprecating

vdemeester

LGTM 🐸

dnephin · 2017-10-30T18:45:21Z

docs/reference/commandline/trust_sign.md


 Sign an image

+Options:
+      --help    print usage
+  -l, --local   force the signing of a local image


-l should be removed here as well

dnephin · 2017-10-30T18:48:41Z

cli/command/trust/sign_test.go

+	cmd := newSignCommand(cli)
+	cmd.SetArgs([]string{"--local", "reg-name.io/image:red"})
+	cmd.SetOutput(ioutil.Discard)
+	testutil.ErrorContains(t, cmd.Execute(), "error during connect: Get /images/reg-name.io/image:red/json: unsupported protocol scheme")


This seems like a strange expectation for a test case. Can't this use the notary fakes we have in client_test.go to make it a success case?

dnephin · 2017-10-30T18:48:44Z

cli/command/trust/sign.go

 			// Fail fast if the image doesn't exist locally
 			if err := checkLocalImageExistence(ctx, cli, imageName); err != nil {
 				return err
 			}
+			fmt.Fprintf(cli.Out(), "Signing and pushing trust data for local image %s, may overwrite remote trust data\n", imageName)


This should use cli.Err() instead of cli.Out(). This message is informational (like logging), it's not part of "normal program output" (what the user asked for by running the command).

dnephin · 2017-10-30T18:49:53Z

cli/command/trust/sign.go

 	cmd := &cobra.Command{
 		Use:   "sign IMAGE:TAG",
 		Short: "Sign an image",
 		Args:  cli.ExactArgs(1),
 		RunE: func(cmd *cobra.Command, args []string) error {
-			return runSignImage(dockerCli, args[0])
+			return runSignImage(dockerCli, args[0], options)


Minor: now that we have an options struct, the imageName can be passed as part of that struct, instead of a separate parameter. Not super important at this point since there is only a single arg, but it would be more consistent.

dnephin · 2017-10-30T18:51:37Z

cli/command/trust/sign.go

 		},
 	}
+	flags := cmd.Flags()
+	flags.BoolVarP(&options.local, "local", "", false, "Sign a locally tagged image")


I guess this should also use flags.BoolVar() now that it doesn't have a shorthand flag.

thaJeztah · 2017-10-30T22:21:25Z

Linting failure, @eiais

cli/command/trust/sign.go:1::warning: file is not gofmted with -s (gofmt)
cli/command/trust/sign.go:1::warning: file is not goimported (goimports)

Signed-off-by: Kyle Spiers <[email protected]>

dnephin

LGTM

GordonTheTurtle added the status/0-triage label Sep 29, 2017

eiais requested review from mdlinville, thaJeztah and vdemeester as code owners September 29, 2017 17:20

mdlinville suggested changes Sep 29, 2017

View reviewed changes

mdlinville approved these changes Sep 29, 2017

View reviewed changes

vdemeester added status/2-code-review and removed status/0-triage labels Oct 2, 2017

vdemeester requested changes Oct 2, 2017

View reviewed changes

dnephin reviewed Oct 2, 2017

View reviewed changes

thaJeztah reviewed Oct 30, 2017

View reviewed changes

vdemeester approved these changes Oct 30, 2017

View reviewed changes

dnephin reviewed Oct 30, 2017

View reviewed changes

GordonTheTurtle added dco/no and removed dco/no labels Oct 30, 2017

trust sign: add --local flag

f5a5065

Signed-off-by: Kyle Spiers <[email protected]>

docker deleted a comment from GordonTheTurtle Oct 31, 2017

dnephin approved these changes Nov 7, 2017

View reviewed changes

dnephin merged commit ee0615d into docker:master Nov 7, 2017

GordonTheTurtle added this to the 17.12.0 milestone Nov 7, 2017

thaJeztah added impact/changelog kind/experimental labels Nov 7, 2017

albers mentioned this pull request Nov 17, 2017

Add bash completion for trust sign --local #701

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

trust sign: add --local flag #575

trust sign: add --local flag #575

eiais commented Sep 29, 2017

codecov-io commented Sep 29, 2017 •

edited

Loading

mdlinville left a comment

mdlinville Sep 29, 2017

eiais Sep 29, 2017

vdemeester Oct 2, 2017

thaJeztah commented Oct 2, 2017

riyazdf commented Oct 2, 2017

dnephin Oct 2, 2017

dnephin Oct 2, 2017

dnephin Oct 2, 2017

thaJeztah Oct 30, 2017

eiais Oct 30, 2017

vdemeester left a comment

dnephin Oct 30, 2017

dnephin Oct 30, 2017

dnephin Oct 30, 2017

dnephin Oct 30, 2017

dnephin Oct 30, 2017

thaJeztah commented Oct 30, 2017

dnephin left a comment

trust sign: add --local flag #575

trust sign: add --local flag #575

Conversation

eiais commented Sep 29, 2017

codecov-io commented Sep 29, 2017 • edited Loading

Codecov Report

mdlinville 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

thaJeztah commented Oct 2, 2017

riyazdf commented Oct 2, 2017

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

vdemeester 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

Choose a reason for hiding this comment

thaJeztah commented Oct 30, 2017

dnephin left a comment

Choose a reason for hiding this comment

codecov-io commented Sep 29, 2017 •

edited

Loading