Add `--tty` to `docker service create/update` #28076

yongtang · 2016-11-04T19:09:19Z

- What I did

This fix tries to add --tty to docker service create/update. As was specified in #25644, TTY flag has been added to SwarmKit and is already vendored.

- How I did it

This fix add --tty to docker service create/update.

Related document has been updated.

- How to verify it

Additional integration tests has been added.

- Description for the changelog

Add --tty flag to docker service create/update.

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

This fix fixes #25644.

Signed-off-by: Yong Tang yong.tang.github@outlook.com

yongtang · 2016-11-04T19:10:26Z

cc @crosbymichael @stevvooe @thaJeztah.

crosbymichael · 2016-11-04T19:56:55Z

LGTM

thaJeztah · 2016-11-04T21:20:46Z

8000

Just a quick check; if we want to add this to docker service update, what's the UX going to look like?

If the default is "false", and I run docker service update without specifying --tty, is it going to be without TTY?
Or, is it going to be "what was previously set"?
If "2", then to disable it, I'll need to use docker service update --tty=false ?

We should have a look at that before going forward with this, so that we don't lock ourselves in

aaronlehmann · 2016-11-05T01:29:28Z

Or, is it going to be "what was previously set"?
If "2", then to disable it, I'll need to use docker service update --tty=false ?

This is how it should work. As a rule, docker service update only changes things indicated by the command line options. docker service update with no options changes nothing.

aaronlehmann · 2016-11-05T01:30:37Z

The changes so far look good, but this PR should also add --tty=(true|false) to docker service update.

thaJeztah · 2016-11-05T01:34:20Z

@aaronlehmann ah yes you're right, thanks!

And i agree, we should have it in update as well

yongtang · 2016-11-05T04:03:27Z

Thanks @thaJeztah @aaronlehmann. The PR has been updated with --tty added to service update as well. Please take a look and let me know if there are any issues.

yongtang · 2016-11-05T15:00:44Z

The behavior of service update --tty is:

service update (without --tty) will leave the previous value alone, (what was previously set)
service update --tty={true|false} will override the previous value with the value specified (--tty={true|false}).

aaronlehmann · 2016-11-05T16:31:25Z

cli/command/service/opts.go

@@ -490,6 +494,7 @@ const (
 	flagRestartMaxAttempts    = "restart-max-attempts"
 	flagRestartWindow         = "restart-window"
 	flagStopGracePeriod       = "stop-grace-period"
+	flagTty                   = "tty"


aaronlehmann · 2016-11-05T16:32:14Z

cli/command/service/update.go

@@ -272,6 +272,14 @@ func updateService(flags *pflag.FlagSet, spec *swarm.ServiceSpec) error {
 		return err
 	}

+	if anyChanged(flags, flagTty) {


if flags.Changed(flagTTY) {

aaronlehmann · 2016-11-05T16:32:45Z

integration-cli/docker_cli_swarm_test.go

@@ -636,3 +636,72 @@ func (s *DockerSwarmSuite) TestSwarmServiceEnvFile(c *check.C) {
 	c.Assert(err, checker.IsNil)
 	c.Assert(out, checker.Contains, "[VAR1=C VAR2]")
 }
+
+func (s *DockerSwarmSuite) TestSwarmServiceTty(c *check.C) {


TestSwarmServiceTTY

aaronlehmann · 2016-11-05T16:33:54Z

integration-cli/docker_cli_swarm_test.go

+	c.Assert(out, checker.Contains, expectedOutput, check.Commentf("Expected '%s', but got %q", expectedOutput, out))
+}
+
+func (s *DockerSwarmSuite) TestSwarmServiceTtyUpdate(c *check.C) {


TestSwarmServiceTTYUpdate

aaronlehmann · 2016-11-05T16:34:27Z

integration-cli/docker_cli_swarm_test.go

+
+	// Without --tty
+	expectedOutput := "none"
+	out, err := d.Cmd("service", "create", "--name", name, "busybox", "sh", "-c", "if [ -t 0 ]; then echo TTY > /status && top; else echo none > /status && top; fi")


Pull out this shell code into a variable so it can be reused.

yongtang · 2016-11-05T17:26:57Z

Thanks @aaronlehmann for the review. Comments have been addressed. Please take a look and let me know if there are other issues.

aaronlehmann

LGTM

thaJeztah

LGTM, thanks!

/cc @albers @sdurrheimer for completion scripts :)

thaJeztah · 2016-11-07T23:02:07Z

@yongtang I guess this needs an update to the API docs (and API changes for 1.25) as well, can you include those changes in this PR?

yongtang · 2016-11-07T23:06:52Z

@thaJeztah Sure let me update the PR shortly.

yongtang · 2016-11-07T23:17:15Z

@thaJeztah The PR has been updated with doc changes. Please take a look.

thaJeztah

docs LGTM, thanks for the fast update @yongtang!

This fix tries to add `--tty` to `docker service create/update`. As was specified in 25644, `TTY` flag has been added to SwarmKit and is already vendored. This fix add `--tty` to `docker service create/update`. Related document has been updated. Additional integration tests has been added. This fix fixes 25644. Signed-off-by: Yong Tang <yong.tang.github@outlook.com>

yongtang · 2016-11-08T00:43:40Z

@thaJeztah Updated docs again for examples 😅

thaJeztah · 2016-11-08T00:44:39Z

Perfect, thank you!

vieux · 2016-11-08T02:00:05Z

LGTM

Pull request moby#27745 added support for the client to talk to older versions of the daemon. Various flags were added to docker 1.13 that are not compatible with older daemons. This PR adds annotations to those flags, so that they are automatically hidden if the daemon is older than docker 1.13 (API 1.25). Not all new flags affect the API (some are client-side only). The following PR's added new flags to docker 1.13 that affect the API; - moby#23430 added `--cpu-rt-period`and `--cpu-rt-runtime` - moby#27800 / moby#25317 added `--group` / `--group-add` / `--group-rm` - moby#27702 added `--network` to `docker build` - moby#25962 added `--attachable` to `docker network create` - moby#27998 added `--compose-file` to `docker stack deploy` - moby#22566 added `--stop-timeout` to `docker run` and `docker create` - moby#26061 added `--init` to `docker run` and `docker create` - moby#26941 added `--init-path` to `docker run` and `docker create` - moby#27958 added `--cpus` on `docker run` / `docker create` - moby#27567 added `--dns`, `--dns-opt`, and `--dns-search` to `docker service create` - moby#27596 added `--force` to `docker service update` - moby#27857 added `--hostname` to `docker service create` - moby#28031 added `--hosts`, `--host-add` / `--host-rm` to `docker service create` and `docker service update` - moby#28076 added `--tty` on `docker service create` / `docker service update` - moby#26421 added `--update-max-failure-ratio`, `--update-monitor` and `--rollback` on `docker service update` - moby#27369 added `--health-cmd`, `--health-interval`, `--health-retries`, `--health-timeout` and `--no-healthcheck` options to `docker service create` and `docker service update` Signed-off-by: Sebastiaan van Stijn <github@gone.nl>

Pull request moby/moby#27745 added support for the client to talk to older versions of the daemon. Various flags were added to docker 1.13 that are not compatible with older daemons. This PR adds annotations to those flags, so that they are automatically hidden if the daemon is older than docker 1.13 (API 1.25). Not all new flags affect the API (some are client-side only). The following PR's added new flags to docker 1.13 that affect the API; - moby/moby#23430 added `--cpu-rt-period`and `--cpu-rt-runtime` - moby/moby#27800 / moby/moby#25317 added `--group` / `--group-add` / `--group-rm` - moby/moby#27702 added `--network` to `docker build` - moby/moby#25962 added `--attachable` to `docker network create` - moby/moby#27998 added `--compose-file` to `docker stack deploy` - moby/moby#22566 added `--stop-timeout` to `docker run` and `docker create` - moby/moby#26061 added `--init` to `docker run` and `docker create` - moby/moby#26941 added `--init-path` to `docker run` and `docker create` - moby/moby#27958 added `--cpus` on `docker run` / `docker create` - moby/moby#27567 added `--dns`, `--dns-opt`, and `--dns-search` to `docker service create` - moby/moby#27596 added `--force` to `docker service update` - moby/moby#27857 added `--hostname` to `docker service create` - moby/moby#28031 added `--hosts`, `--host-add` / `--host-rm` to `docker service create` and `docker service update` - moby/moby#28076 added `--tty` on `docker service create` / `docker service update` - moby/moby#26421 added `--update-max-failure-ratio`, `--update-monitor` and `--rollback` on `docker service update` - moby/moby#27369 added `--health-cmd`, `--health-interval`, `--health-retries`, `--health-timeout` and `--no-healthcheck` options to `docker service create` and `docker service update` Signed-off-by: Sebastiaan van Stijn <github@gone.nl> Upstream-commit: 5d2722f83db9e301c6dcbe1c562c2051a52905db Component: cli

GordonTheTurtle added the status/0-triage label Nov 4, 2016

yongtang mentioned this pull request Nov 4, 2016

docker service requires a TTY option #25644

Closed

icecrime added the area/swarm label Nov 4, 2016

thaJeztah added status/1-design-review and removed status/0-triage labels Nov 4, 2016

yongtang force-pushed the 25644-docker-service-tty branch from 72acbea to 9f03a40 Compare November 5, 2016 04:01

yongtang changed the title ~~Add --tty to docker service create~~ Add --tty to docker service create/update Nov 5, 2016

aaronlehmann reviewed Nov 5, 2016

View reviewed changes

yongtang force-pushed the 25644-docker-service-tty branch from 9f03a40 to 0c086c8 Compare November 5, 2016 17:04

aaronlehmann approved these changes Nov 5, 2016

View reviewed changes

yongtang added status/2-code-review and removed status/1-design-review labels Nov 7, 2016

thaJeztah approved these changes Nov 7, 2016

View reviewed changes

thaJeztah added status/3-docs-review impact/changelog impact/cli and removed status/2-code-review labels Nov 7, 2016

thaJeztah added this to the 1.13.0 milestone Nov 7, 2016

thaJeztah added the impact/api label Nov 7, 2016

thaJeztah mentioned this pull request Nov 7, 2016

[epic] add more options to service create / service update #25303

Open

yongtang force-pushed the 25644-docker-service-tty branch from 0c086c8 to 9c21521 Compare November 7, 2016 23:16

thaJeztah approved these changes Nov 7, 2016

View reviewed changes

thaJeztah added status/4-merge and removed status/3-docs-review labels Nov 7, 2016

yongtang force-pushed the 25644-docker-service-tty branch from 9c21521 to 599be5a Compare November 8, 2016 00:42

vieux assigned dnephin Nov 8, 2016

dnephin merged commit 69efb46 into moby:master Nov 8, 2016

yongtang deleted the 25644-docker-service-tty branch November 8, 2016 03:24

albers mentioned this pull request Nov 8, 2016

Add bash completion for docker service {create,update} --tty #28156

Merged

sdurrheimer mentioned this pull request Nov 10, 2016

Add zsh completion for 'docker service {create,update} --tty' #28256

Merged

thaJeztah mentioned this pull request Jan 16, 2017

Add version annotation to various flags added in 1.13 #30186

Merged

ShivamSarodia mentioned this pull request May 19, 2017

TTY support on Docker services docker/docker-py#1622

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Add `--tty` to `docker service create/update` #28076

Add `--tty` to `docker service create/update` #28076

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Add --tty to docker service create/update #28076

Add --tty to docker service create/update #28076

Uh oh!

Conversation

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Add `--tty` to `docker service create/update` #28076

Add `--tty` to `docker service create/update` #28076