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

Fixed depends_on recreation behaviour for issue #6589 #6592

Merged
merged 1 commit into from Mar 26, 2019

Conversation

Projects
None yet
4 participants
@joeweoj
Copy link
Contributor

joeweoj commented Mar 19, 2019

Previously any containers which did not have any links were always recreated.
In order to fix depends_on and preserve expected links recreation behaviour, we now only use the ConvergenceStrategy.always recreation strategy if the service defines links but the container does not have any.

Resolves #6589

@GordonTheTurtle

This comment has been minimized.

Copy link

GordonTheTurtle commented Mar 19, 2019

Please sign your commits following these rules:
https://github.com/moby/moby/blob/master/CONTRIBUTING.md#sign-your-work
The easiest way to do this is to amend the last commit:

$ git clone -b "6589-depends_on-recreation-fix" git@github.com:treatwell/compose.git somewhere
$ cd somewhere
$ git commit --amend -s --no-edit
$ git push -f

Amending updates the existing PR. You DO NOT need to open a new one.

@joeweoj joeweoj force-pushed the treatwell:6589-depends_on-recreation-fix branch from 7caee3c to adf4127 Mar 19, 2019

@GordonTheTurtle GordonTheTurtle removed the dco/no label Mar 19, 2019

@jcsirot

This comment has been minimized.

Copy link

jcsirot commented Mar 20, 2019

Thank you for this PR. Please could you add a test covering the bug it fixes.

@joeweoj joeweoj force-pushed the treatwell:6589-depends_on-recreation-fix branch from adf4127 to fd3fbcf Mar 25, 2019

@joeweoj

This comment has been minimized.

Copy link
Contributor Author

joeweoj commented Mar 25, 2019

@jcsirot I have added some tests to verify the fixed depends_on behaviour (i verified the links tests still passed when i created the PR).

Let me know if you have any questions. Thanks

@jcsirot

This comment has been minimized.

Copy link

jcsirot commented Mar 25, 2019

thank you @joeweoj.
@ulyssessouza PTAL

Show resolved Hide resolved tests/integration/state_test.py Outdated
Show resolved Hide resolved tests/integration/state_test.py Outdated
Show resolved Hide resolved tests/integration/state_test.py Outdated
Show resolved Hide resolved tests/integration/state_test.py Outdated
Show resolved Hide resolved compose/project.py

@joeweoj joeweoj force-pushed the treatwell:6589-depends_on-recreation-fix branch from fd3fbcf to 0949870 Mar 26, 2019

Show resolved Hide resolved tests/integration/state_test.py Outdated
@ijc

ijc approved these changes Mar 26, 2019

Copy link
Contributor

ijc left a comment

With the one last (I think) missing set() in test_change_root_always_recreate_deps addressed this LGTM, thanks!

Fixed depends_on recreation behaviour for issue #6589
Previously any containers which did *not* have any links were always recreated.
In order to fix depends_on and preserve expected links recreation behaviour, we now only use the ConvergenceStrategy.always recreation strategy for a service if any of the the following conditions are true:
* --always-recreate-deps flag provided
* service container is stopped
* service defines links but the container does not have any
* container has links but the service definition does not

Signed-off-by: joeweoj <joewardell@gmail.com>

@joeweoj joeweoj force-pushed the treatwell:6589-depends_on-recreation-fix branch from 0949870 to 8a33994 Mar 26, 2019

@joeweoj

This comment has been minimized.

Copy link
Contributor Author

joeweoj commented Mar 26, 2019

@jcsirot now the change has been approved, can i ask what the next steps are?
we're obviously keen to get this fix into the next release.
thanks

@ijc ijc merged commit e84ffb6 into docker:master Mar 26, 2019

1 check passed

dco-signed All commits are signed
@ijc

This comment has been minimized.

Copy link
Contributor

ijc commented Mar 26, 2019

Next step is I merge it 😄. Done, thanks!

@joeweoj

This comment has been minimized.

Copy link
Contributor Author

joeweoj commented Mar 26, 2019

lovely. thanks very much!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.