-
Notifications
You must be signed in to change notification settings - Fork 1.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
kubernetes: Fix hangs when connecting #7707
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.
This fixes a regression from PR #7585 (released in version 150). Without an auth-provider, the behaviour is now again the same as with ≤ 149. Thus setting priority.
The unused $timeout
looks weird to me. I see that other kubernetes scripts do that too, but it actually is being used in some, so my gut feeling is that you were meaning to wrap the kubectl version
call in a timeout?
@@ -804,8 +804,9 @@ | |||
|
|||
.factory("cockpitKubectlConfig", [ | |||
'$q', | |||
"$timeout", |
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.
Incosistent quoting (but may be irrelevant, see below).
'cockpitRunCommand', | ||
function($q, runCommand) { | ||
function($q, $timeout, runCommand) { |
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.
This isn't being used anywhere, did you mean to use it in the kubectl version
call?
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.
Leftover cruft from my first approach. I'll remove.
Can you please add a "Fixes #7688" to the commit log? (Or let's do it when merging, if the |
e74587d
to
2245aa0
Compare
If a kube config has no user data for a context kubectl prompts. Unfortunately there is no way to stop this. So only run kubectl version when when know we have a user with a auth-provider section. Make sure this is the default user by using --minify to get the current user only. Closes cockpit-project#7707 Fixes cockpit-project#7688
2245aa0
to
77cd408
Compare
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
If a kube config has no user data for a context kubectl prompts. Unfortunately there is no way
to stop this. So only run kubectl version when when know we have a user with a auth-provider
section. Make sure this is the default user by using --minify to get the current user only.