Fix ctxName by adding a non-global val

[Jeremy-Boyle]

What this PR does / why we need it

Which issue(s) this PR fixes

Fixes #825

Describe testing done for PR

This fixes the ctxName which is currently a global value getting lost when NewRootCMD is called. Causing the new context to fail

Additional information

Special notes for your reviewer

A rework would probably be best to handle this due to how the NewRootCMD gets called and to not use a global value for CTX name, however, due to not wanting to refactor and make a specific type and change signatures this is the best option that i could come up with please review and make suggestions.

[Jeremy-Boyle]

Issue is specifically here:

tanzu-cli/pkg/command/context_tkg_helper.go

Line 69 in 16cbcf1

rootCmd, err := NewRootCmd()

You can see it when you run debug and manually step and see ctxtName being lost.

This is due to how Cobra works.

[prkalle]

LGTM. Thanks for the changes!

[anujc25]

LGTM. Thanks for the fixing this.

As you have already mentioned this is a temporary fix but we might need to fix this properly by not invoking NewRootCmd at all.

The way we are creating NewRootCmdwill wipeout many other variables and not just ctxName. Basically when we do NewRootCmd it will initialize all the commands again along with default values for example this line will reinitialize ctxName to "" string when command gets reinitialized.

@Jeremy-Boyle we will look into fixing this properly soon but we can go ahead and merge this to unblock you.

[anujc25]

After some discussion with the team we are exploring the proper fix to this issue. Until then we will be holding back on this PR from merging.

[Jeremy-Boyle]

Okay sounds great ! I was thinking the same thing but it looked like it would need a bit of refactoring if NewRootCmd wasn't changed hence why I did what I did. Happy to help !

[Jeremy-Boyle]

Another thing to note here a great improvement would be setting the tanzu-cli context that gets created via idp to something useful.

for example with pinniped and tgks:
tanzu-cli-2ffcf2bc-4400-45a1-9868-39bf6740702b@2ffcf2bc-4400-45a1-9868-39bf6740702b

This isn't exactly useful at all if you have multiple environments, I glanced over some of the code, but didn't notice anything useful that was being provided while stepping, I wasn't able to directly find where or how that pinniped plugin is being built. I assume its a private repo.

Suggestion might be utilizing the endpoint for something useful but parse the http:// or another environment variable that would allow the user to change it.

examples:

tanzu context create tkgs-dev --endpoint https://some-dns-name.com
[email protected]

# Or the ip respectively
tanzu-cli-2ffcf2bc-4400-45a1-9868-39bf6740702b@ip-addr

# Or potentially use both along with ctxName
[email protected]

I've been doing the following as a work around for the issue in the mr , but also to name the context something useful that then could be used for access to the supervisor. I did take a look around at the wcp nginx proxy, but unfortunately its not passing anything that I can see that would be useful to get the actual supervisor name from vCenter.

# This will fail out and create a new context in your kubeconfig
tanzu context create  context-name --endpoint https://some.fqdn.com
# Rename the newly created context name to something useful
kubectl config rename-context tanzu-cli-{guid-id}@{guid-id} new-context-name
# Add it to tanzu context create
tanzu context create context-name --kubeconfig ~/.kube/config --kubecontext new-context-name

I'd be happy to provide a MR and move this discussion to a new issue/MR if its something that you think would be a improvement.

[anujc25]

Okay sounds great ! I was thinking the same thing but it looked like it would need a bit of refactoring if NewRootCmd wasn't changed hence why I did what I did. Happy to help !

Thanks for the investigation and root causing the issue. It made the fix much easier for us. We will mostly go ahead with #840 change to fix the mentioned issue and close this one once that is merged.

[anujc25]

Another thing to note here a great improvement would be setting the tanzu-cli context that gets created via idp to something useful.

for example with pinniped and tgks: tanzu-cli-2ffcf2bc-4400-45a1-9868-39bf6740702b@2ffcf2bc-4400-45a1-9868-39bf6740702b

This isn't exactly useful at all if you have multiple environments, I glanced over some of the code, but didn't notice anything useful that was being provided while stepping, I wasn't able to directly find where or how that pinniped plugin is being built. I assume its a private repo.

Suggestion might be utilizing the endpoint for something useful but parse the http:// or another environment variable that would allow the user to change it.

examples:

tanzu context create tkgs-dev --endpoint https://some-dns-name.com
[email protected]

# Or the ip respectively
tanzu-cli-2ffcf2bc-4400-45a1-9868-39bf6740702b@ip-addr

# Or potentially use both along with ctxName
[email protected]

I've been doing the following as a work around for the issue in the mr , but also to name the context something useful that then could be used for access to the supervisor. I did take a look around at the wcp nginx proxy, but unfortunately its not passing anything that I can see that would be useful to get the actual supervisor name from vCenter.

# This will fail out and create a new context in your kubeconfig
tanzu context create  context-name --endpoint https://some.fqdn.com
# Rename the newly created context name to something useful
kubectl config rename-context tanzu-cli-{guid-id}@{guid-id} new-context-name
# Add it to tanzu context create
tanzu context create context-name --kubeconfig ~/.kube/config --kubecontext new-context-name

I'd be happy to provide a MR and move this discussion to a new issue/MR if its something that you think would be a improvement.

Can you please file a Github Issue for this with your suggestion so that we can discuss more on a separate issue? I will bring this to the teams attention as well to get more opinions on this.

[anujc25]

#840 is merged. I am closing this PR now. We will cherry-pick the same fix to release 1.5 branch so that this can be included in v1.5.2 release happening this month.

[Jeremy-Boyle]

Another thing to note here a great improvement would be setting the tanzu-cli context that gets created via idp to something useful.
for example with pinniped and tgks: tanzu-cli-2ffcf2bc-4400-45a1-9868-39bf6740702b@2ffcf2bc-4400-45a1-9868-39bf6740702b
This isn't exactly useful at all if you have multiple environments, I glanced over some of the code, but didn't notice anything useful that was being provided while stepping, I wasn't able to directly find where or how that pinniped plugin is being built. I assume its a private repo.
Suggestion might be utilizing the endpoint for something useful but parse the http:// or another environment variable that would allow the user to change it.
examples:

tanzu context create tkgs-dev --endpoint https://some-dns-name.com
[email protected]

# Or the ip respectively
tanzu-cli-2ffcf2bc-4400-45a1-9868-39bf6740702b@ip-addr

# Or potentially use both along with ctxName
[email protected]

I've been doing the following as a work around for the issue in the mr , but also to name the context something useful that then could be used for access to the supervisor. I did take a look around at the wcp nginx proxy, but unfortunately its not passing anything that I can see that would be useful to get the actual supervisor name from vCenter.

# This will fail out and create a new context in your kubeconfig
tanzu context create  context-name --endpoint https://some.fqdn.com
# Rename the newly created context name to something useful
kubectl config rename-context tanzu-cli-{guid-id}@{guid-id} new-context-name
# Add it to tanzu context create
tanzu context create context-name --kubeconfig ~/.kube/config --kubecontext new-context-name

I'd be happy to provide a MR and move this discussion to a new issue/MR if its something that you think would be a improvement.

Can you please file a Github Issue for this with your suggestion so that we can discuss more on a separate issue? I will bring this to the teams attention as well to get more opinions on this.

Created for our discussion, its mostly the same comment :) #843