-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
Conversation
5ae5c6f
to
b882ab3
Compare
Looks like CI is broken on this PR. We've been having problems with that on a few PRs |
There was a problem hiding this 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.
Thanks @dnephin - I'll definitely do that too. In regards to the CI, I thought it was because I had not included the |
Codecov Report
@@ 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 |
03490b8
to
ebfc89f
Compare
@dnephin added a full-example test, but I still have issues in the following tests:
|
cli/compose/schema/bindata.go
Outdated
@@ -303,4 +327,3 @@ func _filePath(dir, name string) string { | |||
cannonicalName := strings.Replace(name, "\\", "/", -1) | |||
return filepath.Join(append([]string{dir}, strings.Split(cannonicalName, "/")...)...) | |||
} | |||
|
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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 👍
ebfc89f
to
60869c8
Compare
@dnephin seems like this was the issue after all 👍 - let me know if there's anything else you'd like me to do 😄 |
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 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>
60869c8
to
2950667
Compare
Done 👌 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
thanks! |
@thaJeztah FYI this is missing in the docs for the |
@hairyhenderson oh, thanks for spotting that! Looks like I forgot checking for that 😅 |
Fixes #346
- What I did
Added support for the "order" key in the "update_config":
- 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
using this stack:
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)