Cleanup pluginmanager of unused feature flag code #484
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
marckhouzam
force-pushed
the
feat/cleanupPluginManager
branch
4 times, most recently
from
b3bfacf to
5e4fc78
Compare
This commit removes all the unused code around feature flags from the
`manager.go` file. Those changes are:
1. Remove `ValidatePlugin()` which was never used. There is a
separate copy of this function in the tanzu-plugin-runtime repository.
2. Remove `discoverPlugins()` and directly use `discoverSpecificPlugins()`
3. Remove `discoverServerPluginsBasedOnCurrentServer` which is no longer
used.
4. Include the logic of `discoverServerPluginsBasedOnAllCurrentContexts()`
directly into `DiscoverServerPlugins()` which had become a one-liner.
5. Remove the following functions that were no longer used:
- `AvailablePlugins()`
- `AvailablePluginsFromLocalSource()`
- `availablePlugins()`
- `combineDuplicatePlugins()`
- `getInstalledButNotDiscoveredStandalonePlugins()`
- `DiscoveredFromPlugininfo()`
- `availablePluginsFromStandaloneAndServerPlugins()`
- `pluginIndexForName()`
- `legacyPluginInstall()`
- `GetRecommendedVersionOfPlugin()`
Completely remove the feature flag constant:
FeatureDisableCentralRepositoryForTesting
* Update the unit tests for the plugin manager.
This commit improves our ability to test the pluginmanager.
It does this using two techniques:
1. Creating a test plugin inventory DB in the cache and requesting the
inventory code to always use the cache (without checking the digest).
To force the use of the cache the commit introduces a test variable
`TEST_TANZU_CLI_USE_DB_CACHE_ONLY`.
This approach allows to discover plugins and groups without needing
a real OCI registry.
2. Creating "fake" plugin binaries in the plugin binary cache. This
allows installation of plugins to find the binaries in the cache and
therefore never have to go to an OCI registry to download them.
With these two techniques, the unit tests can discovery and install
plugins, and can also do so using groups. The plugin sync can also be
tested with this.
Note: the unit tests would still benefit from some tests moving to this
approach of testing, but this will need to come in a future effort.
Signed-off-by: Marc Khouzam <[email protected]>
marckhouzam
force-pushed
the
feat/cleanupPluginManager
branch
from
5e4fc78 to
d3f7c96
Compare
anujc25
approved these changes
Sep 19, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
What this PR does / why we need it
This commit removes all the unused code around feature flags from the
manager.gofile.Code cleanup details:
ValidatePlugin()which was never used. There is a separate copy of this function in the tanzu-plugin-runtime repository.discoverPlugins()and directly usediscoverSpecificPlugins()discoverServerPluginsBasedOnCurrentServerwhich is no longer used.discoverServerPluginsBasedOnAllCurrentContexts()directly intoDiscoverServerPlugins()which had become a one-liner.-
AvailablePlugins()-
AvailablePluginsFromLocalSource()-
availablePlugins()-
combineDuplicatePlugins()-
getInstalledButNotDiscoveredStandalonePlugins()-
DiscoveredFromPlugininfo()-
availablePluginsFromStandaloneAndServerPlugins()-
pluginIndexForName()-
legacyPluginInstall()-
GetRecommendedVersionOfPlugin()Completely remove the feature flag constant:
FeatureDisableCentralRepositoryForTestingUpdate the unit tests for the plugin manager.
This commit also improves our ability to test the pluginmanager.
It does this using two techniques:
Creating a test plugin inventory DB in the cache and requesting the inventory code to always use the cache (without checking the digest). To force the use of the cache the commit Introduces a test variable
TANZU_CLI_USE_DB_CACHE_ONLY. This approach allows to discover plugins and groups without needing a real OCI registry.Creating "fake" plugin binaries in the plugin binary cache. This allows installation of plugins to find the binaries in the cache and therefore never have to go to an OCI registry to download them.
With these two techniques, the unit tests can discovery and install plugins, and can also do so using groups. The plugin sync can also be tested with this.
Which issue(s) this PR fixes
Part of #477
Describe testing done for PR
Release note
Additional information
Special notes for your reviewer
A new test variable is introduced:
TEST_TANZU_CLI_USE_DB_CACHE_ONLY. When set to1(or any other enabled boolean value) it forces theDBBackedOCIDiscoveryto only use the cache. Using this variable, tests now setup an sqliteDB file in the cache, and the tests will use it without having to access a registry. Note that we have a
DiscoveryOptionsWithUseLocalCacheOnly()which could have been used for some tests, specifically the tests discovering plugins. However, in other tests such as the ones installing plugins, using only the cache is not normally supported so this variable is a way to short-circuit that.The test coverage from the
manager.gohas gone from 73.6% to 76.5%. This may not seem like a big deal, but after the cleanup the test coverage had actually dropped to 68.6%, so getting back to over 75% required some effort.More unit tests would benefit from moving to the new approach of testing instead of relying on local plugin installation. However such an improvement will need to be done in a future effort.