-
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
Allow getting PluginGroup and installing plugins with PluginGroup with vMAJOR
or vMAJOR.MINOR
or vMAJOR.MINOR.PATCH
versions
#494
Conversation
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 looks great!
Some very minor nits and two bigger points below.
-
The behaviour when using shortened versions with air-gap download should be discussed (the change is probably from Allow installing plugins with
vMAJOR
orvMAJOR.MINOR
orvMAJOR.MINOR.PATCH
versions #486). From what I can see the groupvmware-tkg/shortversion:v1.1.1
contains plugins with a shortened version ofv1
. Using that group, when I run atz plugin download-bundle --group vmware-tkg/shortversion:v1.1.1
I see that it download all plugins that match versionv1
. I was wondering if it should instead only download the latest patch that matchesv1
, as we do forplugin install
. -
While trying this out I notice that when we use a shortened version (for plugins or groups), a pre-release is considered newer than the previous patch. I wonder if that is what we want? For example, in the local test repos we have the
secret
plugin with versionsv1.9.1
andv1.9.2-beta.1
and when install the shorted versionv1.9
it installsv1.9.2-beta.1
. From an end-user perspective, I don't think this is correct.
If you agree, we can open an issue for that and not delay this PR.
…sion is used in PluginGroup
b927a5b
to
49fece8
Compare
Thanks for pointing this out. I would agree with the suggestion of only downloading latest minor/patch version of the plugin in this case. I have made minor changes in the latest commit to handle this situation. Please take a look.
I will file a separate issue to handle this as this might require more discussion. |
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! Thanks @anujc25 .
LGTM
What this PR does / why we need it
vMAJOR
orvMAJOR.MINOR
orvMAJOR.MINOR.PATCH
versionsWhich issue(s) this PR fixes
Fixes #493
Describe testing done for PR
Release note
Additional information
Special notes for your reviewer