Add error handling when updating non-existing record

[QuantumDancer]

This is my proposed solution to fix #305

I tried to only catch this specific exception. All other exceptions should still be passed along.

Please let me know if we should do things differently.

Also, I was not sure how I can create a unit test for this specific case, because the auditor server is not directly mocked if I understood the test setup correctly.

I tested this in Freiburg in production and found no issues with this fix so far.

[codecov-commenter]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (2bf9dcb) 98.84% compared to head (19ca892) 98.85%.

❗ Current head 19ca892 differs from pull request most recent head e513aac. Consider uploading reports for the commit e513aac to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##           master     #306   +/-   ##
=======================================
  Coverage   98.84%   98.85%           
=======================================
  Files          56       56           
  Lines        2345     2356   +11     
=======================================
+ Hits         2318     2329   +11     
  Misses         27       27           

Files Changed	Coverage Δ
tardis/plugins/auditor.py	100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[giffels]

Thanks a lot for the contribution. Without checking any details yet, could you add a unittest for that use-case as well, please?
You could potentially use cls.mock_auditorclientbuilder_patcher to mock the behaviour. I think you can basically use the side_effect method of the mock self.mock_auditorclientbuilder to throw the exception, when updating the record.

self.client.update.side_effect = RuntimeError(...)

run_async(
            self.plugin.notify,
            state=DownState(),
            resource_attributes=self.test_param,
        )

"plus check the result and add further tests here to test re-raising etc."

self.client.update.side_effect = None

[QuantumDancer]

Thanks for the instructions regarding the unit testing. I've now added tests that should cover all three cases. Please let me know, I should change something else.

[giffels]

LGTM! I have added a unittest for the case of a non-matching RegEx.

[giffels]

This not related o the plugin code, but to Auditor instead. Should the returned HTTP error not be 404 -Not Found instead, when trying to update a non-existent resource?

[QuantumDancer]

Yes, I think you are right. This might need a little bit of time to get fixed in Auditor due to the holiday season. I've opened a new issue ALU-Schumacher/AUDITOR#378. I plan to include this change in the v0.2.0 release of Auditor. Once released, we will create a PR for tardis that includes the version update and updates to all breaking changes.

[maxfischer2781]

I've added some suggestions for smoothing the error detection. Since I'm guessing most people looking at the code won't actually know the exact error message, it would be good to have that as straightforward as possible.
FWIW, I don't know the error message myself so if you have an even better approach, go for it.

[maxfischer2781]

It looks like these double-if could be folded into one, making it more obvious that you are really just checking one thing and unconditionally raise otherwise.

Either...

[maxfischer2781]

match and check the code:

Suggested change

	reqwest_error_match = REQWEST_ERROR.match(str(e))
	if reqwest_error_match:
	http_error_code = reqwest_error_match.group(1)
	if http_error_code == "400":
	error_match = REQWEST_ERROR.match(str(e))
	if error_match and error_match.group(1) == "400:
	...

(this could also do with an assignment expression, but is rather dense then)
or ...

[maxfischer2781]

make a literal match against the error, since effectively this is what you are doing:

Suggested change

	reqwest_error_match = REQWEST_ERROR.match(str(e))
	if reqwest_error_match:
	http_error_code = reqwest_error_match.group(1)
	if http_error_code == "400":
	if str(e).startswith("Reqwest Error: HTTP status client error (400 Bad Request)"):

In this case I would recommend to create a new constant for the error message instead of the pattern. Note that I've guessed the necessity for .startswith from the pattern's trailing .*, it may be unnecessary or not.

[giffels]

@maxfischer2781 thanks and 👍 for your remarks. Just a reminder that assignment expressions would not work until #304 is approved and merged.

[QuantumDancer]

Thanks for the suggestion. I agree that maybe the error message matching was slightly overengineered for this case 😉 I've replaced the regex logic with a simple str(e).startswith(...).

[giffels]

LGTM!

[maxfischer2781]

Thanks for the contribution and final touch-ups. Good to go.