-
Notifications
You must be signed in to change notification settings - Fork 771
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
change strategy to recreate if volumes present, fix #264 #378
Conversation
@containscafeine are you still working on this? |
@surajssd yep |
d1eb888
to
da9c6d7
Compare
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.
This all looks good to me! Got to love how there's more lines of code for testing than the actual feature 👍 LGTM from me
case *deployapi.DeploymentConfig: | ||
objType.Spec.Strategy.Type = deployapi.DeploymentStrategyTypeRecreate | ||
} | ||
} |
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 👍
code LGTM, but wanna give it a try once by running it against application! |
@containscafeine also title says 'no test' change strategy to recreate if volumes present, no tests, fix #264 but you seem to have added tests? |
@containscafeine tested with etherpad application from fixtures and it works! would like to have this behavior documented then we are good to merge this. |
da9c6d7
to
120121f
Compare
@surajssd thanks for pointing that out, added docs. |
When volumes are specified in the Docker Compose files, then in case of Kubernetes, Deployment's Spec.Strategy.Type and in case of OpenShift, DeploymentConfig's Spec.Strategy.Type are set to "Recreate". This is a safer deployment strategy when Volumes are getting used. This commit also adds unit tests for Kubernetes as well as OpenShift, and fixes the failing functional tests (tests.sh) due to the change. No functional tests have been added since the functionality is already covered when the volume mounts are being tested earlier in the file. This fixes kubernetes#264
120121f
to
b73efa5
Compare
@surajssd good to go now? |
No description provided.