X Tutup
Skip to content

Update docker-credential-helpers dependency#1151

Merged
vdemeester merged 1 commit intodocker:masterfrom
vdemeester:update-docker-credential-helper-pass
Jun 28, 2018
Merged

Update docker-credential-helpers dependency#1151
vdemeester merged 1 commit intodocker:masterfrom
vdemeester:update-docker-credential-helper-pass

Conversation

@vdemeester
Copy link
Collaborator

@vdemeester vdemeester commented Jun 27, 2018

This is mainly for the pass helper ; pass shouldn't be called
every docker command anymore ;).

Linked to docker/docker-credential-helpers#96

cc @euank

Signed-off-by: Vincent Demeester vincent@sbr.pm

@vdemeester vdemeester requested review from dnephin, n4ss and thaJeztah June 27, 2018 13:22
silvin-lubecki
silvin-lubecki reviewed Jun 27, 2018
View reviewed changes
cli/config/credentials/default_store_linux.go Outdated

func defaultCredentialsStore() string {
if pass.PassInitialized {
pass := pass.Pass{}
Copy link
Contributor

@silvin-lubecki silvin-lubecki Jun 27, 2018

Choose a reason for hiding this comment

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

nit:
Too much pass 😵
And not fan of shadowing a package by a variable.

Copy link
Contributor

@n4ss n4ss Jun 27, 2018

Choose a reason for hiding this comment

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

yeah same, besides that, LGTM!

n4ss
n4ss reviewed Jun 27, 2018
View reviewed changes
Copy link
Contributor

@n4ss n4ss left a comment

Choose a reason for hiding this comment

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

LGTM after a rename to passStore or something similar.

@vdemeester
Update docker-credential-helpers dependency
d9741fc 
This is mainly for the `pass` helper ; `pass` shouldn't be called
every docker command anymore ;).

Signed-off-by: Vincent Demeester <vincent@sbr.pm>
@vdemeester vdemeester force-pushed the update-docker-credential-helper-pass branch from 8a8ea33 to d9741fc Compare June 28, 2018 12:44
@vdemeester
Copy link
Collaborator Author

vdemeester commented Jun 28, 2018

@n4ss @silvin-lubecki Updated 😉

silvin-lubecki
silvin-lubecki approved these changes Jun 28, 2018
View reviewed changes
Copy link
Contributor

@silvin-lubecki silvin-lubecki left a comment

Choose a reason for hiding this comment

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

LGTM

@vdemeester vdemeester merged commit 2935539 into docker:master Jun 28, 2018
@GordonTheTurtle GordonTheTurtle added this to the 18.06.0 milestone Jun 28, 2018
@vdemeester vdemeester deleted the update-docker-credential-helper-pass branch June 28, 2018 12:57
@thaJeztah
Copy link
Member

thaJeztah commented Jun 28, 2018

@n4ss or @vdemeester should we tag a v0.6.1 release? docker/docker-credential-helpers@v0.6.0...master

@thaJeztah thaJeztah modified the milestones: 18.06.0, 18.07.0 Jun 28, 2018
@thaJeztah
Copy link
Member

thaJeztah commented Jun 28, 2018

Also; this fixes #699, correct?

@vdemeester
Copy link
Collaborator Author

vdemeester commented Jun 28, 2018

@thaJeztah it fixes docker/docker-credential-helpers#96 not #699 as it still execute any pass command in the PATH, just not for all docker commands.

@euank
Copy link
Contributor

euank commented Jun 28, 2018

pass shouldn't be called every docker command anymore ;).

I don't think that's correct and I don't think this actually fixes #699.

Before, docker-credential-helpers called pass in init so it was impossible for anyone to use the library without pass being called.

It's now possible to use it without pass running, but I don't think docker/cli does.

Specifically, the cli call here will execute pass any time it loads a config file which doesn't contain any auth configuration:

cli/cli/config/config.go

Lines 109 to 120 in ea65e90

// LoadDefaultConfigFile attempts to load the default config file and returns
// an initialized ConfigFile struct if none is found.
func LoadDefaultConfigFile(stderr io.Writer) *configfile.ConfigFile {
configFile, err := Load(Dir())
if err != nil {
fmt.Fprintf(stderr, "WARNING: Error loading config file: %v\n", err)
}
if !configFile.ContainsAuth() {
configFile.CredentialsStore = credentials.DetectDefaultStore(configFile.CredentialsStore)
}
return configFile
}

This will allow people to avoid the call now by setting their config file in such a way that's skipped, but for a default empty config file it will still execute.

I think either credential checks need to be lazy such that it only initializes that if it's needed, not as soon as it loads the config file, or it needs to be a simpler initialization check there of just LookPath on the well-known 'pass' name.

@vdemeester
Copy link
Collaborator Author

vdemeester commented Jun 28, 2018

@euank thanks for the comment 👼 I think you're right, somehow I though I also did sthg like #1131 in there 😅

@euank
Copy link
Contributor

euank commented Jun 28, 2018

I'd be happy to PR lazy initialization (something like euank@ae945b7) if that seems like a preferable approach... but I don't know if invoking pass on docker pull when no auth config is present is a reasonable behaviour or not.

Perhaps there should be IsPresent in credential-helpers and I was wrong to not include that.

Since I've realized lazy initialization will still end up invoking it even on pull of public images, it seems to me like either adding IsPresent to credential-helpers or directly doing LookPath in this codebase will be the best bet. Do you have any preference @vdemeester?

@vdemeester
Copy link
Collaborator Author

vdemeester commented Jun 29, 2018

@euank I think you're right, either provide a IsPresent and using it in docker/cli or even simpler doing a LookPath in docker/cli seems fine by me
(cc @thaJeztah @n4ss )

@euank
Copy link
Contributor

euank commented Jun 29, 2018

PR filed (#1160) @vdemeester

@max-baz max-baz mentioned this pull request Jun 29, 2018
Open
@thaJeztah thaJeztah mentioned this pull request Jul 3, 2018
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@silvin-lubecki silvin-lubecki silvin-lubecki approved these changes

@dnephin dnephin Awaiting requested review from dnephin

@thaJeztah thaJeztah Awaiting requested review from thaJeztah

+1 more reviewer

@n4ss n4ss n4ss left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

status/2-code-review

Projects

None yet

Milestone

18.09.0

Development

Successfully merging this pull request may close these issues.

6 participants

@vdemeester @thaJeztah @euank @n4ss @silvin-lubecki @GordonTheTurtle
X Tutup