Skip to content
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

Avoid uselessly trying to migrate plugins #832

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 8 additions & 12 deletions pkg/catalog/cleanup.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,6 @@
package catalog

import (
configlib "github.com/vmware-tanzu/tanzu-plugin-runtime/config"
configtypes "github.com/vmware-tanzu/tanzu-plugin-runtime/config/types"
)

Expand Down Expand Up @@ -47,27 +46,24 @@ func DeleteIncorrectPluginEntriesFromCatalog() {
}

// MigrateContextPluginsAsStandaloneIfNeeded updates the catalog cache to move all the
// context-scoped plugins associated with the active context as standalone plugins
// context-scoped plugins as standalone plugins
// and removes the context-scoped plugin mapping from the catalog cache.
// This is to ensure backwards compatibility when user migrates from pre v1.3 version of
// the CLI, the context-scoped plugins are still gets shown as installed
// This is to ensure backwards compatibility when the user migrates from pre v1.3 version of
// the CLI, the context-scoped plugins are still shown as installed
func MigrateContextPluginsAsStandaloneIfNeeded() {
activeContexts, err := configlib.GetAllActiveContextsList()
if err != nil || len(activeContexts) == 0 {
return
}

c, lockedFile, err := getCatalogCache(true)
if err != nil {
return
}
defer lockedFile.Close()

for _, ac := range activeContexts {
for pluginKey, installPath := range c.ServerPlugins[ac] {
for _, association := range c.ServerPlugins {
for pluginKey, installPath := range association {
c.StandAlonePlugins.Add(pluginKey, installPath)
}
delete(c.ServerPlugins, ac)
}
// Delete all entries by reassigning to a new empty map
c.ServerPlugins = make(map[string]PluginAssociation)

_ = saveCatalogCache(c, lockedFile)
Comment on lines 53 to 68
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What I understand is with this change now we are converting all the ServerPlugins as StandalonePlugins instead of just doing that for the ActiveContexts.
Is this change intentional?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Correct. See point 3 in the PR description:

with this PR, since we only run the plugin migration once, we must migrate the context-scoped plugins for all contexts immediately. This is a slight change in behaviour because context-scoped plugins for an inactive (and possibly old context) will now appear as standalone plugins right away for the new CLI. Since this new behaviour is similar to point 2 above, I feel it is acceptable

Originally, I started by keeping the original solution of checking this migration at every command, but not blindly updating the catalog if there was nothing to migrate. However this still has a cost compared to running the migration only once, since we don't have to check the catalog for every command, just in case.

But I wanted your opinion if there may be side-effects I missed.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

instead of just doing that for the ActiveContexts

To be very clear, just in case, with the previous approach, when the active context would change, we would migrate the plugins for the new active context. So, assuming every context eventually became active, then all ServerPlugins would eventually be migrated as StandalonePlugins.

With this PR we do it all at once and be done with it.

Do your recall if "only doing it for the active contexts" was a kind of optimization? Maybe because it was run for every command?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do your recall if "only doing it for the active contexts" was a kind of optimization? Maybe because it was run for every command?

It was not for optimization but to reliably do the migration as different contexts can recommend different versions of a same plugin. So when we migrate the context-scoped plugins for all available contexts at once we can loose the data as the last migrated plugin version will win. I think we need to be careful and think this through.

}
Loading