-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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
cmd/operator-sdk/new/cmd.go: fallback to default helm RBAC role without kubeconfig #1627
cmd/operator-sdk/new/cmd.go: fallback to default helm RBAC role without kubeconfig #1627
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.
LGTM
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.
Overall lgtm, one question that I would like answered.
roleScaffold := helm.DefaultRoleScaffold | ||
if k8sCfg, err := config.GetConfig(); err != nil { | ||
log.Warnf("Using default RBAC rules: failed to get Kubernetes config: %s", err) | ||
} else if dc, err := discovery.NewDiscoveryClientForConfig(k8sCfg); 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.
What happens here if k8sCfg
is nil
and we pass it to NewDiscoveryClientForConfig
?
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 that case, config.GetConfig()
would have to return nil, nil
, which is probably not something that it would ever do. But better safe than sorry 🙂 . I'll check if the follow-on functions handle nil
for both k8sCfg
and dc
.
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.
Missed the else
part of the if
, nvm. :)
/test marker |
/test e2e-aws-ansible |
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 on green
/test e2e-aws-ansible |
1 similar comment
/test e2e-aws-ansible |
Description of the change:
Updates
operator-sdk new --type=helm
to fallback to default RBAC rules if there is an error getting the kubeconfig or creating a discovery client.Motivation for the change:
Closes #1625