Open chaozbj opened 4 years ago
@zhanggbj Have you any comments on that? If no objections, I will assign it to myself to do. Thanks!
@chaozbj good finding! Thanks!
about the title, I guess it should be kubectl context
instead of kubelet context
, could you please update it and the description.
Please also help to verify your refine PR works with IKS cluster and minikube, thanks!
/assign
@zhanggbj After I improved the codes to:
// rootCmd represents the base command when called without any subcommands
func NewAdminCommand(params ...pkg.AdminParams) *cobra.Command {
p := &pkg.AdminParams{}
if err := p.Initialize(); err != nil {
fmt.Printf("Failed to create kubernetes client, %+v\n", err)
os.Exit(1)
}
...
I found it will break the existing UT codes of root command since no kube config is available during testing. I checked our codes and knative client codes, got two options:
The params in function NewAdminCommand(params ...pkg.AdminParams)
actually is not used, we can change it and pass a fake pkg.AdminParams
for UT codes, this function will be improved like:
// rootCmd represents the base command when called without any subcommands
func NewAdminCommand(p *pkg.AdminParams) *cobra.Command {
if p == nil {
p = &pkg.AdminParams{}
}
if err := p.Initialize(); err != nil {
fmt.Printf("Failed to create kubernetes client, %+v\n", err)
os.Exit(1)
}
...
version
without a kubenets client still needs user to set a kubeconfig first.Another option is like knative client: NewServingClient func(namespace string) (clientservingv1.KnServingClient, error)
We can declare a member function of AdminParams
to delay creating kubenets client interface till subcommand running. If we use this option:
p *pkg.AdminParams
from NewAdminCommand
functionroot
command version, help
can run successfully even if the kubectl context is not setp.ClientSet
is replaced by a function and won't be created in advance.Which one do you prefer? Thanks!
BTW, you can use kubectl config unset current-context
to reproduce the runtime error
@chaozbj Thanks for the investigation. Briefly, option 2 sounds good to me.
For option 1, I think it doesn't make sense to ask the end-user to specify a Kubeconfig for version
or help
command.
For option 2, it is more reasonable to specify a Kubeconfig when it is really required, and it good to be consistent with kn client itself.
Even though there will be more code change, but I think it does worth it!
Thanks again for the nice summary 👍
@zhanggbj Ok, I also like option 2, I will do it, thanks!
If the kubectl context is not set or run
kubectl config unset current-context
to unset it, we will get runtime error when runkn admin
, for example:I checked the codes in
AdminParams
:I think we can do some improvements for the above functions so that we can avoid outputting runtime error if kubectl context is not set:
Initialize()
, we'd better return error when fail to createClientSet
NewAdminCommand()
, we should check the error fromp.Initialize()
, if error is returned, we can print the error message and exit to run command.