-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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 panic setting ingress status #3492
Conversation
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.
LGTM
Is this fix testable?
@errm You mean like can I build an image to test with? Or can a unit test be written to test this? Yes to the first, but the 2nd not sure. It relies on the response from the client, not sure how to mock that response in any meaningful manner... |
Yes I meant a unit test As this code lives inside of the client it does seem quite hard to test.... Just asking the question though because on the whole adding unit tests to cover regressions like these stop us from re-introducing the same bugs later during refactoring. But having looked closely at this method, I don't see how to do that without refactoring the logical parts into another function... |
In total agreement. |
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.
Theoretically, we could unit test the code in question. Practically, we'd likely have to test against a custom, rather complex mock of ours that mimics client-go.
Eventually we'll need integration tests, though client-go's fake implementation could complete the picture. But that'd be a bigger endeavor.
For now: LGTM.
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.
LGTM 👏
What does this PR do?
Fixes a panic when trying to compare a non-existent ingress status with the expected status
Motivation
Fixes #3490