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

add --detach to docker scale #243

Merged
merged 2 commits into from
Jul 4, 2017
Merged

add --detach to docker scale #243

merged 2 commits into from
Jul 4, 2017

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 Outdated
}

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

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

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

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
Contributor

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
Contributor

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

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

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

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

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

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

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

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

@aaronlehmann aaronlehmann aaronlehmann left review comments

@vdemeester vdemeester vdemeester approved these changes

@dnephin dnephin dnephin approved these changes

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