-
Notifications
You must be signed in to change notification settings - Fork 38.6k
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
refactor createAPIServerClient for easier integration with 3rd party … #9423
refactor createAPIServerClient for easier integration with 3rd party … #9423
Conversation
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") If this message is too spammy, please complain to ixdy. |
cc @dchen1107 |
client, err := s.createAPIServerClient() | ||
var apiclient *client.Client | ||
clientConfig, err := s.CreateAPIServerClientConfig() | ||
if err != nil { |
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.
I'm confused about the change you made starting here. To avoid changing the existing logic, shouldn't it be
if err != nil {
...log something...
}
apiclient, err = client.New(clientconfig)
if (err != nil) {
apiclient = nil
}
?
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.
ugh! you're right ... I accidentally blew away my first change for this, and when I re-wrote it I reversed the logic. will remedy ASAP
5fa0f79
to
a05570a
Compare
not sure why shippable is failing, can't see the build logs there (shippable just hangs) |
Some integration test is failing. I'll re-run shippable to see if it's a flake.
|
client, err := s.createAPIServerClient() | ||
var apiclient *client.Client | ||
clientConfig, err := s.CreateAPIServerClientConfig() | ||
if err == nil { |
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.
I would change this to
if (err != nil) {
return err
}
apiclient, err = client.New(clientConfig)
...
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.
will do
On Tue, Jun 9, 2015 at 12:50 AM, David Oppenheimer <notifications@github.com
wrote:
In cmd/kubelet/app/server.go
#9423 (comment)
:@@ -253,7 +253,11 @@ func (s *KubeletServer) Run(_ []string) error {
glog.Warning(err)
}
- client, err := s.createAPIServerClient()
- var apiclient *client.Client
- clientConfig, err := s.CreateAPIServerClientConfig()
- if err == nil {
I would change this to
if (err != nil) {
return err
}
apiclient, err = client.New(clientConfig)
...—
Reply to this email directly or view it on GitHub
https://github.com/GoogleCloudPlatform/kubernetes/pull/9423/files#r31982539
.
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.
@davidopp pretty sure that change is breaking the tests because it causes Run() to abort when there's no API client configured for the kubelet.
634f69c
to
f87bd39
Compare
change looks fine, re-running shippable (was a flake) |
I don't know why this is failing shippable but it does not seem to be a flake. The test that is failing is the second test in "script" namely
Here is the output from the failed run:
Here is the first part of the output from a run (of a different PR) that passes:
It looks like they diverge starting at the message "no api servers specified" |
You might want to try running that test manually on the command line to debug this. |
Thanks, will take a look. On Tue, Jun 9, 2015 at 5:05 PM, David Oppenheimer notifications@github.com
|
client, err := s.createAPIServerClient() | ||
clientConfig, err := s.CreateAPIServerClientConfig() | ||
if err != nil { | ||
return err |
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.
pretty sure that this is the problem. the code never used to work this way
Can you squash your two commits, and we'll let the tests run again, and then I think it will be OK? |
…kubelet extensions, e.g. kubernetes-mesos
e3dceb2
to
f54eeeb
Compare
ok, squashed |
LGTM |
Can one of the admins verify that this patch is reasonable to test? (reply "ok to test", or if you trust the user, reply "add to whitelist") If this message is too spammy, please complain to ixdy. |
refactor createAPIServerClient for easier integration with 3rd party …
…kubelet extensions, e.g. kubernetes-mesos
xref mesosphere/kubernetes-mesos#332
xref #8882 -- this PR is a prerequisite for kubernetes-mesos integration
/cc @davidopp @guenter @bgrant0607 @thockin