-
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
Switch default discovery source registry to projects.packages.broadcom.com
#755
Conversation
777002f
to
7d2c0ca
Compare
956fbcc
to
be9b8f9
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.
Nicely done. I was worried this would be difficult, but you found a very elegant way to handle this! Nice.
Minor nits but LGTM
Forgot these minor points: Do we need to deal with users who are running v1.3.0-alpha.* or do we ask them to run While at it, do you want to replace the followowign with something "broadcom"?
|
I do not think we need to do anything to support |
be9b8f9
to
8766bd7
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!
8766bd7
to
c76cd41
Compare
|
||
// Considering we are updating the discovery source to point to new registry, | ||
// User must be prompted to access the EULA again. So, let's first unset EULA status | ||
err = config.SetEULAStatus(config.EULAStatusUnset) |
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.
I have always expected when we need to reprompt EULA it will involve (1) setting a new major.minor
in CurrentEULAVersion, and (2) updating the eulaPromptMsg,
instead of explicitly unsetting the status.
Can we consider this a special situation that doesn't follow this pattern?
Could we have just done (1) instead?
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 didn't think about (1) at all before you mentioned it. We can go with approach (1) as well.
The minor difference will be that with the current approach, we do not prompt EULA for the users using the custom registry. But with the approach (1) you mentioned, it will always be prompted for the users. I think it makes sense to prompt the EULA again for all the users as users can use the tanzu plugin download-bundle
which will download images from a new location by default.
So, I am inclined towards updating this PR with approach (1) of updating CurrentEULAVersion
with a new major.minor.
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, I just have a question about the means to the eula retrigger.
Also:
Worth mentioning in some md how to revert back to projects.registry
or you feel this is better done out-of-band?
I added that as part of the release note and might be better to keep it as part of the release note as most of the users should not need to switch back to the old registry. |
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.
I like the new handling of the EULA re-prompt 👍
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.
…om.com` (vmware-tanzu#755) * Switch default registry to projects.packages.broadcom.com * Update CurrentEULAVersion to re-prompt EULA to the users
…om.com` (#755) * Switch default registry to projects.packages.broadcom.com * Update CurrentEULAVersion to re-prompt EULA to the users
What this PR does / why we need it
projects.packages.broadcom.com
.projects.packages.broadcom.com/tanzu_cli
project.registry.vmware.com/tanzu_cli
will be kept for some time for users consuming older versions of Tanzu CLI and bothprojects.packages.broadcom.com/tanzu_cli
andproject.registry.vmware.com/tanzu_cli
.Details:
projects.packages.broadcom.com/tanzu_cli/plugins/plugin-inventory:latest
Which issue(s) this PR fixes
Fixes #
Describe testing done for PR
v1.3.0
version of Tanzu CLI and runs any command for the first time, CLI will update the discovery source and prompt for EULA.Release note
Additional information
Special notes for your reviewer