-
Notifications
You must be signed in to change notification settings - Fork 20
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
Experimental: Get the Tanzu Hub Endpoint based on the Central Config Metadata #764
Experimental: Get the Tanzu Hub Endpoint based on the Central Config Metadata #764
Conversation
Regenerated the test central repos
d1a5e94
to
b00a5cb
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.
Looks great. This gives us much more flexibility.
I on the fence about the config format using 5 separate keys instead of a single key. I see that the code reads one value at a time to make decisions so I see why you used different keys, but I still wonder if it would be more readable to have a single key with a struct; then you'd read the whole struct at the start and use only the content that you need.
I'm only suggesting this because it would make the central_config.yaml
file more readable for a human, which is not something that is critical, so I'm not set on it.
I'll leave it up to you and other reviewers.
LGTM
pkg/auth/csp/tanzu.go
Outdated
// We will get the central configuration from the default discovery source | ||
discoverySource, err := config.GetCLIDiscoverySource(cliconfig.DefaultStandaloneDiscoveryName) | ||
if err != nil { | ||
return "", err |
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.
Corner case, but if the user has done a tanzu plugin source delete default
(which is a hidden command), and error will be return here. Should we fallback to CSP in that case, or should we error out?
fe67ada
to
a10f441
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, thanks for the updates!
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.
Very nice
LGTM!
…Metadata (vmware-tanzu#764) * Update Test Central Config to include Hub metadata * Get the Tanzu Hub Endpoint based on the Central Config Metadata
…Metadata (#764) * Update Test Central Config to include Hub metadata * Get the Tanzu Hub Endpoint based on the Central Config Metadata
What this PR does / why we need it
If
useCentralConfig
isfalse
, Tanzu CLI will use CSP to get the hub endpointcspProductIdentifier
andcspDisplayName
from central config keyscli.core.tanzu_platform_csp_product_identifier
andcli.core.tanzu_platform_csp_display_name
respectively.If
useCentralConfig
istrue
, Tanzu CLI uses the central config keyendpointProduction
to get the endpoint for production andendpointStaging
for staging. If the values are empty it will use default values specified within the CLI.Which issue(s) this PR fixes
Fixes #
Describe testing done for PR
Release note
Additional information
Special notes for your reviewer