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

Fix panic setting ingress status #3492

Merged
merged 1 commit into from Jun 14, 2018
Merged

Conversation

dtomcej
Copy link
Contributor

@dtomcej dtomcej commented Jun 13, 2018

What does this PR do?

Fixes a panic when trying to compare a non-existent ingress status with the expected status

Motivation

Fixes #3490

Copy link
Contributor

@errm errm left a 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?

@dtomcej
Copy link
Contributor Author

dtomcej commented Jun 13, 2018

@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...

@errm
Copy link
Contributor

errm commented Jun 13, 2018

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...

@dtomcej
Copy link
Contributor Author

dtomcej commented Jun 13, 2018

In total agreement.

Copy link
Contributor

@timoreimann timoreimann left a 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.

Copy link
Contributor

@nmengin nmengin left a comment

Choose a reason for hiding this comment

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

LGTM 👏

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants