Skip to content
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.

Sign up for GitHub

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

Jump to bottom

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

Merged
merged 1 commit into from Sep 13, 2017
Merged

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

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
3 changes: 1 addition & 2 deletions cli/command/service/create.go
View file
Open in desktop
Expand Up @@ -123,8 +123,7 @@ func runCreate(dockerCli command.Cli, flags *pflag.FlagSet, opts *serviceOptions

fmt.Fprintf(dockerCli.Out(), "%s\n", response.ID)

if opts.detach {
warnDetachDefault(dockerCli.Err(), apiClient.ClientVersion(), flags, "created")
if opts.detach || versions.LessThan(apiClient.ClientVersion(), "1.29") {
return nil
}

Expand Down
11 changes: 0 additions & 11 deletions cli/command/service/helpers.go
View file
Open in desktop
@@ -1,15 +1,12 @@
package service

import (
"fmt"
"io"
"io/ioutil"

"github.com/docker/cli/cli/command"
"github.com/docker/cli/cli/command/service/progress"
"github.com/docker/docker/api/types/versions"
"github.com/docker/docker/pkg/jsonmessage"
"github.com/spf13/pflag"
"golang.org/x/net/context"
)

Expand All @@ -34,11 +31,3 @@ func waitOnService(ctx context.Context, dockerCli command.Cli, serviceID string,
}
return err
}

// warnDetachDefault warns about the --detach flag future change if it's supported.
func warnDetachDefault(err io.Writer, clientVersion string, flags *pflag.FlagSet, msg string) {
if !flags.Changed("detach") && versions.GreaterThanOrEqualTo(clientVersion, "1.29") {
fmt.Fprintf(err, "Since --detach=false was not specified, tasks will be %s in the background.\n"+
"In a future release, --detach=false will become the default.\n", msg)
}
}
40 changes: 0 additions & 40 deletions cli/command/service/helpers_test.go
View file
Open in desktop

This file was deleted.

2 changes: 1 addition & 1 deletion cli/command/service/opts.go
View file
Open in desktop
Expand Up @@ -691,7 +691,7 @@ func buildServiceDefaultFlagMapping() flagDefaults {
}

func addDetachFlag(flags *pflag.FlagSet, detach *bool) {
flags.BoolVarP(detach, flagDetach, "d", true, "Exit immediately instead of waiting for the service to converge")
flags.BoolVarP(detach, flagDetach, "d", false, "Exit immediately instead of waiting for the service to converge")
flags.SetAnnotation(flagDetach, "version", []string{"1.29"})
}

Expand Down
9 changes: 4 additions & 5 deletions cli/command/service/rollback.go
View file
Open in desktop
Expand Up @@ -7,8 +7,8 @@ import (
"github.com/docker/cli/cli"
"github.com/docker/cli/cli/command"
"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 {
Expand All @@ -19,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"},
}
Expand All @@ -31,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()

Expand All @@ -56,8 +56,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") {
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

return nil
}

Expand Down
10 changes: 4 additions & 6 deletions cli/command/service/scale.go
View file
Open in desktop
Expand Up @@ -10,9 +10,9 @@ import (
"github.com/docker/cli/cli"
"github.com/docker/cli/cli/command"
"github.com/docker/docker/api/types"
"github.com/docker/docker/api/types/versions"
"github.com/pkg/errors"
"github.com/spf13/cobra"
"github.com/spf13/pflag"
)

type scaleOptions struct {
Expand All @@ -27,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)
},
}

Expand All @@ -54,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()
Expand All @@ -79,9 +79,7 @@ func runScale(dockerCli command.Cli, flags *pflag.FlagSet, options *scaleOptions
}

if len(serviceIDs) > 0 {
if options.detach {
warnDetachDefault(dockerCli.Err(), dockerCli.Client().ClientVersion(), flags, "scaled")
} else {
if !options.detach && versions.GreaterThanOrEqualTo(dockerCli.Client().ClientVersion(), "1.29") {
for _, serviceID := range serviceIDs {
if err := waitOnService(ctx, dockerCli, serviceID, false); err != nil {
errs = append(errs, fmt.Sprintf("%s: %v", serviceID, err))
Expand Down
3 changes: 1 addition & 2 deletions cli/command/service/update.go
View file
Open in desktop
Expand Up @@ -216,8 +216,7 @@ func runUpdate(dockerCli command.Cli, flags *pflag.FlagSet, options *serviceOpti

fmt.Fprintf(dockerCli.Out(), "%s\n", serviceID)

if options.detach {
warnDetachDefault(dockerCli.Err(), dockerCli.Client().ClientVersion(), flags, "updated")
if options.detach || versions.LessThan(apiClient.ClientVersion(), "1.29") {
return nil
}

Expand Down
2 changes: 1 addition & 1 deletion docs/deprecated.md
View file
Open in desktop
Expand Up @@ -24,7 +24,7 @@ see [Feature Deprecation Policy](https://docs.docker.com/engine/#feature-depreca

**Deprecated In Release: v17.05.0**

**Disabled by default in release: v17.09**
**Disabled by default in release: [v17.09](https://github.com/docker/docker-ce/releases/tag/v17.09.0-ce)**

Docker 17.05.0 added an optional `--detach=false` option to make the
`docker service create` and `docker service update` work synchronously. This
Expand Down