-
Notifications
You must be signed in to change notification settings - Fork 22
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: Set the Tanzu Hub endpoint when creating tanzu
context
#734
Experimental: Set the Tanzu Hub endpoint when creating tanzu
context
#734
Conversation
e1f5777
to
01765fc
Compare
tanzu
contexttanzu
context
ec5baff
to
03c7b3b
Compare
pkg/command/context.go
Outdated
if err != nil { | ||
log.V(7).Infof("unable to get Tanzu Hub endpoint. Error: %v", err.Error()) | ||
} else { | ||
c.AdditionalMetadata[config.TanzuHubEndpointKey] = tanzuHubEndpoint |
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 sets a new field in the metadata but in the call below updateTanzuContextMetadata
I think we completely overwrite the metadata with the one from the existing context and we loose the tanzu hub field.
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.
Amm. We only overwrite the metadata if the same context already exists right? So, will this be an issue for new contexts?
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.
can we move this to after L666?
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.
Good point @anujc25. Could the endpoint change on a re-login?
Upgrading an existing context when we first release this feature would be impacted but maybe that’s not a big deal?
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.
Sorry, I didn't see the further discussion on this thread. But I have updated the PR to account for re-login with the recent change.
} | ||
|
||
for _, s := range services.ServicesList { | ||
if s.ProductIdentifier == "TANZU-SAAS" && strings.Contains(s.DisplayName, "Tanzu Application Platform") { // TODO: Can this be improved to use some unique id? |
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.
Not blocking, but maybe as a followup:
This looks like a check that could use some flexibility.
That is, if any of the string patterns could be subjected to change. Perhaps using a default set now, but having it be updatable via Central config could be an option?
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.
Good point. I will create a follow up PR to use Central Configuration Option to fetch this details.
pkg/command/context.go
Outdated
if err != nil { | ||
log.V(7).Infof("unable to get Tanzu Hub endpoint. Error: %v", err.Error()) | ||
} else { | ||
c.AdditionalMetadata[config.TanzuHubEndpointKey] = tanzuHubEndpoint |
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.
can we move this to after L666?
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, modulo the go dep update
8282cdd
to
47752df
Compare
vmware-tanzu#734) * Fetch Tanzu Hub endpoint from CSP when creating the `tanzu` context
#734) * Fetch Tanzu Hub endpoint from CSP when creating the `tanzu` context
What this PR does / why we need it
tanzu
contextWhich issue(s) this PR fixes
Fixes #
Describe testing done for PR
Release note
Additional information
Special notes for your reviewer