Add comma-separated --auto-accept support. #24146

johnharris85 · 2016-06-29T21:08:39Z

Addresses #24143, simplifying --auto-accept syntax for a better UX.

Add support for comma-separated --auto-accept value.

$ docker swarm update --auto-accept worker,manager.

Signed-off-by: John Harris john@johnharris.io

dnephin · 2016-06-29T21:10:25Z

If we're going to support all instead of --auto-accept worker --auto-accept manager I think we should make a much larger change to the AutoAcceptOption.

The logic of Set becomes a lot simpler if you can't set it more than once.

johnharris85 · 2016-07-02T18:36:22Z

I'm implementing the --auto-accept-add and --auto-accept-rm flags, but wondering how auto-acceptance should be handled on docker swarm init. Obviously add still works there but should rm error? Or just silently do nothing (as it looks like default behavior for init is going to be auto-accept = false for both anyway)?

dnephin · 2016-07-03T16:50:47Z

I guess on init it should actually just use the current behaviour

thaJeztah · 2016-07-06T08:21:17Z

I see the design discussion continued in the issue; #24143 (comment), so it looks like this PR needs to be updated

/cc @aaronlehmann @aluzzardi ?

johnharris85 · 2016-07-06T14:35:49Z

Ye @thaJeztah just waiting for the design stuff to be fleshed out over there before updating the PR.

aluzzardi · 2016-07-07T00:38:52Z

api/client/swarm/opts.go

@@ -112,6 +112,15 @@ func (o *AutoAcceptOption) Set(value string) error {
 			return fmt.Errorf("value NONE is incompatible with %s", value)
 		}
 		o.values[value] = true
+	case "ALL":
+		if accept, ok := o.values[WORKER]; ok && accept {
+			return fmt.Errorf("value NONE is incompatible with %s", WORKER)


This should just say value NONE is compatible with ALL

johnharris85 · 2016-07-07T23:07:46Z

Added support for comma-separated values --auto-accept worker,manager as well as all. Also backwards compatible with the current way of setting these flags. Added tests to reflect the changes.

cpuguy83 · 2016-07-08T19:46:19Z

I don't think all is a good idea.
Today we accept manager, worker and it's ok, but in the future we may have other types (storage?) where it makes less sense to support all

cpuguy83 · 2016-07-08T19:48:14Z

meh, on second thought it's probably fine.

johnharris85 · 2016-07-08T19:55:05Z

@cpuguy83 all is a UX feature for me. I think there is more refactoring to be done here (to make all future proof for example) but the current syntax --auto-accept worker --auto-accept manager is too long-hand IMO. The --auto-accept manager,worker syntax is not currently supported (this PR adds it).

all makes even more sense when considering the potential for future options. If I'm doing some local experiments / development and I need to type --auto-accept n where n is maybe 4 or 5 options. That's going to get old pretty quickly :p

thaJeztah · 2016-07-08T20:21:01Z

all makes even more sense when considering the potential for future options.

I'm not sure we should develop for possible future options. What if the new future option is something that should be an "opt-in"? If someone set all (because: lazy), we could have a potential security issue. Better to keep it explicit, and have people update their config to allow the new type than to allow it without them knowing.

johnharris85 · 2016-07-08T20:28:41Z

Maybe just allowing a comma-separated syntax is better then, and removing all? Would give a better UX shorthand instead of multiple --auto-accept flags but also preserve the security aspect?

aaronlehmann · 2016-07-08T20:29:28Z

@johnharris85: I think that's fine.

thaJeztah · 2016-07-08T20:31:20Z

Thanks! We can always add all back in future if we think it's ok; removing it is more troublesome once it's there

johnharris85 · 2016-07-08T21:29:46Z

all removed and refactored some of the none checking logic to make it simpler. Tests amended.

aaronlehmann · 2016-07-09T00:30:01Z

api/client/swarm/opts.go

@@ -18,6 +18,8 @@ const (
 	WORKER = "WORKER"
 	// MANAGER constant for manager name
 	MANAGER = "MANAGER"
+	// NONE constant
+	NONE = "NONE"


Not strictly related to your change, but could you please change these constants to CamelCase? Identifiers should never be all-caps in Golang. I'm surprised golint doesn't complain about this.

https://golang.org/doc/effective_go.html#mixed-caps
https://github.com/golang/go/wiki/CodeReviewComments#mixed-caps

Also, they should not be exported. And then we can drop the silly comments.

They're being used in opts_test.go. So either they stay exported (but CamelCase) or we hard code the values into all the test cases?

opts_test.go is in the same package, so they don't need to be exported.

Right, thanks.

aaronlehmann · 2016-07-09T00:32:24Z

Can you please update the PR summary and description to reflect the changes from the review, and squash the commits?

aaronlehmann · 2016-07-09T00:39:34Z

api/client/swarm/opts.go

+		switch value {
+		case "", NONE:
+			// NONE must stand alone, so if any non-NONE setting exists, conflict
+			if !o.values[NONE] && len(o.values) > 0 {


I'd suggest moving this check outside of the loop, with something like:

if o.values[NONE] && len(o.values) > 1 { ... }

Among other things, this will give a more consistent error message that won't depend on the order of the values.

I actually don't see the value in specifying the explicit conflict, can't any error (where there is none + n) return with something like "value NONE cannot be specified alongside other node types" ?

Error then consistent across all and simplifies this logic.

Move to above line#105? Set is only called once though (and o.values is always empty until we set something in it) so that check will never eval to true right?

Agreed, I don't see the value in specifying the conflict.

I was thinking this could move below the loop, so it inspects o.values after the map is populated.

Ah OK I get it. Ye can do, but with the standardizing of the error now does this have any other main benefits? I guess you could argue that error is checking the flag set as a whole, or that it's part of the none case. No strong feeling either way really.

I think it makes most sense for it to be below the loop because otherwise you have to check for conflicts both in the none case (in case none comes after a different value) and in the other cases (in case they come after none). If the check is below the loop, only a single check is needed.

aaronlehmann · 2016-07-11T15:08:40Z

api/client/swarm/opts.go

-	for key, value := range o.values {
-		keys = append(keys, fmt.Sprintf("%s=%v", strings.ToLower(key), value))
+	for key := range o.values {
+		keys = append(keys, fmt.Sprintf("%s=%v", strings.ToLower(key), true))


Just noticed this: should this be a list of values, instead of x=true, y=true? There is no =false, so the =true seems redundant.

Ye I thought that too, maybe just for readability / clarity (for repr), although ye, not strictly required as it's true if it exists by default. Happy to change but think it's clearer to stay. No strong preference either way though.

Okay, if you decide to keep the current format, can you change the Sprintf call to fmt.Sprintf("%s=true", strings.ToLower(key))?

aaronlehmann · 2016-07-11T17:01:25Z

api/client/swarm/opts.go

+	for _, value := range strings.Split(acceptValues, ",") {
+		value = strings.ToUpper(value)
+		switch value {
+		case "", none:


Sorry, one final comment. I'm wondering it makes sense to treat "" the same as none. It would mean that worker,,manager is parsed as worker,none,manager. I guess the intent here is to treat --auto-accept "" as --auto-accept none. If that's the case, maybe it would be better to do it at the very beginning of this function:

if acceptValues == "" { o.values[none] = struct{}{} } else { for _, value := range strings.Split(acceptValues, ",") { ... } }

Good point, but then worker,,manager would trigger the default case (and maybe it should)? Or in addition to the above change, should a case be added for an empty string inside the loop to just ignore it?

I'm not sure, but either sounds reasonable to me. Triggering an error in that case (by hitting the default case) is probably the safest approach.

And ye, I guess that was the original intent (that code existed before my PR) but I'm not sure whether I agree with it. I think a blank --auto-accept should error or pass silently, not set to none. That doesn't seem intuitive behaviour. Perhaps it was done for security reasons? In case someone set it accidentally but didn't pass anything, default to none. That may have made sense before, but aren't the defaults changing to false for both manager and worker anyway?

And ye, I guess that was the original intent (that code existed before my PR) but I'm not sure whether I agree with it. I think a blank --auto-accept should error or pass silently, not set to none.

Makes sense.

That doesn't seem intuitive behaviour. Perhaps it was done for security reasons? In case someone set it accidentally but didn't pass anything, default to none.

Not really sure that makes sense. It's hard to accidentally pass an empty string as an argument.

That may have made sense before, but aren't the defaults changing to false for both manager and worker anyway?

The autoaccept defaults aren't changing. What changed recently on master is that by default, a secret token is needed to join a swarm.

aaronlehmann · 2016-07-11T20:30:43Z

Code LGTM

Can you please update the commit message and pull request description to summarize the final state of the changes?

This will also need updates to the docs before merging.

Thanks!

Signed-off-by: John Harris <john@johnharris.io>

cpuguy83 · 2016-07-12T01:47:10Z

LGTM

cpuguy83 · 2016-07-12T01:49:29Z

ping @thaJeztah

thaJeztah · 2016-07-12T14:29:14Z

Thanks! docs LGTM

aaronlehmann · 2016-07-12T19:16:47Z

docs/reference/commandline/swarm_init.md

@@ -66,6 +66,13 @@ For example, the following initializes a cluster with auto-acceptance of workers
 $ docker swarm init --listen-addr 192.168.99.121:2377 --auto-accept worker
 ```

+It is possible to pass a comma-separated list of node types. The following initializes a cluster
+with auto-acceptance of both `worker` and `manager` nodes


I think none needs to be mentioned somewhere in the docs.

@aaronlehmann good point; just created #24552

aluzzardi · 2016-07-12T21:14:26Z

Ping @icecrime

GordonTheTurtle added status/0-triage dco/no Automatically set by a bot when one of the commits lacks proper signature labels Jun 29, 2016

johnharris85 force-pushed the fix-swarm-update-auto-accept branch from dca2659 to 083c78e Compare June 29, 2016 21:15

GordonTheTurtle removed the dco/no Automatically set by a bot when one of the commits lacks proper signature label Jun 29, 2016

johnharris85 mentioned this pull request Jun 30, 2016

Swarm update does not allow both workers and managers to be auto-accepted #24143

Closed

thaJeztah added status/1-design-review and removed status/0-triage labels Jul 6, 2016

thaJeztah added this to the 1.12.0 milestone Jul 6, 2016

aluzzardi reviewed Jul 7, 2016
View reviewed changes

johnharris85 force-pushed the fix-swarm-update-auto-accept branch from 083c78e to 1f0ff67 Compare July 7, 2016 23:03

johnharris85 force-pushed the fix-swarm-update-auto-accept branch from 1f0ff67 to 600f60d Compare July 8, 2016 21:27

aaronlehmann reviewed Jul 9, 2016
View reviewed changes

johnharris85 force-pushed the fix-swarm-update-auto-accept branch 2 times, most recently from cde7bdd to f87ca41 Compare July 9, 2016 19:25

johnharris85 changed the title ~~Add 'all' option to swarm update --auto-accept.~~ Add comma-separated --auto-accept support. Jul 9, 2016

aaronlehmann reviewed Jul 11, 2016
View reviewed changes

johnharris85 force-pushed the fix-swarm-update-auto-accept branch from f87ca41 to c3d978a Compare July 11, 2016 16:10

aaronlehmann reviewed Jul 11, 2016
View reviewed changes

aaronlehmann added status/2-code-review and removed status/1-design-review labels Jul 11, 2016

johnharris85 force-pushed the fix-swarm-update-auto-accept branch from c3d978a to de0d213 Compare July 11, 2016 20:13

johnharris85 force-pushed the fix-swarm-update-auto-accept branch from de0d213 to ad57cf3 Compare July 11, 2016 20:45

Add support for comma-separated --auto-accept syntax.

8e14882

Signed-off-by: John Harris <john@johnharris.io>

johnharris85 force-pushed the fix-swarm-update-auto-accept branch from ad57cf3 to 8e14882 Compare July 11, 2016 22:47

cpuguy83 added status/4-merge process/cherry-pick status/3-docs-review and removed status/2-code-review status/4-merge labels Jul 12, 2016

thaJeztah merged commit 7da11b1 into moby:master Jul 12, 2016

aaronlehmann reviewed Jul 12, 2016
View reviewed changes

thaJeztah mentioned this pull request Jul 12, 2016

Add "auto-accept=none" to documentation #24552

Merged

tiborvass mentioned this pull request Jul 12, 2016

[WIP] Cherry picks 1.12.0 rc4 #24467

Merged

tiborvass added process/cherry-picked and removed process/cherry-pick labels Jul 12, 2016

Add comma-separated --auto-accept support. #24146

Add comma-separated --auto-accept support. #24146

Conversation

johnharris85 commented Jun 29, 2016 • edited

dnephin commented Jun 29, 2016

johnharris85 commented Jul 2, 2016

dnephin commented Jul 3, 2016

thaJeztah commented Jul 6, 2016

johnharris85 commented Jul 6, 2016

Choose a reason for hiding this comment

johnharris85 commented Jul 7, 2016

cpuguy83 commented Jul 8, 2016

cpuguy83 commented Jul 8, 2016

johnharris85 commented Jul 8, 2016

thaJeztah commented Jul 8, 2016

johnharris85 commented Jul 8, 2016

aaronlehmann commented Jul 8, 2016

thaJeztah commented Jul 8, 2016

johnharris85 commented Jul 8, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlehmann commented Jul 9, 2016

Choose a reason for hiding this comment

johnharris85 Jul 9, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

johnharris85 Jul 11, 2016 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aaronlehmann commented Jul 11, 2016

cpuguy83 commented Jul 12, 2016

cpuguy83 commented Jul 12, 2016

thaJeztah commented Jul 12, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aluzzardi commented Jul 12, 2016

johnharris85 commented Jun 29, 2016 •

edited

johnharris85 Jul 9, 2016 •

edited

johnharris85 Jul 11, 2016 •

edited