-
Notifications
You must be signed in to change notification settings - Fork 115
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鈥檒l occasionally send you account related emails.
Already on GitHub? Sign in to your account
required-rollout takes a percent #240
Conversation
@@ -168,16 +167,27 @@ def find_latest_rs(deployment_data) | |||
end | |||
|
|||
def min_available_replicas |
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 need to rework this function I dropped in the value from the annotation, but that鈥檚 not the right.
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.
Fixed the logic. Ready for 馃憖
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.
Only a couple very minor quibbles. Overall LGTM.
end | ||
|
||
def percent?(value) | ||
value =~ /%/ |
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.
how about /d+%
?
assert deploy.deploy_succeeded? | ||
|
||
deploy = build_synced_deployment( | ||
template: build_deployment_template(status: deployment_status, rollout: '34%', max_unavailable: 2), |
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.
let's add 0%
and 100%
cases for good measure. We could also leave out max_unavailable
in this test, since it shouldn't matter. Or we could set it to 0%
to prove it isn't being referenced.
def test_validation_with_percent_rollout_annotation | ||
deploy = build_synced_deployment(template: build_deployment_template(rollout: '10%'), replica_sets: []) | ||
deploy.kubectl.expects(:run).with('create', '-f', anything, '--dry-run', '--output=name', anything).returns( | ||
["", "super failed", SystemExit.new(1)] |
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.
If we're trying to prove that this value is valid, why not make this command return 0 and assert deploy_validate_definition
/assert_empty deploy.validation_error_msg
?
@@ -80,6 +80,34 @@ def test_deploy_succeeded_with_max_unavailable | |||
refute deploy.deploy_succeeded? | |||
end | |||
|
|||
def test_deploy_succeeded_with_annotation_as_number |
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.
_as_percent
?
#208 added the
kubernetes-deploy.shopify.io/required-rollout
annotation. There is a corner case whenmaxUnavailable
is0%
and the annotation becomes a no-op. This PR allows a user to specify a number or a percent (e.g.10
or15%
) to the annotation instead of using the value frommaxUnavailable
.Previously there was a concern that allowing users to provide their own value would allow the user to 馃敨 馃懀 . Would it be better if this feature required
maxUnavailable
to be0%
/ only took a%
? That would limit the blast radius.cc: @dwradcliffe