New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 1 commit into from Sep 13, 2017

Conversation

@thaJeztah
Member

thaJeztah commented Sep 12, 2017

- 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)

maxresdefault

ping @vdemeester @aaronlehmann @dnephin

@vdemeester

LGTM 🐯

@@ -56,8 +57,7 @@ func runRollback(dockerCli command.Cli, flags *pflag.FlagSet, options *serviceOp
fmt.Fprintf(dockerCli.Out(), "%s\n", serviceID)
if options.detach {
warnDetachDefault(dockerCli.Err(), apiClient.ClientVersion(), flags, "rolled back")
if options.detach || versions.LessThan(apiClient.ClientVersion(), "1.29") {

This comment has been minimized.

@thaJeztah

thaJeztah Sep 12, 2017

Member

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.

@thaJeztah

thaJeztah Sep 12, 2017

Member

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.

This comment has been minimized.

@dnephin

dnephin Sep 12, 2017

Collaborator

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.

@dnephin

dnephin Sep 12, 2017

Collaborator

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.

This comment has been minimized.

@thaJeztah

thaJeztah Sep 12, 2017

Member

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

@thaJeztah

thaJeztah Sep 12, 2017

Member

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

This comment has been minimized.

@dnephin

dnephin Sep 12, 2017

Collaborator

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.

@dnephin

dnephin Sep 12, 2017

Collaborator

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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Sep 13, 2017

Member

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)
Member

thaJeztah commented Sep 13, 2017

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)
Use non-detached mode as default for service commands
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

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Sep 13, 2017

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

codecov-io commented Sep 13, 2017

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

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Sep 13, 2017

Member

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()
Member

thaJeztah commented Sep 13, 2017

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

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

7 of 8 checks passed

codecov/patch 44.44% of diff hit (target 50%)
Details
ci/circleci: cross Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: shellcheck Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: validate Your tests passed on CircleCI!
Details
codecov/project 49.05% (-0.01%) compared to f697de3
Details
dco-signed All commits are signed

@thaJeztah thaJeztah deleted the thaJeztah:change-detach-default branch Sep 13, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment