expose config and credential functionalities to 3rd parties by AkihiroSuda · Pull Request #197 · docker/cli

AkihiroSuda · 2017-06-15T05:59:28Z

- What I did

expose config and credential fuhnctionalities to 3rd parties

Example: contrib/client-with-cli-config (but without whole cli)
See also moby/moby#30817

- How I did it

- How to verify it

$ go run ./contrib/client-with-cli-config/main.go push example.com/foo/bar
$ go run ./contrib/client-with-cli-config/main.go pull example.com/foo/bar

- Description for the changelog

expose config and credential fuhnctionalities to 3rd parties

- A picture of a cute animal (not mandatory but encouraged)

Signed-off-by: Daniel Nephin <dnephin@docker.com>

AkihiroSuda · 2017-06-15T06:00:24Z

@dnephin @vieux @vdemeester @jdgiotta

Example: contrib/client-with-cli-config (but without whole cli) See also moby/moby#30817 Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

codecov-io · 2017-06-15T07:38:12Z

Codecov Report

Merging #197 into master will increase coverage by 1.11%.
The diff coverage is 27.18%.

@@ Coverage Diff @@## master #197 +/- ##
==========================================
+ Coverage 45.67% 46.79% +1.11% 
==========================================
Files 171 165 -6 Lines 11513 11231 -282 ==========================================
- Hits 5259 5255 -4 + Misses 5947 5670 -277 + Partials 307 306 -1

boaz0

LGTM!

vdemeester

LGTM 🐮

dnephin

I think the "fix unintentional change" and "fix CI failure" commits can be squashed to another commit.

dnephin · 2017-06-15T17:36:58Z

 	"testing"

 	"github.com/pkg/errors"
 	"github.com/stretchr/testify/assert"
 	"golang.org/x/net/context"

-	// Prevents a circular import with "github.com/docker/cli/cli/internal/test"
-	. "github.com/docker/cli/cli/command"
-	"github.com/docker/cli/cli/internal/test"


dnephin · 2017-06-15T17:47:19Z

+)
+
+// ElectAuthServer returns the default registry to use (by asking the daemon)
+func ElectAuthServer(ctx context.Context, c client.APIClient) (string, []error, error) {


I noticed you added an error return value, but it seems to always be nil. Can we remove it?

dnephin · 2017-06-15T17:48:31Z

@@ -0,0 +1,85 @@
+// demonstration of using REST API client with cli config (push/pull with credential)


Maybe this could be a in a gist or a separate example repo that we can link to from docs?

dnephin · 2017-06-15T17:57:04Z

+	if err != nil {
+ return err
+	}
+	encodedAuth, err := registry.EncodeAuthToBase64(authConfig)


I notice this code seems to be duplicated a bunch of times. I looked through the use of authConfigs and it seems that in almost every single case we either immediately encode them, or in a few cases we pass it around a bit, then encode it later on.
I'd like to suggest a refactoring which will remove this duplication. Since we're making this change to expose an interface I think it's probably the right time to fix this, as changing it later will break the interface.
Instead of running these 3 returns values, I propose we return a new AuthConfig struct (as noted elsewhere I believe that err is unnecessary and can be removed).
In registry/registry.go:
typeAuthConfigstruct { config types.AuthConfigwarnings []string } func (a*AuthConfig) PrintWarnings(out io.Writer) { ...// the for loop and fmt.Printf() } func (s*AuthConfig) Encode() (string, error) { returnEncodeAuthToBase64(s.config) } func (s*AuthConfig) Config() types.AuthConfig { returns.config }
Then consuming code could call:
authConfig:=registry.ResolveAuthConfig(ctx, dockerCli.Client(), dockerCli.ConfigFile(), repoInfo.Index) authConfig.PrintWarnings(dockerCli.Err()) encodedAuth, err:=authConfig.Encode() iferr!=nil { returnerr }
Or it could pass around the authConfig and worry about encoding later.
WDYT?

dnephin · 2017-06-15T18:00:53Z

+
+// GetAllCredentials returns all of the credentials stored in all of the
+// configured credential stores.
+func GetAllCredentials(configFile *configfile.ConfigFile) (map[string]types.AuthConfig, error) {


I don't see this used. Is the code still using the old function?

dnephin · 2017-06-15T18:02:09Z

+// CredentialsStore returns a new credentials store based
+// on the settings provided in the configuration file. Empty string returns
+// the default credential store.
+func CredentialsStore(configFile *configfile.ConfigFile, serverAddress string) credentials.Store {


same here, not used?

dnephin · 2017-06-15T18:02:39Z

@@ -0,0 +1,56 @@
+package registry


I think this file belongs in config/. It does return registry types, but it's operating on the config file more than anything else.

dnephin · 2017-06-21T19:13:37Z

I need this change to support unit testing of some build functions. I'm going to resolve the conflicts and address my comments so I can cherry-pick these commits.

dnephin · 2017-06-21T21:29:48Z

I addressed the config part in #227 I haven't touched the registry functions yet

AkihiroSuda · 2017-06-23T03:13:34Z

Thank you, seems I should close this?

[17.06] vndr libnetwork to bring in fix for overlay network ip reuse

Move config file loading to more appropriate packages.
787aa69
Signed-off-by: Daniel Nephin <dnephin@docker.com>

GordonTheTurtle added the status/0-triage label Jun 15, 2017

AkihiroSuda mentioned this pull request Jun 15, 2017
client: export config and credential functionalities to 3rd parties moby/moby#30817
Closed

AkihiroSuda added 2 commits June 15, 2017 06:18

expose config and credential functionalities to 3rd parties
4615cd8
Example: contrib/client-with-cli-config (but without whole cli) See also moby/moby#30817 Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

cli/cli/{config, registry} -> cli/{config, registry}
29138da
Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

AkihiroSuda force-pushed the config branch from 56685a3 to 29138daCompare June 15, 2017 06:20

AkihiroSuda changed the title ~~expose config and credential fuhnctionalities to 3rd parties~~expose config and credential functionalities to 3rd partiesJun 15, 2017

AkihiroSuda added 2 commits June 15, 2017 06:32

fix unintentional change
3d15299
Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

fix CI failure
1747278
Signed-off-by: Akihiro Suda <suda.akihiro@lab.ntt.co.jp>

AkihiroSuda force-pushed the config branch from 96c941a to 1747278Compare June 15, 2017 07:25

boaz0 approved these changes Jun 15, 2017
View reviewed changes

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

vdemeester approved these changes Jun 15, 2017
View reviewed changes

dnephin suggested changes Jun 15, 2017
View reviewed changes

AkihiroSuda closed this Jun 23, 2017

nobiit pushed a commit to nobidev/docker-cli that referenced this pull request Nov 19, 2025
Merge pull requestdocker#197from andrewhsu/ven-ln
99a2732
[17.06] vndr libnetwork to bring in fix for overlay network ip reuse

		@@ -0,0 +1,85 @@
		// demonstration of using REST API client with cli config (push/pull with credential)

Uh oh!

Conversation

AkihiroSuda commented Jun 15, 2017

Uh oh!

AkihiroSuda commented Jun 15, 2017

Uh oh!

codecov-io commented Jun 15, 2017

Codecov Report

Uh oh!

boaz0 left a comment

Choose a reason for hiding this comment

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

dnephin left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephinJun 15, 2017

Choose a reason for hiding this comment

Uh oh!

dnephinJun 15, 2017

Choose a reason for hiding this comment

Uh oh!

dnephinJun 15, 2017

Choose a reason for hiding this comment

Uh oh!

dnephinJun 15, 2017

Choose a reason for hiding this comment

Uh oh!

dnephinJun 15, 2017

Choose a reason for hiding this comment

Uh oh!

dnephinJun 15, 2017

Choose a reason for hiding this comment

Uh oh!

dnephinJun 15, 2017

Choose a reason for hiding this comment

Uh oh!

dnephin commented Jun 21, 2017

Uh oh!

dnephin commented Jun 21, 2017

Uh oh!

AkihiroSuda commented Jun 23, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

dnephin left a comment •
edited
Loading