Notifications chef client runs skip ignored failure #634
Conversation
lancewf
force-pushed
the
lancewf/notifiction_skip_ignored_failure
branch
from
f16f4e2
to
b8a7343
Compare
lancewf
added
automate-gateway
notifications
| @@ -385,7 +385,7 @@ func getBoolIfExists(fieldName string, body []byte) bool { | |||
| fieldBytes, _, _, err := jsonparser.Get(body, fieldName) | |||
| if err == nil { | |||
| b, err := jsonparser.ParseBoolean(fieldBytes) | |||
| if err != nil { | |||
| if err == nil { | |||
There was a problem hiding this comment.
This change fixes the bug.
There was a problem hiding this comment.
could you add a comment saying why we do the err == nil check here? i know you included some info about it in the pr, but i think our future selves might appreciate having a comment in the code there
| @@ -169,7 +169,7 @@ func TestDataCollectorParseBytesToChefRunFailure(t *testing.T) { | |||
| assert.Equal(t, "", res1.Delta) | |||
| assert.Equal(t, "insights-test", res1.CookbookName) | |||
| assert.Equal(t, "0.1.1", res1.CookbookVersion) | |||
| assert.Equal(t, false, res1.IgnoreFailure.GetBoolValue()) | |||
| assert.Equal(t, true, res1.IgnoreFailure.GetBoolValue()) | |||
There was a problem hiding this comment.
This is the only real change to this test. The other tests are the go simplification.
| @@ -3127,7 +3127,7 @@ | |||
| "before": {}, | |||
| "duration": "0", | |||
| "delta": "", | |||
| "ignore_failure": false, | |||
| "ignore_failure": true, | |||
There was a problem hiding this comment.
This allows the above test to check if the valve is converted correctly.
Signed-off-by: Lance Finfrock <lfinfrock@chef.io>
lancewf
force-pushed
the
lancewf/notifiction_skip_ignored_failure
branch
from
b8a7343
to
27a9ca9
Compare
lancewf
changed the title
Signed-off-by: Lance Finfrock <lfinfrock@chef.io>
| @@ -34,6 +34,24 @@ | |||
| end | |||
| end | |||
|
|
|||
| #converge-failure-with-ignored-failure-report_run_converge | |||
| let(:failed_ccr_message_with_ignored_failure) do | |||
| return @failed_ccr_json ||= begin | |||
There was a problem hiding this comment.
Memoizing with an instance variable inside a let block is redundant, and it is dangerous in this case since the instance variable will be set by the first let to be referenced.
| expect(after_request_count).to eq(before_request_count + 1) | ||
|
|
||
| # get the last sent notification | ||
| notification = JSON.parse(request.body, symbolize_names: true)[:requests][before_request_count] |
There was a problem hiding this comment.
Nitpick: instead of accessing this by indexing into the array, you could replace [before_request_count] with .last.
There was a problem hiding this comment.
I always forget how friendly Ruby is with lists.
🔩 Description
Notifications from failed CCRs should skip resource failures that are marked 'ignore_failure' = true. The bug was found in the gateway CCR parsing code. When trying to parse the boolean flag the error variable was checked incorrectly.
👟 Demo Script / Repro Steps
start_all_servicesJSON_FILE=/src/inspec/a2-api-integration/files/fixtures/converge/converge-failure-report_with_ignored_failure_run_converge.json send_chef_run_example lbrebuild components/automate-gatewayJSON_FILE=/src/inspec/a2-api-integration/files/fixtures/converge/converge-failure-report_with_ignored_failure_run_converge.json send_chef_run_example lb⛓️ Related Resources
#529
✅ Checklist