hide `--detach` for docker < 17.05 #219

vieux · 2017-06-21T01:03:05Z

1st I added --detach to docker service scale
then while testing I realized that using docker 17.05 with a 17.03 client doesn't work with --detach=true since it's using an unknown filter to 17.03. so I hid the flag (as it should have been)

vieux · 2017-06-21T01:12:02Z

Here is the current behavior:

$docker service update --replicas=8 test
test
Since --detach=false was not specified, tasks will be updated in the background.
In a future release, --detach=false will become the default.

When adding the flag as suggested:

$docker service update --detach=false --replicas=8 test
test
Error response from daemon: Invalid filter '_up-to-date'

vieux · 2017-06-21T01:12:10Z

ping @thaJeztah

codecov-io · 2017-06-21T01:13:08Z

Codecov Report

Merging #219 into master will increase coverage by 0.03%.
The diff coverage is 55%.

@@            Coverage Diff             @@
##           master     #219      +/-   ##
==========================================
+ Coverage   46.81%   46.85%   +0.03%     
==========================================
  Files         172      172              
  Lines       11686    11689       +3     
==========================================
+ Hits         5471     5477       +6     
+ Misses       5903     5900       -3     
  Partials      312      312

thaJeztah · 2017-06-21T01:20:31Z

cli/command/service/scale.go

+
+		if options.detach {
+			if !flags.Changed("detach") && versions.GreaterThanOrEqualTo(dockerCli.Client().ClientVersion(), "1.29") {
+				fmt.Fprintln(dockerCli.Err(), "Since --detach=false was not specified, tasks will be scaled in the background.\n"+


Should we add a global definition of this error somewhere?

Hm; also (this is gonna be tricky); the warning should no longer be printed if the client talks to docker 17.09 (or whichever version changes the default)

This sentence is slightly different from the create one: "scaled" vs "updated" :d

I agree 17.09 is probably a good time to do the switch. I'll work on a config file key for it in the meantime.

Instead of defining the error in a global we should make this a function and pass in the different string as an arg.

func warnDetachDefault(dockerCli command.Cli, flags pflag.FlatSet, msg string) { ... }

We can use that function from all 3 locations, and it would be easy to test.

dnephin · 2017-06-21T17:01:38Z

cli/command/service/scale.go

 )

 func newScaleCommand(dockerCli *command.DockerCli) *cobra.Command {
-	return &cobra.Command{
+	options := newServiceOptions()


This shouldn't use service options (which is a huge struct).

We should create a new scaleOptions struct which would include just detach for now.

dnephin · 2017-06-21T17:04:24Z

cli/command/service/scale.go

+
+		if options.detach {
+			if !flags.Changed("detach") && versions.GreaterThanOrEqualTo(dockerCli.Client().ClientVersion(), "1.29") {
+				fmt.Fprintln(dockerCli.Err(), "Since --detach=false was not specified, tasks will be scaled in the background.\n"+


Instead of defining the error in a global we should make this a function and pass in the different string as an arg.

func warnDetachDefault(dockerCli command.Cli, flags pflag.FlatSet, msg string) { ... }

We can use that function from all 3 locations, and it would be easy to test.

dnephin · 2017-06-21T17:08:57Z

cli/command/service/scale.go

+
+	flags := cmd.Flags()
+	flags.BoolVarP(&options.detach, "detach", "d", true, "Exit immediately instead of waiting for the service to converge")
+	flags.SetAnnotation("detach", "version", []string{"1.29"})


We could move these two lines to an addDetachFlag() function and call it from addServiceFlags() (and here).

We should also create a constant flagDetach like we have for a bunch of the other flags in service/opts.go

thaJeztah · 2017-06-24T00:34:22Z

docs/reference/commandline/service_scale.md


 Scale one or multiple replicated services

 Options:
+  -d, --detach                             Exit immediately instead of waiting for the service to converge (default true)


Can you also add it to the completion scripts, or want someone to take care of that in a follow-up?

dnephin

I wouldn't worry about the rest of the test coverage for this change. The coverage should come when unit tests are written for the service commands.

dnephin · 2017-06-26T03:16:00Z

cli/command/service/scale.go

@@ -59,6 +68,15 @@ func runScale(dockerCli *command.DockerCli, args []string) error {
 		if err := runServiceScale(dockerCli, serviceID, scale); err != nil {


runServiceScale should accept this new ctx, so we don't create another one

dnephin · 2017-06-26T03:17:00Z

cli/command/service/opts.go

@@ -453,6 +453,10 @@ func convertExtraHostsToSwarmHosts(extraHosts []string) []string {
 	return hosts
 }

+type scaleOptions struct {
+	detach bool
+}


minor, but this struct should go in scale.go since it's only used in that one file. serviceOptions is a rare case where there are a lot of options shared between multiple commands.

dnephin · 2017-06-26T03:23:03Z

cli/command/service/helpers_test.go

+	for _, test := range tests {
+		out := new(bytes.Buffer)
+		cli.SetErr(out)
+		detach = test.detach


I don't think this is triggering flags.Changed(). You can use flags.Lookup(flagDetach).Changed = ...

There's one more test case here, isn't there? {true, "1.29", false), which I think will fail until ^ is fixed

vieux · 2017-06-27T01:28:12Z

split the PR in two, here is the bugfix/refactor, new feature in #243

Signed-off-by: Victor Vieux <[email protected]>

thaJeztah

LGTM

tiborvass · 2017-06-27T03:47:28Z

LGTM

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

thaJeztah reviewed Jun 21, 2017

View reviewed changes

dnephin added status/1-design-review and removed status/0-triage labels Jun 21, 2017

dnephin reviewed Jun 21, 2017

View reviewed changes

thaJeztah reviewed Jun 24, 2017

View reviewed changes

thaJeztah added status/2-code-review and removed status/1-design-review labels Jun 24, 2017

thaJeztah added this to the 17.06.1 milestone Jun 24, 2017

dnephin reviewed Jun 26, 2017

View reviewed changes

vieux changed the title ~~add --detach to docker service scale and fix 17.05 -> 17.03~~ hide --detach for docker < 17.05 Jun 27, 2017

vieux mentioned this pull request Jun 27, 2017

add --detach to docker scale #243

Merged

docker deleted a comment from thaJeztah Jun 27, 2017

hide --detach for docker < 17.05

db60f25

Signed-off-by: Victor Vieux <[email protected]>

thaJeztah approved these changes Jun 27, 2017

View reviewed changes

tiborvass merged commit c3e5445 into docker:master Jun 27, 2017

vieux deleted the scale branch June 27, 2017 16:51

seemethere mentioned this pull request Jun 28, 2017

hide --detach for docker < 17.05 seemethere/cli#1

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

hide `--detach` for docker < 17.05 #219

hide `--detach` for docker < 17.05 #219

vieux commented Jun 21, 2017

vieux commented Jun 21, 2017

vieux commented Jun 21, 2017

codecov-io commented Jun 21, 2017 •

edited

Loading

thaJeztah Jun 21, 2017

thaJeztah Jun 21, 2017

vieux Jun 21, 2017

dnephin Jun 21, 2017

dnephin Jun 21, 2017

dnephin Jun 21, 2017

dnephin Jun 21, 2017

thaJeztah Jun 24, 2017

dnephin left a comment

dnephin Jun 26, 2017

dnephin Jun 26, 2017

dnephin Jun 26, 2017

vieux commented Jun 27, 2017

thaJeztah left a comment

tiborvass commented Jun 27, 2017

		@@ -59,6 +68,15 @@ func runScale(dockerCli *command.DockerCli, args []string) error {
		if err := runServiceScale(dockerCli, serviceID, scale); err != nil {

hide --detach for docker < 17.05 #219

hide --detach for docker < 17.05 #219

Conversation

vieux commented Jun 21, 2017

vieux commented Jun 21, 2017

vieux commented Jun 21, 2017

codecov-io commented Jun 21, 2017 • edited Loading

Codecov Report

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

dnephin left a 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

vieux commented Jun 27, 2017

thaJeztah left a comment

Choose a reason for hiding this comment

tiborvass commented Jun 27, 2017

hide `--detach` for docker < 17.05 #219

hide `--detach` for docker < 17.05 #219

codecov-io commented Jun 21, 2017 •

edited

Loading