-
Notifications
You must be signed in to change notification settings - Fork 108
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
[SHIPA-2627] omit default backoffLimit value #242
Conversation
eb8695b
to
1436529
Compare
1436529
to
11a82f2
Compare
fix tests & template nil check adds tests add tests
11a82f2
to
15f11c4
Compare
@@ -0,0 +1,72 @@ | |||
package v1beta1 |
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.
Added these tests just to keep coverage up. Coverage dipped in a couple packages.
@@ -109,6 +110,24 @@ func TestJobListNames(t *testing.T) { | |||
}, | |||
wantOut: []string{"hello"}, | |||
}, | |||
{ | |||
name: "filter", |
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 test and the one below it share the same name, so it might be better to change the names to something like filter - no match
and filter-success
.
@@ -23,8 +23,6 @@ Deploy a job. | |||
const ( | |||
defaultJobVersion = "v1" | |||
defaultJobParallelism = 1 | |||
defaultJobCompletions = 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.
Was this supposed to be deleted?
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.
Two minor nitpicks, looks good otherwise. Thanks for adding those tests to types!
@@ -12,7 +12,7 @@ spec: | |||
{{- if $.Values.job.completions }} | |||
completions: {{ $.Values.job.completions }} | |||
{{- end }} | |||
{{- if $.Values.job.backoffLimit }} | |||
{{- if not (kindIs "invalid" $.Values.job.backoffLimit) }} |
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.
nice!
Description
Removes redundant
defaultBackoffLimit
from CLI. K8s sets the default to 6 when empty. MakesbackoffLimit
a pointer, permitting passing a0
in.Fixes # shipa-2627
Type of change
Testing
Documentation
Final Checklist:
Additional Information (omit if empty)
Please include anything else relevant to this PR that may be useful to know.