Formed in 2009, the Archive Team (not to be confused with the archive.org Archive-It Team) is a rogue archivist collective dedicated to saving copies of rapidly dying or deleted websites for the sake of history and digital heritage. The group is 100% composed of volunteers and interested parties, and has expanded into a large amount of related projects for saving online and digital history.
add --detach to docker scale
#243
Conversation
|
@thaJeztah not sure how to add the completion scripts here, I don't see --detach for the other service commands |
thaJeztah
added
impact/changelog
status/2-code-review
and removed
status/0-triage
labels
| } | ||
|
|
||
| if options.detach { | ||
| warnDetachDefault(dockerCli.Err(), dockerCli.Client().ClientVersion(), flags, "scaled") |
There was a problem hiding this comment.
Should this be inside the for loop?
|
CI is failing; |
Signed-off-by: Victor Vieux <victorvieux@gmail.com>
Codecov Report
@@ 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 |
|
@thaJeztah PTAL |
|
Thanks. It would also be great to add this flag to |
cli/command/service/scale.go
Outdated
| continue | ||
| } | ||
|
|
||
| if err := waitOnService(ctx, dockerCli, serviceID, false); err != nil { |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
Thanks, I missed this. I think we should scale them all at once.
There was a problem hiding this comment.
yeah I agree it would be best to not change the existing behaviour
There was a problem hiding this comment.
@vieux you'll be working on this?
|
@dnephin @vdemeester how about this ? (I kept it simple) |
|
I understand it's not a perfect solution, but I think it's OK for now. |
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) |
There was a problem hiding this comment.
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() ?
There was a problem hiding this comment.
I also removed the warning if there is only errors.
Signed-off-by: Victor Vieux <victorvieux@gmail.com>
|
@dnephin PTAL |
7 participants
depends on #219
fixes moby/moby#32368