-
Notifications
You must be signed in to change notification settings - Fork 13
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
Move the DiscoverySources config section #58
Move the DiscoverySources config section #58
Conversation
791e35e
to
97bd48a
Compare
CI for |
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.
Overall looks good.
Please add the release note in the PR description. Also, we should verify the change with Cross-version API Compatibility Tests
as mentioned in the previous comment.
97bd48a
to
1eb1245
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!
The handling of discovery sources is kept the same but the section of the configuration file in which they are stored is moved. The old section was under "clientOptions.cli.discoverySources" while the new section is now under "cli.discoverySources". This change is necessary as plugins built with the previous version of the runtime library would set a "default" discovery source which the new independent CLI should not use; by moving the discovery sources section in the config file, the new CLI can ignore the old section and trust that the new section only contains valid sources, even when older plugins are used. Signed-off-by: Marc Khouzam <kmarc@vmware.com>
Signed-off-by: Marc Khouzam <kmarc@vmware.com>
1eb1245
to
b32ea59
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 changes
The handling of discovery sources is kept the same but the section of the configuration file in which they are stored is moved. The old section was under "clientOptions.cli.discoverySources" while the new section is now under "cli.discoverySources". This change is necessary as plugins built with the previous version of the runtime library would set a "default" discovery source which the new independent CLI should not use; by moving the discovery sources section in the config file, the new CLI can ignore the old section and trust that the new section only contains valid sources, even when older plugins are used. Signed-off-by: Marc Khouzam <kmarc@vmware.com>
What this PR does / why we need it
This PR moves the DiscoverySources configuration section from "clientOptions.cli.discoverySources" to "cli.discoverySources". The actual handling of discovery sources is kept the same; it is just the section of the configuration file in which they are stored that is moved.
This change is necessary as plugins built with the previous version of the runtime library would set a "default" discovery source which the new independent CLI should not use; by moving the discovery sources section in the config file, the new CLI can ignore the old section and trust that the new section only contains valid sources, even when older plugins are used.
Note that we don't yet modify the CLI to make use of this change but simply start by making sure the
tz plugin source
commands properly work with the new section of discoverySources of the config file. For this to work though PR vmware-tanzu/tanzu-cli#225 must be used for the CLI.Which issue(s) this PR fixes
Fixes #54
Describe testing done for PR
Using PR vmware-tanzu/tanzu-cli#225, I built the CLI and then verified that the
tanzu plugin source
commands work with the new discoverySources section.Release note
Additional information
Special notes for your reviewer