Skip to content
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

Modifying Kiali Client methods to use getRequestAndUnmarshalInto to avoid masking of errors returned by the server #7215

Merged
merged 9 commits into from Apr 1, 2024

Conversation

PrinsonF77
Copy link
Contributor

Describe the change

Kiali client methods would try to unmarshall response from the server without checking the status code. This would mask the actual error sent by the server and make debugging difficult
This PR modifies Kiali Client methods to use getRequestAndUnmarshalInto to avoid masking of errors returned by the server.

Steps to test the PR

Run Integration tests for the backend. Changes in this PR are made to the methods used by these tests.

Automation testing

N/A

Issue reference

#6777

getRequestAndUnmarshalInto to avoid masking of errors returned by
the server (kiali#6777)
…ceHealth

To address some tests that use these methods and require the error returned from the server to be NIL.
Some tests that use this method expect no error to be sent
by the server, on non 200 status codes.
@PrinsonF77
Copy link
Contributor Author

@nrfox One thing I noticed while making these changes is that there are some tests, that check for the invalid condition.
For example,
https://github.com/kiali/kiali/blob/076f52e7a7474d8af7b0e2c8c3a3b5addc60242b/tests/integration/tests/namespaces_test.go#L37C1-L46C2

These tests require that the error returned by the client method is NIL. But modifying the client method to use getRequestandUnMarshalInto returns an error on a non-200 error code. Currently, I have modified the kiali client methods that are called by such tests to use the old code, as that does not return an error. But ideally, it looks like a better idea to me, to modify these tests to expect an error to be returned (with a specific code/ message). What are your thoughts on this?

@nrfox
Copy link
Contributor

nrfox commented Mar 25, 2024

But ideally, it looks like a better idea to me, to modify these tests to expect an error to be returned (with a specific code/ message). What are your thoughts on this?

That sounds good. You could go a few different ways with that. 1. Return some kind of custom error type from getRequestAndUnmarshalInto that has the code/message that the tests can use if they want to do assertions on the response code. 2. Have some lower level client method like DoRequest where the tests are more verbose but you have more control over the request and assertions on the response. Either way (or something else that accomplishes the same thing) is fine with me. The ultimate goal of this change is to not hide the underlying error by unmarshaling into a bad response.

@nrfox
Copy link
Contributor

nrfox commented Mar 25, 2024

Looks like you'll need to rebase as well to fix conflicts

@PrinsonF77
Copy link
Contributor Author

@nrfox Can you please review the PR.

@nrfox nrfox marked this pull request as ready for review April 1, 2024 12:29
Copy link
Contributor

@nrfox nrfox left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good, thanks for the contribution!

@nrfox nrfox merged commit 6d88138 into kiali:master Apr 1, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants