Use non-detached mode as default for service commands by thaJeztah · Pull Request #525 · docker/cli

thaJeztah · 2017-09-12T12:41:26Z

- What I did

Commit 330a003 added a --detach=false option
to various service-related commands, with the intent to make this the default in
a future version (17.09).

This patch changes the default to use "interactive" (non-detached), allowing
users to override this by setting the --detach option.

To prevent problems when connecting to older daemon versions (17.05 and below,
see commit db60f25), the detach option is
ignored for those versions, and detach is always true.

Before this change, a warning was printed to announce the upcoming default:

$ docker service create nginx:alpine
saxiyn3pe559d753730zr0xer
Since --detach=false was not specified, tasks will be created in the background.
In a future release, --detach=false will become the default.

After this change, no warning is printed, but --detach is disabled;

$ docker service create nginx:alpine
y9jujwzozi0hwgj5yaadzliq6
overall progress: 1 out of 1 tasks
1/1: running   [==================================================>]
verify: Service converged

Setting the --detach flag makes the cli use the pre-17.06 behavior:

$ docker service create --detach nginx:alpine
280hjnzy0wzje5o56gr22a46n

Running against a 17.03 daemon, without specifying the --detach flag;

$ docker service create nginx:alpine
kqheg7ogj0kszoa34g4p73i8q

- Description for the changelog

Use interactive (non-detached) mode for `service create`, `service update`, `service rollback`, and `service scale` by default

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

ping @vdemeester @aaronlehmann @dnephin

vdemeester

LGTM 🐯

thaJeztah · 2017-09-12T12:48:33Z

cli/command/service/rollback.go


-	if options.detach {
-		warnDetachDefault(dockerCli.Err(), apiClient.ClientVersion(), flags, "rolled back")
+	if options.detach || versions.LessThan(apiClient.ClientVersion(), "1.29") {


One thing I was thinking is if we should put the version-check somewhere central, and just change options.detach to true, based on that version; this would prevent possible bugs if more commands get an interactive variant.

I don't think we need the version check anymore. The behaviour is client-side only, right? So even if they are running against an older daemon we should keep the same default for detach. I think we can remove the version check.

I wasn't sure if older daemons would return the right response for the waitOnService() to work correctly; if that's not an issue then I am +1 on removing

Apparently it depends on this _up-to-date=true filter which was added in 17.05, so I guess it does depend on this version still.

thaJeztah · 2017-09-13T10:22:49Z

Looks like there's a lint warning; I'll have a look at those;

cli/command/service/rollback.go:35:41:warning: parameter flags is unused (unparam)
cli/command/service/scale.go:58:38:warning: parameter flags is unused (unparam)

Commit 330a003 added a `--detach=false` option to various service-related commands, with the intent to make this the default in a future version (17.09). This patch changes the default to use "interactive" (non-detached), allowing users to override this by setting the `--detach` option. To prevent problems when connecting to older daemon versions (17.05 and below, see commit db60f25), the detach option is ignored for those versions, and detach is always true. Before this change, a warning was printed to announce the upcoming default: $ docker service create nginx:alpine saxiyn3pe559d753730zr0xer Since --detach=false was not specified, tasks will be created in the background. In a future release, --detach=false will become the default. After this change, no warning is printed, but `--detach` is disabled; $ docker service create nginx:alpine y9jujwzozi0hwgj5yaadzliq6 overall progress: 1 out of 1 tasks 1/1: running [==================================================>] verify: Service converged Setting the `--detach` flag makes the cli use the pre-17.06 behavior: $ docker service create --detach nginx:alpine 280hjnzy0wzje5o56gr22a46n Running against a 17.03 daemon, without specifying the `--detach` flag; $ docker service create nginx:alpine kqheg7ogj0kszoa34g4p73i8q Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

codecov-io · 2017-09-13T10:28:15Z

Codecov Report

Merging #525 into master will decrease coverage by <.01%.
The diff coverage is 44.44%.

@@            Coverage Diff             @@
##           master     #525      +/-   ##
==========================================
- Coverage   49.06%   49.05%   -0.01%     
==========================================
  Files         200      200              
  Lines       16407    16397      -10     
==========================================
- Hits         8050     8044       -6     
+ Misses       7938     7934       -4     
  Partials      419      419

thaJeztah · 2017-09-13T10:29:18Z

fixed linting error, PTAL;

diff --git a/cli/command/service/rollback.go b/cli/command/service/rollback.go
index da8c1fad..375e6d51 100644
--- a/cli/command/service/rollback.go
+++ b/cli/command/service/rollback.go
@@ -9,7 +9,6 @@ import (
        "github.com/docker/docker/api/types"
        "github.com/docker/docker/api/types/versions"
        "github.com/spf13/cobra"
-       "github.com/spf13/pflag"
 )

 func newRollbackCommand(dockerCli command.Cli) *cobra.Command {
@@ -20,7 +19,7 @@ func newRollbackCommand(dockerCli command.Cli) *cobra.Command {
                Short: "Revert changes to a service's configuration",
                Args:  cli.ExactArgs(1),
                RunE: func(cmd *cobra.Command, args []string) error {
-                       return runRollback(dockerCli, cmd.Flags(), options, args[0])
+                       return runRollback(dockerCli, options, args[0])
                },
                Tags: map[string]string{"version": "1.31"},
        }
@@ -32,7 +31,7 @@ func newRollbackCommand(dockerCli command.Cli) *cobra.Command {
        return cmd
 }

-func runRollback(dockerCli command.Cli, flags *pflag.FlagSet, options *serviceOptions, serviceID string) error {
+func runRollback(dockerCli command.Cli, options *serviceOptions, serviceID string) error {
        apiClient := dockerCli.Client()
        ctx := context.Background()

diff --git a/cli/command/service/scale.go b/cli/command/service/scale.go
index 5ab083a1..d38b01b4 100644
--- a/cli/command/service/scale.go
+++ b/cli/command/service/scale.go
@@ -13,7 +13,6 @@ import (
        "github.com/docker/docker/api/types/versions"
        "github.com/pkg/errors"
        "github.com/spf13/cobra"
-       "github.com/spf13/pflag"
 )

 type scaleOptions struct {
@@ -28,7 +27,7 @@ func newScaleCommand(dockerCli command.Cli) *cobra.Command {
                Short: "Scale one or multiple replicated services",
                Args:  scaleArgs,
                RunE: func(cmd *cobra.Command, args []string) error {
-                       return runScale(dockerCli, cmd.Flags(), options, args)
+                       return runScale(dockerCli, options, args)
                },
        }

@@ -55,7 +54,7 @@ func scaleArgs(cmd *cobra.Command, args []string) error {
        return nil
 }

-func runScale(dockerCli command.Cli, flags *pflag.FlagSet, options *scaleOptions, args []string) error {
+func runScale(dockerCli command.Cli, options *scaleOptions, args []string) error {
        var errs []string
        var serviceIDs []string
        ctx := context.Background()

dnephin

LGTM

thaJeztah requested review from mdlinville and vdemeester as code owners September 12, 2017 12:41

GordonTheTurtle added the status/0-triage label Sep 12, 2017

vdemeester added status/1-design-review and removed status/0-triage labels Sep 12, 2017

vdemeester approved these changes Sep 12, 2017

View reviewed changes

thaJeztah commented Sep 12, 2017

View reviewed changes

mdlinville approved these changes Sep 12, 2017

View reviewed changes

aaronlehmann approved these changes Sep 13, 2017

View reviewed changes

thaJeztah force-pushed the change-detach-default branch from 8ef7406 to 0c27355 Compare September 13, 2017 10:28

thaJeztah added status/2-code-review and removed status/1-design-review labels Sep 13, 2017

dnephin approved these changes Sep 13, 2017

View reviewed changes

dnephin merged commit e5aeb4b into docker:master Sep 13, 2017

thaJeztah deleted the change-detach-default branch September 13, 2017 20:38

thaJeztah mentioned this pull request Sep 18, 2017

[17.09] Use non-detached mode as default for service commands docker-archive/docker-ce#230

Closed

seemethere mentioned this pull request Sep 27, 2017

Explicitly add --detach to service CLI calls moby/moby#35007

Merged

thaJeztah added impact/changelog impact/deprecation impact/documentation labels Oct 14, 2017

thaJeztah added this to the 17.10.0 milestone Feb 19, 2020

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Use non-detached mode as default for service commands#525

Use non-detached mode as default for service commands#525
dnephin merged 1 commit intodocker:masterfrom
thaJeztah:change-detach-default

thaJeztah commented Sep 12, 2017

Uh oh!

vdemeester left a comment

Uh oh!

thaJeztah Sep 12, 2017

Uh oh!

dnephin Sep 12, 2017

Uh oh!

thaJeztah Sep 12, 2017

Uh oh!

dnephin Sep 12, 2017

Uh oh!

thaJeztah commented Sep 13, 2017

Uh oh!

codecov-io commented Sep 13, 2017 •

edited

Loading

Uh oh!

thaJeztah commented Sep 13, 2017

Uh oh!

dnephin left a comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

Conversation

thaJeztah commented Sep 12, 2017

Uh oh!

vdemeester left a comment

Choose a reason for hiding this comment

Uh oh!

thaJeztah Sep 12, 2017

Choose a reason for hiding this comment

Uh oh!

dnephin Sep 12, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah Sep 12, 2017

Choose a reason for hiding this comment

Uh oh!

dnephin Sep 12, 2017

Choose a reason for hiding this comment

Uh oh!

thaJeztah commented Sep 13, 2017

Uh oh!

codecov-io commented Sep 13, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

thaJeztah commented Sep 13, 2017

Uh oh!

dnephin left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

codecov-io commented Sep 13, 2017 •

edited

Loading