X Tutup
Skip to content

add --detach to docker scale #243

Merged
vieux merged 2 commits intodocker:masterfrom
vieux:scale2
Jul 4, 2017
Merged

add --detach to docker scale #243
vieux merged 2 commits intodocker:masterfrom
vieux:scale2

Conversation

@vieux
Copy link
Contributor

@vieux vieux commented Jun 27, 2017

depends on #219

fixes moby/moby#32368

@vieux vieux changed the title Scale2 add --detach to docker scale Jun 27, 2017
@vieux vieux changed the title add --detach to docker scale add --detach to docker scale Jun 27, 2017
@vieux vieux mentioned this pull request Jun 27, 2017
Merged
@vieux
Copy link
Contributor Author

vieux commented Jun 27, 2017

@thaJeztah not sure how to add the completion scripts here, I don't see --detach for the other service commands

thaJeztah
thaJeztah reviewed Jun 27, 2017
View reviewed changes
cli/command/service/scale.go
}

if options.detach {
warnDetachDefault(dockerCli.Err(), dockerCli.Client().ClientVersion(), flags, "scaled")
Copy link
Member

@thaJeztah thaJeztah Jun 27, 2017

Choose a reason for hiding this comment

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

Should this be inside the for loop?

Copy link
Contributor Author

@vieux vieux Jun 27, 2017

Choose a reason for hiding this comment

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

oh good point

@thaJeztah
Copy link
Member

thaJeztah commented Jun 27, 2017

CI is failing;

cli/command/service/helpers_test.go:7: testing redeclared as imported package name
	previous declaration at cli/command/service/helpers_test.go:5
FAIL	github.com/docker/cli/cli/command/service [build failed]

@vieux
add --detach to docker scale
6c6b109 
Signed-off-by: Victor Vieux <victorvieux@gmail.com>
@codecov-io
Copy link

codecov-io commented Jun 27, 2017 •
edited
Loading

Codecov Report

Merging #243 into master will decrease coverage by 1.65%.
The diff coverage is 0%.

@@            Coverage Diff             @@
##           master     #243      +/-   ##
==========================================
- Coverage   48.44%   46.78%   -1.66%     
==========================================
  Files         173      172       -1     
  Lines       11748    11706      -42     
==========================================
- Hits         5691     5477     -214     
- Misses       5696     5917     +221     
+ Partials      361      312      -49

@vieux
Copy link
Contributor Author

vieux commented Jun 27, 2017

@thaJeztah PTAL

vdemeester
vdemeester approved these changes Jun 27, 2017
View reviewed changes
Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🦁

@aaronlehmann
Copy link

aaronlehmann commented Jun 27, 2017

Thanks. It would also be great to add this flag to docker stack deploy. There has been a lot of interest in that: moby/moby#32367. I see @vdemeester assigned this to himself.

aaronlehmann
aaronlehmann reviewed Jun 27, 2017
View reviewed changes
cli/command/service/scale.go Outdated
continue
}

if err := waitOnService(ctx, dockerCli, serviceID, false); err != nil {
Copy link

@aaronlehmann aaronlehmann Jun 27, 2017

Choose a reason for hiding this comment

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

This will change the behavior to wait for the first service to scale before scaling the next one. Previously, they would all be scaled at the same time. I wonder if it's better to continue scaling them simultaneously, and show the progress for all services at the same time. On the other hand, this is a lot more work, and I'm not sure enough people try to scale multiple services at once for this to matter. Any thoughts?

Copy link
Contributor

@dnephin dnephin Jun 27, 2017

Choose a reason for hiding this comment

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

Thanks, I missed this. I think we should scale them all at once.

Copy link
Contributor Author

@vieux vieux Jun 27, 2017

Choose a reason for hiding this comment

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

yeah I agree it would be best to not change the existing behaviour

Copy link
Member

@thaJeztah thaJeztah Jun 27, 2017

Choose a reason for hiding this comment

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

@vieux you'll be working on this?

Copy link
Contributor Author

@vieux vieux Jun 30, 2017

Choose a reason for hiding this comment

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

yes

@vieux
Copy link
Contributor Author

vieux commented Jun 30, 2017

@dnephin @vdemeester how about this ? (I kept it simple)

vdemeester
vdemeester approved these changes Jun 30, 2017
View reviewed changes
Copy link
Collaborator

@vdemeester vdemeester left a comment

Choose a reason for hiding this comment

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

LGTM 🐮

@vieux
Copy link
Contributor Author

vieux commented Jun 30, 2017

I understand it's not a perfect solution, but I think it's OK for now.

dnephin
dnephin reviewed Jun 30, 2017
View reviewed changes
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

Just one question

cli/command/service/scale.go Outdated
for _, arg := range args {
parts := strings.SplitN(arg, "=", 2)
serviceID, scaleStr := parts[0], parts[1]
serviceIDs = append(serviceIDs, serviceID)
Copy link
Contributor

@dnephin dnephin Jun 30, 2017

Choose a reason for hiding this comment

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

Do we still want to wait on a service if it failed to update?

Maybe we should only add the serviceID to this list if there is no error from runServiceScale() ?

Copy link
Contributor Author

@vieux vieux Jul 3, 2017

Choose a reason for hiding this comment

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

I agree

Copy link
Contributor Author

@vieux vieux Jul 3, 2017

Choose a reason for hiding this comment

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

I also removed the warning if there is only errors.

@vieux
scale all services at once
58d3fc2 
Signed-off-by: Victor Vieux <victorvieux@gmail.com>
@vieux
Copy link
Contributor Author

vieux commented Jul 4, 2017

@dnephin PTAL

dnephin
dnephin approved these changes Jul 4, 2017
View reviewed changes
Copy link
Contributor

@dnephin dnephin left a comment

Choose a reason for hiding this comment

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

LGTM

@vieux vieux merged commit 43fb4a4 into docker:master Jul 4, 2017
@GordonTheTurtle GordonTheTurtle added this to the 17.07.0 milestone Jul 4, 2017
@vieux vieux deleted the scale2 branch July 4, 2017 14:37
@thaJeztah thaJeztah mentioned this pull request Jul 4, 2017
Closed
@albers albers mentioned this pull request Jul 26, 2017
Merged
@jphuynh jphuynh mentioned this pull request Jul 27, 2017
Merged
nobiit pushed a commit to nobidev/docker-cli that referenced this pull request Nov 19, 2025
@andrewhsu
Merge pull request docker#243 from andrewhsu/rc3
2357fb2 
[17.09] bump version to 17.09.0-ce-rc3
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@thaJeztah thaJeztah thaJeztah left review comments

@vdemeester vdemeester vdemeester approved these changes

+2 more reviewers

@aaronlehmann aaronlehmann aaronlehmann left review comments

@dnephin dnephin dnephin approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

impact/changelog status/2-code-review

Projects

None yet

Milestone

17.07.0

Development

Successfully merging this pull request may close these issues.

Interactive docker service scale

7 participants

@vieux @thaJeztah @codecov-io @aaronlehmann @vdemeester @dnephin @GordonTheTurtle
X Tutup