Add support for configs to compose format #83

cpuguy83 · 2017-05-15T15:22:03Z

Ping @aaronlehmann @vdemeester @dnephin

cpuguy83 · 2017-05-15T15:41:30Z

cli/compose/loader/loader.go:44::warning: cyclomatic complexity 22 of function Load() is high (> 19) (gocyclo)

I didn't really add any extra complexity here.

dnephin · 2017-05-15T16:22:25Z

cc @shin- since this affects docker-compose

dnephin · 2017-05-15T16:25:20Z

I didn't really add any extra complexity here.

There are 3 new if statements which means the complexity increased by 3.

I have a fix for this function in #61: https://github.com/docker/cli/pull/61/files#diff-4051eff50243f2cbcfa7172b42568ee0 which also removes a bunch of the duplication.

You could grab that fix here, don't worry about cherry-picking it.

dnephin

This generally looks correct

dnephin · 2017-05-15T16:27:07Z

cli/command/stack/deploy_composefile.go

+		secret, _, err := client.ConfigInspectWithRaw(ctx, configSpec.Name)
+		if err == nil {
+			// secret already exists, then we update that
+			if err := client.ConfigUpdate(ctx, secret.ID, secret.Meta.Version, configSpec); err != nil {


s/secret/config/

The variable above here, the comment, and the comment below this as well

dnephin · 2017-05-15T16:29:42Z

cli/command/stack/deploy_composefile.go

+			if err := client.ConfigUpdate(ctx, secret.ID, secret.Meta.Version, configSpec); err != nil {
+				return err
+			}
+		} else if apiclient.IsErrSecretNotFound(err) {


There is a generic IsErrNotFound() we can use instead of this

dnephin · 2017-05-15T16:31:00Z

cli/compose/convert/service.go

+
+		configSpec, exists := configSpecs[config.Source]
+		if !exists {
+			return nil, errors.Errorf("undefined secret %q", config.Source)


s/secret/config/

dnephin · 2017-05-15T16:32:01Z

cli/compose/loader/loader.go

 	}
 }

+func transformServiceConfigObj(data interface{}) (interface{}, error) {


i think we can use the same function as secrets, since there is no type and the "string" version is the same?

dnephin · 2017-05-15T16:35:27Z

cli/compose/types/types.go

 }

+// ServiceConfigObjConfig is the config obj configuration for a service
+type ServiceConfigObjConfig struct {


Could we use this instead these actually diverge:

type ServiceConfigObjConfig ServiceSecretConfig

or

type ServiceConfigObjConfig struct { ServiceSecretConfig }

That might let us re-use some code as well?

And we could leave a comment about changing them when we need to

I'm not sure if this is worth the LOC saved.
These are separate objects that have totally different API's, use-cases, and in the future features.

I'd be more worried about real functional divergence getting automatically applied to both objects than accidental divergence in these types.

Are they that separate? At the very least they share 5 fields! Even as other fields are added, these 5 fields will still overlap.

If not this solution, how about a common struct for these fields that gets included in both?

I'm really worried about doing this.
They happen to share similar code because they currently provide vary similar functionality, but I consider this coincidental duplication.

I would rather see where/how secrets and configs take off before trying to de-dup.

I'd like to experiment with extracting a common type before we merge this. I can look at it later today or tomorrow.

If it doesn't significantly remove code, I'll be ok with it like this.

I see what you're saying about different features, but I think that's more significant for the backend. From the client, where all we're doing is reading a config and converting to other types, I expect the code to stay quite similar.

I've updated to use a shared struct for now.

dnephin · 2017-05-15T16:35:46Z

cli/compose/types/types.go

 }
+
+// ConfigObjConfig is the config for the swarm "Config" object
+type ConfigObjConfig struct {


same questions about aliasing this to SecretConfig

aaronlehmann · 2017-05-15T16:41:12Z

cli/command/stack/deploy_composefile.go

+			if err := client.ConfigUpdate(ctx, secret.ID, secret.Meta.Version, configSpec); err != nil {
+				return err
+			}
+		} else if apiclient.IsErrSecretNotFound(err) {


apiclient.IsErrConfigNotFound(err)

aaronlehmann · 2017-05-15T16:42:02Z

cli/compose/convert/compose_test.go

+func TestConfigs(t *testing.T) {
+	namespace := Namespace{name: "foo"}
+
+	configText := "this is the first secret"


not a secret

aaronlehmann · 2017-05-15T16:42:59Z

cli/compose/convert/service.go

+
+		configSpec, exists := configSpecs[config.Source]
+		if !exists {
+			return nil, errors.Errorf("undefined secret %q", config.Source)


undefined config %q

aaronlehmann · 2017-05-15T16:43:20Z

cli/command/stack/deploy_composefile.go

+	client := dockerCli.Client()
+
+	for _, configSpec := range configs {
+		secret, _, err := client.ConfigInspectWithRaw(ctx, configSpec.Name)


Change the variable name; it's not a secret

aaronlehmann · 2017-05-15T16:43:32Z

cli/command/stack/deploy_composefile.go

+	for _, configSpec := range configs {
+		secret, _, err := client.ConfigInspectWithRaw(ctx, configSpec.Name)
+		if err == nil {
+			// secret already exists, then we update that


Not a secret

aaronlehmann · 2017-05-15T16:43:54Z

cli/command/stack/deploy_composefile.go

+				return err
+			}
+		} else if apiclient.IsErrSecretNotFound(err) {
+			// secret does not exist, then we create a new one.


Not a secret

aaronlehmann · 2017-05-15T16:44:15Z

cli/compose/convert/compose_test.go

+	namespace := Namespace{name: "foo"}
+
+	configText := "this is the first secret"
+	secretFile := tempfile.NewTempFile(t, "convert-configs", configText)


Update the variable name

cpuguy83 · 2017-05-15T18:18:01Z

Updated and all green.

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

dnephin

LGTM

shin-

FWIW

aaronlehmann · 2017-05-16T21:26:02Z

LGTM

thaJeztah · 2017-05-16T21:29:16Z

Opened docker/docs#3290 for tracking docs

bump VERSION to 17.06.0-ce-rc5

Update to go 1.9.3 Upstream-commit: 7ea33ac7993e2abfd2404e147d95a3b41a29ccbe Component: packaging

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

cpuguy83 force-pushed the add_configs_to_compose branch 2 times, most recently from 5fab5a0 to f9c4c81 Compare May 15, 2017 15:32

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

dnephin reviewed May 15, 2017

View reviewed changes

aaronlehmann reviewed May 15, 2017

View reviewed changes

cpuguy83 force-pushed the add_configs_to_compose branch 2 times, most recently from d30fc41 to 6195a21 Compare May 15, 2017 17:50

cpuguy83 added this to the 17.06.0 milestone May 16, 2017

cpuguy83 force-pushed the add_configs_to_compose branch 2 times, most recently from e0e0854 to 223b42f Compare May 16, 2017 21:09

Add support for configs to compose format

e574286

Signed-off-by: Brian Goff <cpuguy83@gmail.com>

cpuguy83 force-pushed the add_configs_to_compose branch from 223b42f to e574286 Compare May 16, 2017 21:10

dnephin approved these changes May 16, 2017

View reviewed changes

shin- approved these changes May 16, 2017

View reviewed changes

aaronlehmann merged commit 37bf8ff into docker:master May 16, 2017

thaJeztah mentioned this pull request May 16, 2017

Update docker-compose documentation for service "configs" docker/docs#3290

Closed

thaJeztah added the impact/compose-file label Jul 20, 2017

nobiit pushed a commit to nobidev/docker-cli that referenced this pull request Nov 19, 2025

Merge pull request docker#83 from seemethere/increment_rc5

b7e4173

bump VERSION to 17.06.0-ce-rc5

nobiit pushed a commit to nobidev/docker-cli that referenced this pull request Nov 19, 2025

Merge pull request docker#83 from thaJeztah/bump-go-1.9.3

4d4d834

Update to go 1.9.3 Upstream-commit: 7ea33ac7993e2abfd2404e147d95a3b41a29ccbe Component: packaging

Add support for configs to compose format #83

Add support for configs to compose format #83

Uh oh!

Conversation

cpuguy83 commented May 15, 2017

Uh oh!

cpuguy83 commented May 15, 2017

Uh oh!

dnephin commented May 15, 2017

Uh oh!

dnephin commented May 15, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dnephin May 15, 2017 • 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!

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!

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!

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!

cpuguy83 commented May 15, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

shin- left a comment

Choose a reason for hiding this comment

Uh oh!

aaronlehmann commented May 16, 2017

Uh oh!

thaJeztah commented May 16, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

dnephin May 15, 2017 •

edited

Loading