Skip to content

Comments

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

Merged
dnephin merged 1 commit intomoby:masterfrom
yongtang:25644-docker-service-tty
Nov 8, 2016
Merged

Add --tty to docker service create/update#28076
dnephin merged 1 commit intomoby:masterfrom
yongtang:25644-docker-service-tty

Conversation

@yongtang
Copy link
Member

@yongtang yongtang commented Nov 4, 2016 •
edited
Loading

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

cute_kitten-wide

This fix fixes #25644.

Signed-off-by: Yong Tang [email protected]

@yongtang yongtang mentioned this pull request Nov 4, 2016
Closed
@yongtang
Copy link
Member Author

yongtang commented Nov 4, 2016

cc @crosbymichael @stevvooe @thaJeztah.

@crosbymichael
Copy link
Contributor

crosbymichael commented Nov 4, 2016

LGTM

@thaJeztah
Copy link
Member

thaJeztah commented Nov 4, 2016

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
Copy link

aaronlehmann commented Nov 5, 2016

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
Copy link

aaronlehmann commented Nov 5, 2016

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

@thaJeztah
Copy link
Member

thaJeztah commented Nov 5, 2016

@aaronlehmann ah yes you're right, thanks!

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

@yongtang yongtang force-pushed the 25644-docker-service-tty branch from 72acbea to 9f03a40 Compare November 5, 2016 04:01
@yongtang yongtang changed the title Add --tty to docker service create Add --tty to docker service create/update Nov 5, 2016
@yongtang
Copy link
Member Author

yongtang commented Nov 5, 2016

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
Copy link
Member Author

yongtang commented Nov 5, 2016

The behavior of service update --tty is:

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

aaronlehmann
aaronlehmann reviewed Nov 5, 2016
View reviewed changes
cli/command/service/opts.go Outdated
Copy link

@aaronlehmann aaronlehmann Nov 5, 2016

Choose a reason for hiding this comment

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

flagTTY

aaronlehmann
aaronlehmann reviewed Nov 5, 2016
View reviewed changes
cli/command/service/update.go Outdated
Copy link

@aaronlehmann aaronlehmann Nov 5, 2016

Choose a reason for hiding this comment

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

if flags.Changed(flagTTY) {

aaronlehmann
aaronlehmann reviewed Nov 5, 2016
View reviewed changes
integration-cli/docker_cli_swarm_test.go Outdated
Copy link

@aaronlehmann aaronlehmann Nov 5, 2016

Choose a reason for hiding this comment

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

TestSwarmServiceTTY

aaronlehmann
aaronlehmann reviewed Nov 5, 2016
View reviewed changes
integration-cli/docker_cli_swarm_test.go Outdated
Copy link

@aaronlehmann aaronlehmann Nov 5, 2016

Choose a reason for hiding this comment

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

TestSwarmServiceTTYUpdate

aaronlehmann
aaronlehmann reviewed Nov 5, 2016
View reviewed changes
integration-cli/docker_cli_swarm_test.go Outdated
Copy link

@aaronlehmann aaronlehmann Nov 5, 2016

Choose a reason for hiding this comment

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

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

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

yongtang commented Nov 5, 2016

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

aaronlehmann
aaronlehmann approved these changes Nov 5, 2016
View reviewed changes
Copy link

@aaronlehmann aaronlehmann left a comment

Choose a reason for hiding this comment

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

LGTM

thaJeztah
thaJeztah approved these changes Nov 7, 2016
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

/cc @albers @sdurrheimer for completion scripts :)

@thaJeztah thaJeztah added this to the 1.13.0 milestone Nov 7, 2016
@thaJeztah
Copy link
Member

thaJeztah commented Nov 7, 2016

@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?

@thaJeztah thaJeztah mentioned this pull request Nov 7, 2016
Open
@yongtang
Copy link
Member Author

yongtang commented Nov 7, 2016

@thaJeztah Sure let me update the PR shortly.

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

yongtang commented Nov 7, 2016

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

thaJeztah
thaJeztah approved these changes Nov 7, 2016
View reviewed changes
Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

docs LGTM, thanks for the fast update @yongtang!

@yongtang
Add --tty to docker service create/update
599be5a 
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 <[email protected]>
@yongtang yongtang force-pushed the 25644-docker-service-tty branch from 9c21521 to 599be5a Compare November 8, 2016 00:42
@yongtang
Copy link
Member Author

yongtang commented Nov 8, 2016

@thaJeztah Updated docs again for examples 😅

@thaJeztah
Copy link
Member

thaJeztah commented Nov 8, 2016

Perfect, thank you!

@vieux
Copy link
Contributor

vieux commented Nov 8, 2016

LGTM

@dnephin dnephin merged commit 69efb46 into moby:master Nov 8, 2016
@yongtang yongtang deleted the 25644-docker-service-tty branch November 8, 2016 03:24
@albers albers mentioned this pull request Nov 8, 2016
Merged
@sdurrheimer sdurrheimer mentioned this pull request Nov 10, 2016
Merged
thaJeztah added a commit to thaJeztah/docker that referenced this pull request Jan 16, 2017
@thaJeztah
Add version annotation to various flags added in 1.13
bbbddee 
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 <[email protected]>
@thaJeztah thaJeztah mentioned this pull request Jan 16, 2017
Merged
andrewhsu pushed a commit to docker-archive/docker-ce that referenced this pull request May 19, 2017
@thaJeztah
Add version annotation to various flags added in 1.13
1daa5da 
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 <[email protected]>
Upstream-commit: 5d2722f83db9e301c6dcbe1c562c2051a52905db
Component: cli
@ShivamSarodia ShivamSarodia mentioned this pull request May 19, 2017
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah approved these changes

+1 more reviewer

@aaronlehmann aaronlehmann aaronlehmann approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@dnephin dnephin

Labels

area/swarm impact/api impact/changelog impact/cli status/4-merge

Projects

None yet

Milestone

1.13.0

Development

Successfully merging this pull request may close these issues.

docker service requires a TTY option

8 participants

@yongtang @crosbymichael @thaJeztah @aaronlehmann @vieux @dnephin @icecrime @GordonTheTurtle