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.

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 support for update order in compose deployments #360

Merged
merged 1 commit into from
Aug 1, 2017

Conversation

akalipetis
Copy link
Contributor

@akalipetis akalipetis commented Jul 19, 2017

Fixes #346

- What I did

Added support for the "order" key in the "update_config":

    deploy:
      replicas: 1
      update_config:
        parallelism: 1
        order: start-first | stop-first

- How I did it

Added the new key in the Compose V3.4 schema and hooked it with the implementation that already existed for service create/update.

- How to verify it

docker deploy -c docker-compose.yml demo

using this stack:

version: '3.4'

services:
  nginx:
    image: nginx:1.13.1-alpine
    deploy:
      replicas: 1
      update_config:
        parallelism: 1
        order: start-first
    ports:
      - 80:80

This will create a rolling update, even though we're just using 1 replica.

- Description for the changelog

Support for update_config.order: start-first | stop-first in Docker Compose version 3.4.

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

large_smart_squirrel

@akalipetis
Copy link
Contributor Author

/cc @dnephin I have cherry-picked da96115 and also included b882ab3 which (hopefully) rebuilds correctly the binary data.

This PR is based on #358

@dnephin
Copy link
Contributor

dnephin commented Jul 21, 2017

Looks like CI is broken on this PR. We've been having problems with that on a few PRs

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.

Thanks!

There' also a "full test" in loader_test.go that I think should be updated to include this.

@akalipetis
Copy link
Contributor Author

Thanks @dnephin - I'll definitely do that too. In regards to the CI, I thought it was because I had not included the -s flag in one commit and Gordon did not like me 😂

@codecov-io
Copy link

codecov-io commented Jul 22, 2017

Codecov Report

Merging #360 into master will increase coverage by 0.05%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #360      +/-   ##
==========================================
+ Coverage    46.2%   46.25%   +0.05%     
==========================================
  Files         193      193              
  Lines       16091    16092       +1     
==========================================
+ Hits         7435     7444       +9     
+ Misses       8269     8259      -10     
- Partials      387      389       +2

@akalipetis
Copy link
Contributor Author

@dnephin added a full-example test, but I still have issues in the following tests:

  • validate - I have changed to schema to include the order key, should I rollback this? This will crash the full example, since it needs v3.4 with order included to work
  • lint - no idea really, when I open the logs they're stuck in the docker version step loading without ever showing anything...

@@ -303,4 +327,3 @@ func _filePath(dir, name string) string {
cannonicalName := strings.Replace(name, "\\", "/", -1)
return filepath.Join(append([]string{dir}, strings.Split(cannonicalName, "/")...)...)
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this might be why validate is failing. Do you have precommit hooks that remove this line?

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, I see. I'll fix this and update the PR. Thanks 👍

@akalipetis
Copy link
Contributor Author

@dnephin seems like this was the issue after all 👍 - let me know if there's anything else you'd like me to do 😄

@dnephin
Copy link
Contributor

dnephin commented Jul 28, 2017

I think you can remove the WIP from the title now.

Needs a rebase since we merged another PR which added the 3.4 schema. You can ignore the conflicts in bindata.go, just regenerate the file and add that.

Then squash your commits and this should be ready. You can drop the commit from me, it should already be in master.

Signed-off-by: Antonis Kalipetis <akalipetis@gmail.com>
@akalipetis akalipetis changed the title [WIP] Add support for update order in compose deployments Add support for update order in compose deployments Jul 28, 2017
@akalipetis
Copy link
Contributor Author

Done 👌

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

Copy link
Member

@thaJeztah thaJeztah left a comment

Choose a reason for hiding this comment

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

LGTM

@thaJeztah thaJeztah merged commit 1cd402b into docker:master Aug 1, 2017
@GordonTheTurtle GordonTheTurtle added this to the 17.08.0 milestone Aug 1, 2017
@thaJeztah
Copy link
Member

thanks!

@hairyhenderson
Copy link
Contributor

@thaJeztah FYI this is missing in the docs for the update_config field: https://docs.docker.com/compose/compose-file/#update_config - I've logged an issue in the docs repo: docker/docs#5088

@thaJeztah
Copy link
Member

@hairyhenderson oh, thanks for spotting that! Looks like I forgot checking for that 😅

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

Successfully merging this pull request may close these issues.

Include an "order" key in the "update_config"
6 participants