-
Notifications
You must be signed in to change notification settings - Fork 267
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
improvement(container): configurable deployment strategy #3293
improvement(container): configurable deployment strategy #3293
Conversation
The changes are backwards compatible. The compatibility is ensured by the default values of the new configuration entry `deploymentStrategy`. The default value is "RollingUpdate". Validation ensures the informative error messages.
The new config option is still marked as required, despite there is a default value. Haven't found a way to mark it as an optional in the generated docs.
@@ -742,6 +747,11 @@ const containerServiceSchema = () => | |||
), | |||
addCapabilities: containerAddCapabilitiesSchema("service"), | |||
dropCapabilities: containerDropCapabilitiesSchema("service"), | |||
deploymentStrategy: joi |
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.
The compatibility is ensured by the default value here.
The default value is "RollingUpdate"
.
Validation ensures informative error messages.
@@ -134,6 +138,7 @@ export interface ContainerServiceSpec extends CommonServiceSpec { | |||
tty?: boolean | |||
addCapabilities?: string[] | |||
dropCapabilities?: string[] | |||
deploymentStrategy: DeploymentStrategy |
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 field is not optional, but the changes are backward compatible. There is a default value supplied by the validator, see the comment below.
I had to modify the tests, but it's better to prefer doing extra modifications in tests instead of having an optional field and extra (in fact, redundant) default value logic in the production code.
Converted to draft, there are some test failures. |
What this PR does / why we need it:
This PR makes the deployment strategy configurable for
container
module type.Which issue(s) this PR fixes:
Fixes #3237
Special notes for your reviewer:
Please check my comments in the change set.