-
Notifications
You must be signed in to change notification settings - Fork 126
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
fix(metrics-operator): provide more information for dynatrace api error #2885
Conversation
Signed-off-by: vickysomtee <[email protected]>
Signed-off-by: vickysomtee <[email protected]>
Signed-off-by: vickysomtee <[email protected]>
Signed-off-by: vickysomtee <[email protected]>
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2885 +/- ##
==========================================
- Coverage 85.36% 85.32% -0.05%
==========================================
Files 167 167
Lines 7412 7412
==========================================
- Hits 6327 6324 -3
- Misses 798 800 +2
- Partials 287 288 +1
... and 1 file with indirect coverage changes
Flags with carried forward coverage won't be shown. Click here to find out more. |
metrics-operator/controllers/common/providers/dynatrace/client/client.go
Show resolved
Hide resolved
Signed-off-by: vickysomtee <[email protected]>
Hey @Vickysomtee the ticket requests a Chainsaw test using mockserver, you can copy the implementation of this test but change it for a failing http response to show what would be printed by the operator. The idea is that the mockserver configuration can return a bad http code, and the result of the operator for example a keptn metric should be the error message you added. |
metrics-operator/controllers/common/providers/dynatrace/client/client_test.go
Show resolved
Hide resolved
@RealAnna can you check the link in your comment? It links to a folder. Did you intend to link to a folder or a file Also, just to note, I didn't introduce new error message. I only added statusCode |
Hey @Vickysomtee indeed, I had in mind you changed also the printed message but apparently not... Anyhow line 106, 111 and 127 could use coverage by unit test. For the chainsaw test this is indeed a folder with multiple files, some file declare what the test wll install on the test environment and the other assert the expected result. YOu can have a look at how it works here |
Hi @Vickysomtee, any news on this one? feel free to reach out if you have any questions |
@bacherfl I had a very tight schedule this past weeks. I am settled now, and I will round up this issue ASAP. |
Signed-off-by: vickysomtee <[email protected]>
test/chainsaw/testmetrics/dynatrace-provider/chainsaw-test.yaml
Outdated
Show resolved
Hide resolved
23786c8
to
cf7662a
Compare
Signed-off-by: vickysomtee <[email protected]>
@RealAnna Done! |
Signed-off-by: vickysomtee <[email protected]>
Hello @RealAnna can you help review again? |
Signed-off-by: vickysomtee <[email protected]>
Signed-off-by: vickysomtee <[email protected]>
Quality Gate passedIssues Measures |
name: podtatometric | ||
labels: | ||
app: podtato-head | ||
status: |
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.
you are getting an empty status in the test result. I would say that's not expected. It should either return the value or error. Can you please adapt it accordingly?
closing due to inactivity |
Description
This PR adds more information to the dynatrace api error response
Fixes #2790
How to test
Please describe how to run the tests that you ran to verify your changes.
Provide instructions so we can reproduce.
Please also provide information about any automatic tests that you added.
Checklist
into multiple PRs)
see Contribution Guide
the Contribution Guide