Check jobs for failure status condition #355
Conversation
| @@ -13,6 +13,9 @@ def deploy_succeeded? | |||
| def deploy_failed? | |||
| return false unless deploy_started? | |||
| return false unless @instance_data.dig("spec", "backoffLimit").present? | |||
| (@instance_data.dig("status", "conditions") || []).each do |condition| | |||
| return true if condition["type"] == 'Failed' && condition['status'] == "True" | |||
| end | |||
There was a problem hiding this comment.
Nit: can you extract this into a method?
This condition also has richer information about the reason for the failure in its reason/message fields. Let's adjust failure_message to include that information if we have it.
Can you also add tests? Maybe unit tests that show deploy_failed? is true with either of the statuses I discovered are possible on the issue?
dturn
force-pushed
the
job-failure-condition
branch
from
55d2961
to
294aefc
Compare
| private | ||
|
|
||
| def failed_status_condition | ||
| (@instance_data.dig("status", "conditions") || []).detect do |condition| |
There was a problem hiding this comment.
nit: this would be easier to read if it used a guard clause instead of the || [] trick. I personally prefer find over detect, but I see we already use both in this repo.
| @@ -30,8 +31,20 @@ def status | |||
| end | |||
| end | |||
|
|
|||
| def failure_message | |||
| if (condition = failed_status_condition.presence) | |||
There was a problem hiding this comment.
Its a habit from a style checker, but basically saying I'm intentionally using = and didn't mean to use ==
| end | ||
|
|
||
| def test_job_fails_without_failed_status_condition | ||
| status = { |
There was a problem hiding this comment.
nit: the data this contains isn't a status in this test
| job = KubernetesDeploy::Job.new(namespace: 'test', context: 'nope', definition: template, | ||
| logger: @logger) | ||
| job.deploy_started_at = Time.now.utc | ||
| mediator = KubernetesDeploy::SyncMediator.new(namespace: 'test', context: 'minikube', logger: logger) |
There was a problem hiding this comment.
nit: I don't think this could change anything, but you're using two different fake context names here and two lines above this (and accessing the logger differently)
There was a problem hiding this comment.
This was actually copied from the pod test, I'll fix both.
|
Screen shot of the error message: |
dturn
force-pushed
the
job-failure-condition
branch
from
cbbdb4c
to
bbd97a3
Compare
dturn
force-pushed
the
job-failure-condition
branch
from
bbd97a3
to
aa5b49c
Compare
test_jobs_can_fail is flakey due to not checking the status condition of jobs. So let's check it.
fixes: #350