Discover Plugins from all active contexts and Support Plugin Name conflicts across different Targets #3961
Conversation
anujc25
force-pushed
the
support-plugin-name-conflicts
branch
from
0ae63bc
to
4246829
Compare
Codecov Report
@@ Coverage Diff @@
## main #3961 +/- ##
==========================================
- Coverage 48.22% 47.57% -0.65%
==========================================
Files 433 457 +24
Lines 43122 44740 +1618
==========================================
+ Hits 20795 21287 +492
- Misses 20343 21448 +1105
- Partials 1984 2005 +21
Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here. |
anujc25
changed the title
There was a problem hiding this comment.
Thanks for the change @anujc25, I'm starting with reviewing the docs and posting comments immediately to go faster
anujc25
force-pushed
the
support-plugin-name-conflicts
branch
3 times, most recently
from
e63f022
to
5900ad4
Compare
anujc25
changed the title
anujc25
force-pushed
the
support-plugin-name-conflicts
branch
from
5900ad4
to
7bd4954
Compare
|
I noticed that Say I use a TMC target and remove some plugins I don't want to use ( |
anujc25
force-pushed
the
support-plugin-name-conflicts
branch
from
7bd4954
to
e8c1f3b
Compare
Currently, we do not provide The reason is, |
anujc25
force-pushed
the
support-plugin-name-conflicts
branch
from
e8c1f3b
to
70fdd6f
Compare
Ah ok. We don't provide it, but the PR does add a new
Aha, yes that makes sense (I'm still wrapping my brain around the subtle details of this change). So, if I create a standalone plugin that targets TMC, and I list and eventually install it using the |
anujc25
force-pushed
the
support-plugin-name-conflicts
branch
2 times, most recently
from
204974e
to
69c2550
Compare
| matchedCmd := findSubCommand(RootCmd, cmd) | ||
| if matchedCmd == nil { // If the subcommand for the plugin doesn't exist add the command | ||
| RootCmd.AddCommand(cmd) | ||
| } else if plugins[i].Scope == common.PluginScopeContext && isStandalonePluginCommand(matchedCmd) { |
There was a problem hiding this comment.
What is the use case we specifically worry about here, that requires this logic?
I'm asking because I'm trying to see if we need similar logic in addPluginsToCtxType()
|
@anujc25 I think we should hold off on adding the top commit "Update helptext template to update group name with plugins". That commit is unrelated to this PR but more importantly it makes a user-visible change that I think requires a bit of discussion. I personally would open an issue about it and post it as an independent PR so that it can get the proper attention and discussion. |
|
Deleting a plugin that has a target has some issues. It works if I specify the |
anujc25
force-pushed
the
support-plugin-name-conflicts
branch
from
2abeea4
to
646046b
Compare
This is required because of k8s targeted plugin still relies on
`current` field and we do not want to update that field when context for
tmc targeted context changes.
Scenario:
1. config.yaml looks like below
```
current: kind-test-cluster-2
currentContext:
tmc: tmc-unstable
k8s: kind-test-cluster-2
```
2. tanzu tkr get or tanzu k8s tkr get talks to kind-test-cluster-2
and returns the result.
3. tanzu tmc iam command should use currentContext.tmc and talk to
tmc-unstable endpoint.
4. If user does, tanzu context use tmc-dev to point to new tmc dev
endpoint, based on the existing behavior it will update the
currentContext.tmc as well as current to point to tmc-dev .
```
current: tmc-dev
currentContext:
tmc: tmc-dev
k8s: kind-test-cluster-2
```
5. Because, old tkr plugin still uses current to determine the
cluster to talk to tanzu k8s tkr get will not work anymore as
it is pointing to tmc endpoint.
anujc25
force-pushed
the
support-plugin-name-conflicts
branch
from
ee56daa
to
f466a62
Compare
|
/test install-vc7 |
|
Cluster Generation A/B Results: |
|
Cluster Generation A/B Results: |
|
/test install-vc7 |
|
Cluster Generation A/B Results: |
|
/test install-vc7 |
|
Cluster Generation A/B Results: |
anujc25
force-pushed
the
support-plugin-name-conflicts
branch
from
5221503
to
fea415e
Compare
|
/test install-vc7 |
|
Cluster Generation A/B Results: |
|
@anujc25 I think |
|
Cluster Generation A/B Results: |
vuil
added
area/core-cli
ok-to-merge
What this PR does / why we need it
Discover Plugins from all active contexts:
current(currentServer) field. But with targets implementation, we can have one active context per target. Meaning, we need to discover plugins from multiple active contexts.pluginmanagerAPIs to not takeserverNameas parameter but fetch the active contexts as part of internalDiscoverServerPluginsfunction and provide the result based on the active contexts instead of just singlecurrent(currentServer) field.Support Plugin Name conflicts across different Targets
PluginNameirrespective of whether it is context-scoped plugin, standalone plugin or belongs to a different target group. Meaning, only one plugin of a name can be installed at a time. However, there is expectation that there can be different plugins of the same name (regardless of how they are discovered), each associated with a different target level.PluginNameandTargetof the plugin. This means, plugin names must be Unique per Target Level.Below are the high-level summary of the changes:
Targetfield in CLIPlugin CR for determining Target of the standalone pluginsCatalogto usePluginName_Targetas a key to store plugin details in the mapplugin-managerlifecycle APIs to not acceptServerNameas input but find the current server information as part ofDiscoverServerPluginsAPI. If thecontext-targetfeature-flag is enabled,DiscoverServerPluginsAPI will return plugin discovered through all active contexts.targetflag for plugin lifecycle commands to uniquely identify the plugins incase of name conflicts.targetinformation for standalone plugins CR defined understandalone-pluginspackagek8stargeted plugins when there is name conflict between<none>targeted plugin andk8stargeted plugin.k8starget plugins is needed here as both the plugins can be treated as same pluginstargetinformationTODO:
Context-Typein ClientConfig API toTarget. (This will be done as part of follow-up PR)Which issue(s) this PR fixes
Related to #3714
Describe testing done for PR
Plugin Install Tests
List the plugins before installing any
Different Plugin Name conflict scenarios
Check
tanzu --helpto see theclustercommand is available under root tanzu cli command as well as underkubernetestarget withtanzu kubernetes --helpContinue installing plugin from tmc target:
Verify the help to see the
clustercommand is available undertmctargetList the plugins after installing few plugins.
Different Plugin Name conflict scenarios
Release note
Additional information
Special notes for your reviewer