-
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
Updating Kubernetes tests to properly test missing endpoints code path #1436
Updating Kubernetes tests to properly test missing endpoints code path #1436
Conversation
As a heads up when I run the provider tests locally I get the following failed test:
This PR doesn't touch anything relating to that test. Curious if it fails in Travis. Haven't looked into why but I am running from head. |
provider/kubernetes_test.go
Outdated
Weight: 1, | ||
}, | ||
}, | ||
Servers: map[string]types.Server{}, |
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.
What's the reason we are able to remove / empty a bunch of server definitions?
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.
Oh of course, it's because we now model missingness correctly, right?
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.
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.
In my memory, things seemed much more complicated when we tried to do this the first time back on the v1.2 branch. Must have memorized it incorrectly.
LGTM!
One of the annoying things was at first I just left the |
@regner 👍 In the referencing issue, you also mention the fact that we keep frontends dangling when no Endpoints can be found. Do you know what the effect of that is? And should we address it in this PR or a different one? (I'm open to both paths.) |
I don't know what the effect of that is and am unsure how that should be handled. I think it needs some discussion first so should probably be a different issue and PR. |
@regner makes sense to me. |
@regner could you rebase and fix the conflicts? We recently moved all providers into separate sub-packages. Sorry for the inconveniences! |
1385fa3
to
cf08bbe
Compare
cf08bbe
to
d24ba90
Compare
This fixes #1307