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

change strategy to recreate if volumes present, fix #264 #378

Merged
merged 1 commit into from
Feb 21, 2017

Conversation

concaf
Copy link
Contributor

@concaf concaf commented Jan 12, 2017

No description provided.

@k8s-ci-robot k8s-ci-robot added the cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. label Jan 12, 2017
@surajssd
Copy link
Member

surajssd commented Feb 9, 2017

@containscafeine are you still working on this?

@concaf
Copy link
Contributor Author

concaf commented Feb 13, 2017

@surajssd yep

@concaf concaf force-pushed the deployment_strategy branch 2 times, most recently from d1eb888 to da9c6d7 Compare February 13, 2017 19:49
@concaf concaf changed the title [WIP] change strategy to recreate if volumes present, no tests, fix #264 change strategy to recreate if volumes present, no tests, fix #264 Feb 13, 2017
@concaf
Copy link
Contributor Author

concaf commented Feb 13, 2017

@kadel @surajssd @cdrage up for review, PTAL!

Copy link
Member

@cdrage cdrage left a 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
}
}
Copy link
Member

Choose a reason for hiding this comment

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

LGTM 👍

@surajssd
Copy link
Member

code LGTM, but wanna give it a try once by running it against application!

@surajssd
Copy link
Member

@containscafeine also title says 'no test'

change strategy to recreate if volumes present, no tests, fix #264

but you seem to have added tests?

@surajssd
Copy link
Member

@containscafeine tested with etherpad application from fixtures and it works! would like to have this behavior documented then we are good to merge this.

@concaf concaf changed the title change strategy to recreate if volumes present, no tests, fix #264 change strategy to recreate if volumes present, fix #264 Feb 14, 2017
@concaf concaf force-pushed the deployment_strategy branch from da9c6d7 to 120121f Compare February 14, 2017 10:04
@concaf
Copy link
Contributor Author

concaf commented Feb 14, 2017

@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
@concaf concaf force-pushed the deployment_strategy branch from 120121f to b73efa5 Compare February 20, 2017 06:15
@concaf
Copy link
Contributor Author

concaf commented Feb 20, 2017

@surajssd good to go now?

@surajssd surajssd merged commit 786f5c4 into kubernetes:master Feb 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
cncf-cla: yes Indicates the PR's author has signed the CNCF CLA. rebase needed
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants