Skip to content

Moving docker service digest pinning to client side - #30

Merged
thaJeztah merged 2 commits into
docker:masterfrom
nishanttotla:rename-trust-function
May 18, 2017
Merged

Moving docker service digest pinning to client side#30
thaJeztah merged 2 commits into
docker:masterfrom
nishanttotla:rename-trust-function

Conversation

@nishanttotla

@nishanttotlanishanttotla commented May 6, 2017•
edited
Loading

Copy link
Copy Markdown
Contributor

These are some minor changes that were originally part of this PR: moby/moby#32388

For the flag, the default should be opts.noResolveImage = false, and if --no-resolve-image is passed, then opts.noResolveImage should be set to true.
When opts.noResolveImage = false, it sets QueryRegistry in the service create/update options to true, so that the registry will be looked up to resolve digest and platform information. When opts.noResolveImage = true, then QueryRegistry is set to false. The former case should be the default, and relevant for most users.

They must be moved here now that the CLI is separate.

(Part of the multi-arch support effort in moby/moby#31348)

Ping @tiborvass@thaJeztah

Signed-off-by: Nishant Totla nishanttotla@gmail.com

@nishanttotlanishanttotla mentioned this pull request May 6, 2017
Merged
3 tasks
@nishanttotla
nishanttotlaforce-pushed the rename-trust-function branch 5 times, most recently from c4a035a to 6572aefCompareMay 11, 2017 06:55
@nishanttotlanishanttotla changed the title Renaming content trust digest resolution function for clarityMoving docker service digest pinning to client sideMay 11, 2017
@nishanttotla
nishanttotlaforce-pushed the rename-trust-function branch 12 times, most recently from 180d436 to 5b6e97aCompareMay 15, 2017 23:01
@nishanttotla

nishanttotla commented May 15, 2017

Copy link
Copy Markdown
ContributorAuthor

Added a flag --no-resolve-image.

cc @aluzzardi@aaronlehmann

@nishanttotla
nishanttotlaforce-pushed the rename-trust-function branch 4 times, most recently from f3ab29a to 1f9204aCompareMay 16, 2017 22:35
aaronlehmann
aaronlehmann reviewed May 16, 2017
View reviewed changes
Comment threadcli/command/service/update.go Outdated
if err != nil {
return err
}
if !noResolveImage {

@aaronlehmannaaronlehmannMay 16, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: This can be simplified to !options.noResolveImage.

@nishanttotlanishanttotlaMay 16, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aaronlehmann I followed the pattern used below this for flagRegistryAuth, assuming that this is how pflags work. I'll change it to this and test.

	// only send auth if flag was set
sendAuth, err := flags.GetBool(flagRegistryAuth)
if err != nil {
return err
}
if sendAuth {
...

@dnephindnephinMay 17, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The options struct is the preferred way of doing it. You shouldn't need to use flags.GetBool() in most cases.

@nishanttotlanishanttotlaMay 17, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dnephin fixed.

@nishanttotlanishanttotla mentioned this pull request May 17, 2017
Merged
@vdemeestervdemeester added this to the 17.06.0 milestone May 17, 2017
@nishanttotla

nishanttotla commented May 17, 2017

Copy link
Copy Markdown
ContributorAuthor

@aaronlehmann CI fails because of vendoring issues (I removed the vendor commit because of a merge conflict). Fixed that.

@nishanttotla
nishanttotlaforce-pushed the rename-trust-function branch 4 times, most recently from 16e00b3 to 3354031CompareMay 17, 2017 21:44
@nishanttotla

nishanttotla commented May 17, 2017

Copy link
Copy Markdown
ContributorAuthor

The CI here is failing because of my vendoring of docker/docker, which I need to do to incorporate moby/moby#33237, but that also ends up bringing in moby/moby#32237. The related CLI changes haven't been merged yet: #101.

Essentially, CI will be fixed after I rebase on top of #101. (cc @jlhawn)

@aaronlehmann@thaJeztah@tiborvass, let me know if you have any code review comments until then.

@nishanttotla
nishanttotlaforce-pushed the rename-trust-function branch from 3354031 to dc39020CompareMay 17, 2017 22:24
aaronlehmann
aaronlehmann reviewed May 17, 2017
View reviewed changes
Comment threadcli/command/service/trust.go Outdated
if !command.IsTrusted() {
// Digests are resolved by the daemon when not using content
// trust.
// Digests are resolved in the client when not using content trust.

@aaronlehmannaaronlehmannMay 17, 2017

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This comment is strange. By default, the digest is resolved on the client either way.

@nishanttotlanishanttotlaMay 17, 2017

Copy link
Copy Markdown
ContributorAuthor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@aaronlehmann I updated the comment to make it clearer.

@nishanttotla
nishanttotlaforce-pushed the rename-trust-function branch 2 times, most recently from 4efee83 to ffec441CompareMay 17, 2017 22:57
@nishanttotlanishanttotla mentioned this pull request May 17, 2017
Merged
@nishanttotla
nishanttotlaforce-pushed the rename-trust-function branch from ffec441 to 317c1aaCompareMay 17, 2017 23:41
@nishanttotla

nishanttotla commented May 17, 2017

Copy link
Copy Markdown
ContributorAuthor

Rebased this PR after #101 was merged.

@aaronlehmann

aaronlehmann commented May 17, 2017

Copy link
Copy Markdown

Remove the commit that adds github.com/opencontainers/image-spec to vendor.conf. It was already added in #101.

@nishanttotla
nishanttotlaforce-pushed the rename-trust-function branch from 317c1aa to a252888CompareMay 17, 2017 23:49
@nishanttotla

nishanttotla commented May 18, 2017

Copy link
Copy Markdown
ContributorAuthor

@aaronlehmann@tiborvass@thaJeztah CI passes now.

@tiborvass

tiborvass commented May 18, 2017

Copy link
Copy Markdown
Collaborator

LGTM

@nishanttotla
Service create/update set QueryRegistry appropriately
c61ea6f
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
@nishanttotla
nishanttotlaforce-pushed the rename-trust-function branch from a252888 to 8ec8061CompareMay 18, 2017 00:39
@tonistiigi

tonistiigi commented May 18, 2017

Copy link
Copy Markdown
Member

LGTM

@nishanttotla
Adding a flag to allow disabling registry lookup
39d03bb
Signed-off-by: Nishant Totla <nishanttotla@gmail.com>
@nishanttotla
nishanttotlaforce-pushed the rename-trust-function branch from 8ec8061 to 39d03bbCompareMay 18, 2017 00:55
@aaronlehmann

aaronlehmann commented May 18, 2017

Copy link
Copy Markdown

LGTM

@thaJeztah

thaJeztah commented May 18, 2017

Copy link
Copy Markdown
Member

all 💚

thaJeztah
thaJeztah approved these changes May 18, 2017
View reviewed changes

@thaJeztahthaJeztah left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@thaJeztah
thaJeztah merged commit 74cc280 into docker:masterMay 18, 2017
@nishanttotla
nishanttotla deleted the rename-trust-function branch May 18, 2017 01:06
@nishanttotlanishanttotla mentioned this pull request May 22, 2017
Merged
@aaronlehmannaaronlehmann mentioned this pull request Jun 13, 2017
Merged
5 tasks
thaJeztah pushed a commit to thaJeztah/cli that referenced this pull request Jun 29, 2018
@andrewhsu
Merge pull requestdocker#30from thaJeztah/18.03-backport-fix-trust-…
8c715e7
…signer-remove
[18.03-ee] backport fix docker trust signer removal
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztahthaJeztahthaJeztah approved these changes
@tiborvasstiborvassAwaiting requested review from tiborvass
+2 more reviewers
@dnephindnephindnephin left review comments
@aaronlehmannaaronlehmannaaronlehmann left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

status/2-code-review

Projects

None yet

Milestone

17.06.0

Development

Successfully merging this pull request may close these issues.

7 participants

@nishanttotla@aaronlehmann@tiborvass@tonistiigi@thaJeztah@dnephin@vdemeester