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.

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

Allow --detach and --quiet flags when using --rollback #144

Merged
merged 1 commit into from Jun 1, 2017

Conversation

Projects
None yet
7 participants
@thaJeztah
Member

thaJeztah commented Jun 1, 2017

Commit 78c204e (moby/moby@f9bd8ec / moby/moby#31108 in the moby repo) added a validation to prevent --rollback from being used in combination with other flags that update the service spec.

This validation was not taking into account that some flags only affect the CLI behavior, and are okay to be used when rolling back.

This patch updates the validation, and adds --quiet and --detach to the list of allowed flags.

relates to moby/moby#33444 (comment)

Allow --detach and --quiet flags when using --rollback
Commit 78c204e added
(f9bd8ec in the moby repo)
a validation to prevent `--rollback` from being used
in combination with other flags that update the
service spec.

This validation was not taking into account that
some flags only affect the CLI behavior, and
are okay to be used when rolling back.

This patch updates the validation, and adds
`--quiet` and `--detach` to the list of allowed
flags.

Signed-off-by: Sebastiaan van Stijn <github@gone.nl>
@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Jun 1, 2017

Collaborator

LGTM

Collaborator

aaronlehmann commented Jun 1, 2017

LGTM

@dnephin

dnephin approved these changes Jun 1, 2017

LGTM

@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jun 1, 2017

Codecov Report

Merging #144 into master will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff           @@
##           master     #144   +/-   ##
=======================================
  Coverage   44.96%   44.96%           
=======================================
  Files         169      169           
  Lines       11378    11378           
=======================================
  Hits         5116     5116           
  Misses       5970     5970           
  Partials      292      292

codecov-io commented Jun 1, 2017

Codecov Report

Merging #144 into master will not change coverage.
The diff coverage is 0%.

@@           Coverage Diff           @@
##           master     #144   +/-   ##
=======================================
  Coverage   44.96%   44.96%           
=======================================
  Files         169      169           
  Lines       11378    11378           
=======================================
  Hits         5116     5116           
  Misses       5970     5970           
  Partials      292      292
@cpuguy83

This comment has been minimized.

Show comment
Hide comment
@cpuguy83

cpuguy83 Jun 1, 2017

Collaborator

Kind of stinks we can't specify the rollback order when rolling back.

Collaborator

cpuguy83 commented Jun 1, 2017

Kind of stinks we can't specify the rollback order when rolling back.

@aaronlehmann

This comment has been minimized.

Show comment
Hide comment
@aaronlehmann

aaronlehmann Jun 1, 2017

Collaborator

@cpuguy83: For the swarm manager to know it's doing a rollback, the rollback has to happen server-side instead of the client just submitting PreviousSpec as the new spec. But if the spec is being rolled back on the server side, there is no mechanism to tell the swarm manager "use the previous spec, except change X and Y and Z".

Collaborator

aaronlehmann commented Jun 1, 2017

@cpuguy83: For the swarm manager to know it's doing a rollback, the rollback has to happen server-side instead of the client just submitting PreviousSpec as the new spec. But if the spec is being rolled back on the server side, there is no mechanism to tell the swarm manager "use the previous spec, except change X and Y and Z".

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jun 1, 2017

Member

Also related to this; #142

For the swarm manager to know it's doing a rollback, the rollback has to happen server-side instead of the client just submitting PreviousSpec as the new spec.

Perhaps a PATCH request could be used to allow updating the spec while rolling back (i.e., just allow setting some options)? Anyway, that's a different discussion 😄

Member

thaJeztah commented Jun 1, 2017

Also related to this; #142

For the swarm manager to know it's doing a rollback, the rollback has to happen server-side instead of the client just submitting PreviousSpec as the new spec.

Perhaps a PATCH request could be used to allow updating the spec while rolling back (i.e., just allow setting some options)? Anyway, that's a different discussion 😄

@vdemeester

LGTM 🐸

@vdemeester vdemeester referenced this pull request Jun 1, 2017

Closed

17.06.0 RC2 tracker #2

23 of 23 tasks complete

@dnephin dnephin merged commit 30af879 into docker:master Jun 1, 2017

3 of 4 checks passed

codecov/patch 0% of diff hit (target 50%)
Details
ci/circleci Your tests passed on CircleCI!
Details
codecov/project 44.96% remains the same compared to efaadcf
Details
dco-signed All commits are signed

@GordonTheTurtle GordonTheTurtle added this to the 17.06.0 milestone Jun 1, 2017

@thaJeztah thaJeztah deleted the thaJeztah:allow-some-flags-during-rollback branch Jun 1, 2017

@thaJeztah thaJeztah referenced this pull request Jun 7, 2017

Closed

17.06.0 RC3 tracker #8

40 of 40 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment