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

Add 'docker service rollback' subcommand #205

Merged
merged 1 commit into from Aug 19, 2017

Conversation

Projects
None yet
7 participants
@redpanda
Contributor

redpanda commented Jun 19, 2017

Signed-off-by: Jimmy Leger jimmy.leger@gmail.com

fixes #142
PS: This bug was fixed during a Docker Hackergarten.

- What I did

I add the docker service rollback subcommand

- How I did it

I extracted the rollback process from runUpdate

- How to verify it

Create a service with a single replica:

$ docker service create --name my-service -p 8080:80 nginx:alpine

Confirm that the service is running with a single replica:

$ docker service ls

ID                  NAME                MODE                REPLICAS            IMAGE               PORTS
xbw728mf6q0d        my-service          replicated          1/1                 nginx:alpine        *:8080->80/tcp

Update the service to use three replicas:

$ docker service update --replicas=3 my-service

$ docker service ls

ID                  NAME                MODE                REPLICAS            IMAGE               PORTS
xbw728mf6q0d        my-service          replicated          3/3                 nginx:alpine        *:8080->80/tcp

Now roll back the service to its previous version, and confirm it is
running a single replica again:

$ docker service rollback my-service

$ docker service ls

ID                  NAME                MODE                REPLICAS            IMAGE               PORTS
xbw728mf6q0d        my-service          replicated          1/1                 nginx:alpine        *:8080->80/tcp

- Description for the changelog

Add docker service rollback subcommand

- A picture of a cute animal (not mandatory but encouraged)

redpanda picture

Show outdated Hide outdated cli/command/service/rollback.go
Show outdated Hide outdated cli/command/service/rollback.go
Show outdated Hide outdated cli/command/service/rollback.go
@codecov-io

This comment has been minimized.

Show comment
Hide comment
@codecov-io

codecov-io Jun 19, 2017

Codecov Report

❗️ No coverage uploaded for pull request base (master@17adcbd). Click here to learn what that means.
The diff coverage is 76.36%.

@@            Coverage Diff            @@
##             master     #205   +/-   ##
=========================================
  Coverage          ?   46.33%           
=========================================
  Files             ?      194           
  Lines             ?    16134           
  Branches          ?        0           
=========================================
  Hits              ?     7475           
  Misses            ?     8272           
  Partials          ?      387

codecov-io commented Jun 19, 2017

Codecov Report

❗️ No coverage uploaded for pull request base (master@17adcbd). Click here to learn what that means.
The diff coverage is 76.36%.

@@            Coverage Diff            @@
##             master     #205   +/-   ##
=========================================
  Coverage          ?   46.33%           
=========================================
  Files             ?      194           
  Lines             ?    16134           
  Branches          ?        0           
=========================================
  Hits              ?     7475           
  Misses            ?     8272           
  Partials          ?      387

@GordonTheTurtle GordonTheTurtle added dco/no and removed dco/no labels Jun 25, 2017

@redpanda

This comment has been minimized.

Show comment
Hide comment
@redpanda

redpanda Jun 25, 2017

Contributor

Hello @dnephin!

First thank for your feedbacks.

I don't make the choice to refactor the runUpdate function but just implement runRollback which only call ServiceUpdate endpoint with the needed parameters.

BTW I have some code duplication between runUpdate and runRollback see: https://github.com/docker/cli/pull/205/files#diff-09b79b8a3ba1feabc2d859a294bb1ac5R43
Should I create a function to initialize types.ServiceUpdateOptions ?

Thanks by advance for your feedbacks.

Contributor

redpanda commented Jun 25, 2017

Hello @dnephin!

First thank for your feedbacks.

I don't make the choice to refactor the runUpdate function but just implement runRollback which only call ServiceUpdate endpoint with the needed parameters.

BTW I have some code duplication between runUpdate and runRollback see: https://github.com/docker/cli/pull/205/files#diff-09b79b8a3ba1feabc2d859a294bb1ac5R43
Should I create a function to initialize types.ServiceUpdateOptions ?

Thanks by advance for your feedbacks.

@dnephin

Thanks! This is looking better

I would suggest rebasing on docker/master to pick the new CI changes. It will help your build run faster.

Show outdated Hide outdated cli/command/service/rollback.go
Show outdated Hide outdated cli/command/service/rollback.go
@dnephin

Thanks!

A unit test would be great as well.

There are examples of unit tests in other cli/command packages.

@redpanda

This comment has been minimized.

Show comment
Hide comment
@redpanda

redpanda Jul 4, 2017

Contributor

Yeah I'm working on it! 👍

Contributor

redpanda commented Jul 4, 2017

Yeah I'm working on it! 👍

@GordonTheTurtle GordonTheTurtle added dco/no and removed dco/no labels Jul 4, 2017

@redpanda

This comment has been minimized.

Show comment
Hide comment
@redpanda

redpanda Jul 18, 2017

Contributor
Contributor

redpanda commented Jul 18, 2017

@docker docker deleted a comment from GordonTheTurtle Jul 18, 2017

Show outdated Hide outdated cli/command/service/client_test.go
Show outdated Hide outdated cli/command/service/helpers.go

@GordonTheTurtle GordonTheTurtle added dco/no and removed dco/no labels Jul 18, 2017

@dnephin

Thanks! Code looks good, just a few minor comments.

Please squash your commits down to one or two logical commits.

Show outdated Hide outdated cli/command/service/rollback.go
Show outdated Hide outdated cli/command/service/rollback.go
Show outdated Hide outdated cli/command/service/rollback.go
Show outdated Hide outdated cli/command/service/rollback_test.go

@docker docker deleted a comment from GordonTheTurtle Jul 18, 2017

@redpanda

This comment has been minimized.

Show comment
Hide comment
@redpanda

redpanda Jul 27, 2017

Contributor

@vdemeester everything is OK

Contributor

redpanda commented Jul 27, 2017

@vdemeester everything is OK

@dnephin

LGTM

@thaJeztah

Thank you! Code looks good to me, moving to "docs review"

Show outdated Hide outdated cli/command/service/rollback.go
@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Jul 27, 2017

Member

Can you;

Member

thaJeztah commented Jul 27, 2017

Can you;

@redpanda

This comment has been minimized.

Show comment
Hide comment
@redpanda

redpanda Jul 28, 2017

Contributor

Sure 👍

Contributor

redpanda commented Jul 28, 2017

Sure 👍

@redpanda redpanda requested a review from mistyhacks as a code owner Jul 31, 2017

@vdemeester

Code LGTM 🐸

@thaJeztah

left some comments / suggestions

@redpanda

This comment has been minimized.

Show comment
Hide comment
@redpanda

redpanda Aug 14, 2017

Contributor

Should I implement the flag --update-delay.
This flag can be used with the --rolback flag docker service update command.
See: https://docs.docker.com/engine/reference/commandline/service_update/#rolling-back-to-the-previous-version-of-a-service

Contributor

redpanda commented Aug 14, 2017

Should I implement the flag --update-delay.
This flag can be used with the --rolback flag docker service update command.
See: https://docs.docker.com/engine/reference/commandline/service_update/#rolling-back-to-the-previous-version-of-a-service

@thaJeztah

This comment has been minimized.

Show comment
Hide comment
@thaJeztah

thaJeztah Aug 14, 2017

Member

@redpanda I think it's best to do so in a follow up. This pull request is already in docs review, and provides the core functionality needed; we can enhance the functionality in a follow up 👍

Member

thaJeztah commented Aug 14, 2017

@redpanda I think it's best to do so in a follow up. This pull request is already in docs review, and provides the core functionality needed; we can enhance the functionality in a follow up 👍

@thaJeztah

Two minor comments, but LGTM after those are addressed 👍

@mistyhacks

Looking good. Just a couple more nitpicky changes please. :) 👼

Show outdated Hide outdated docs/reference/commandline/service_rollback.md
@@ -0,0 +1,94 @@
---
title: "service rollback"

This comment has been minimized.

@mistyhacks

mistyhacks Aug 15, 2017

Contributor

quotes not required in any of the front-matter

@mistyhacks

mistyhacks Aug 15, 2017

Contributor

quotes not required in any of the front-matter

This comment has been minimized.

@redpanda

redpanda Aug 16, 2017

Contributor

Should I replaced that line by: title: service rollback ?

@redpanda

redpanda Aug 16, 2017

Contributor

Should I replaced that line by: title: service rollback ?

This comment has been minimized.

@redpanda

redpanda Aug 16, 2017

Contributor

I just read some information about the front-matter.
Should I changed it like that ?

---
title: service rollback
description: The service rollback command description and usage
keywords: ["service", "rollback"]
---

PS: I just keep the same front-matter used in other command line documentation but I can change it. ^^

@redpanda

redpanda Aug 16, 2017

Contributor

I just read some information about the front-matter.
Should I changed it like that ?

---
title: service rollback
description: The service rollback command description and usage
keywords: ["service", "rollback"]
---

PS: I just keep the same front-matter used in other command line documentation but I can change it. ^^

This comment has been minimized.

@mistyhacks

mistyhacks Aug 16, 2017

Contributor

It doesn't seem to hurt anything. We can probably do a bulk fix later on.

@mistyhacks

mistyhacks Aug 16, 2017

Contributor

It doesn't seem to hurt anything. We can probably do a bulk fix later on.

This comment has been minimized.

@redpanda

redpanda Aug 16, 2017

Contributor

Ok

@redpanda

redpanda Aug 16, 2017

Contributor

Ok

Show outdated Hide outdated docs/reference/commandline/service_rollback.md
Show outdated Hide outdated docs/reference/commandline/service_rollback.md
@mistyhacks

This comment has been minimized.

Show comment
Hide comment
@mistyhacks

mistyhacks Aug 16, 2017

Contributor

I feel like I confused you. Don't change the Markdown hint to markdown on files unrelated to the original intent of this commit. We can fix the other ones later.

Contributor

mistyhacks commented Aug 16, 2017

I feel like I confused you. Don't change the Markdown hint to markdown on files unrelated to the original intent of this commit. We can fix the other ones later.

@redpanda

This comment has been minimized.

Show comment
Hide comment
@redpanda

redpanda Aug 16, 2017

Contributor

You right! I will revert these changes. ^^

Contributor

redpanda commented Aug 16, 2017

You right! I will revert these changes. ^^

Add 'docker service rollback' subcommand
Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com>

Implement runRollback to not use runUpdate

Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com>

Add version tag and add flag quiet to suppress progress output

Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com>

Removed flags from warnDetachDefault

Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com>

Used command.Cli interface

Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com>

Add detach flag on rollback command

Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com>

Create a fakeClient for service commands

Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com>

Added unit test for rollback command

Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com>

Used command.Cli interface instead of *command.DockerCli in service commands

Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com>

Revert "Removed flags from warnDetachDefault"

This reverts commit 3e4f601.

Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com>

Fixed test.NewFakeCli instanciation

Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com>

Removed unused receiver

Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com>

Replaced cli by dockerCli

Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com>

Revert "Removed unused receiver"

This reverts commit 604ef7c.

Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com>

Fixed last typo

Signed-off-by: Jimmy Leger <jimmy.leger@gmail.com>
@mistyhacks

This comment has been minimized.

Show comment
Hide comment
@mistyhacks
Contributor

mistyhacks commented Aug 17, 2017

Ping @thaJeztah

@thaJeztah

LGTM, thanks!

@thaJeztah thaJeztah merged commit 3c7ede6 into docker:master Aug 19, 2017

6 checks passed

ci/circleci: cross Your tests passed on CircleCI!
Details
ci/circleci: lint Your tests passed on CircleCI!
Details
ci/circleci: shellcheck Your tests passed on CircleCI!
Details
ci/circleci: test Your tests passed on CircleCI!
Details
ci/circleci: validate Your tests passed on CircleCI!
Details
dco-signed All commits are signed

@GordonTheTurtle GordonTheTurtle added this to the 17.08.0 milestone Aug 19, 2017

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