trust sign: add --local flag by eiais · Pull Request #575 · docker/cli

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

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

Signed-off-by: Kyle Spiers kyle@spiers.me

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


 ```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

 	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

 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

 	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

 }

-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

 },
 	}
+	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


+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

+	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

 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

 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

 },
 	}
+	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 <kyle@spiers.me>

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 suggested 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 <kyle@spiers.me>

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:masterNov 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

Uh oh!

Conversation

eiais commented Sep 29, 2017

Uh oh!

codecov-io commented Sep 29, 2017• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

mdlinville 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!

thaJeztah commented Oct 2, 2017

Uh oh!

riyazdf commented Oct 2, 2017

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!

Choose a reason for hiding this comment

Uh oh!

vdemeester 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!

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Oct 30, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

codecov-io commented Sep 29, 2017•
edited
Loading