-
Notifications
You must be signed in to change notification settings - Fork 4.7k
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
diagnostics: fail gracefully on broken kubeconfig #4737
diagnostics: fail gracefully on broken kubeconfig #4737
Conversation
continuous-integration/openshift-jenkins/merge SUCCESS (https://ci.openshift.redhat.com/jenkins/job/merge_pull_requests_origin/3357/) (Image: devenv-fedora_2375) |
@@ -23,6 +25,9 @@ func (o DiagnosticsOptions) detectClientConfig() (bool, []types.DiagnosticError, | |||
func (o DiagnosticsOptions) buildRawConfig() (*clientcmdapi.Config, error) { | |||
kubeConfig, configErr := o.Factory.OpenShiftClientConfig.RawConfig() | |||
if len(kubeConfig.Contexts) == 0 { | |||
if configErr == 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.
This will be easier to reason about and more consistent if you move this check above the len() == 0
check.
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.
if configErr != nil{
return nil, configErr
}
if len....
Merge tag removed to give a little time for review. |
@@ -23,6 +25,9 @@ func (o DiagnosticsOptions) detectClientConfig() (bool, []types.DiagnosticError, | |||
func (o DiagnosticsOptions) buildRawConfig() (*clientcmdapi.Config, error) { | |||
kubeConfig, configErr := o.Factory.OpenShiftClientConfig.RawConfig() | |||
if len(kubeConfig.Contexts) == 0 { | |||
if configErr == nil { | |||
configErr = errors.New("No contexts found in config file.") | |||
} | |||
return nil, configErr | |||
} | |||
return &kubeConfig, configErr |
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.
That way down here, you can simply do return &kubeConfig, nil
so its very clear to readers that this is the success path.
60ae05a
to
f6aa697
Compare
@deads2k better? |
lgtm [merge] |
f6aa697
to
85c8fcd
Compare
Evaluated for origin merge up to 85c8fcd |
[Test]ing while waiting on the merge queue |
continuous-integration/openshift-jenkins/test SUCCESS (https://ci.openshift.redhat.com/jenkins/job/test_pull_requests_origin/5069/) |
Evaluated for origin test up to 85c8fcd |
Merged by openshift-bot
fixes #4578