This repository was archived by the owner on Oct 10, 2023. It is now read-only.
Implement plugin discovery source CRUD command for context aware CLI#950
Merged
anujc25 merged 2 commits intovmware-tanzu:mainfrom Nov 9, 2021
Merged
Conversation
anujc25
changed the title
anujc25
force-pushed
the
plugin-discovery-commands
branch
4 times, most recently
from
cca655a to
59eeab4
Compare
anujc25
added
area/cli
area/plugin
kind/feature
anujc25
force-pushed
the
plugin-discovery-commands
branch
from
59eeab4 to
1bda275
Compare
Contributor
|
Sorry if it's too late, but would Also see my comment: #947 (comment) |
zjs
approved these changes
Oct 27, 2021
Member
zjs
left a comment
zjs
left a comment
There was a problem hiding this comment.
Looks good. Left a couple of nit-picky comments.
(Note that this was a pretty cursory review as I ran out of time. If there's anywhere you'd like me to take a more careful look at, just let me know!)
anujc25
changed the title
anujc25
force-pushed
the
plugin-discovery-commands
branch
from
1bda275 to
186f284
Compare
Contributor
Author
As discussed in #947, I have updated the noun to |
anujc25
force-pushed
the
plugin-discovery-commands
branch
from
186f284 to
fd31bd0
Compare
imikushin
suggested changes
Nov 5, 2021
Contributor
imikushin
left a comment
imikushin
left a comment
There was a problem hiding this comment.
A few comments to make code simpler and more testable.
Also, please add unit tests for all new functionality.
Comment on lines
+172
to
+156
| discoverySources := cfg.ClientOptions.CLI.DiscoverySources | ||
|
|
||
| newDiscoverySources := []configv1alpha1.PluginDiscovery{} | ||
| found := false | ||
| for _, ds := range discoverySources { | ||
| if checkDiscoveryName(ds, discoveryName) { | ||
| found = true | ||
| ds = configv1alpha1.PluginDiscovery{} | ||
| switch strings.ToLower(discoverySourceType) { | ||
| case common.DiscoveryTypeLocal: | ||
| ds.Local = createLocalDiscoverySource(discoveryName, uri) | ||
| case common.DiscoveryTypeOCI: | ||
| ds.OCI = createOCIDiscoverySource(discoveryName, uri) | ||
| default: | ||
| return errors.Errorf("discovery source type '%s' is not yet supported", discoverySourceType) | ||
| } | ||
| } | ||
| newDiscoverySources = append(newDiscoverySources, ds) | ||
| } | ||
| if !found { | ||
| return discoveryNoExistError | ||
| } | ||
|
|
||
| cfg.ClientOptions.CLI.DiscoverySources = newDiscoverySources |
Contributor
There was a problem hiding this comment.
The whole block should be a separate function (that is easily unit testable):
newDiscoverySources, err := updateDiscoverySources(cfg.ClientOptions.CLI.DiscoverySources, discoveryName, discoverySourceType, uri)
if err != nil {
return err
}
cfg.ClientOptions.CLI.DiscoverySources = newDiscoverySources
Contributor
There was a problem hiding this comment.
This one hasn't been addressed :(
Please refactor consistent with deleteDiscoverySource() and addDiscoverySource()
Contributor
Author
There was a problem hiding this comment.
done. Refactored and added unit tests
anujc25
force-pushed
the
plugin-discovery-commands
branch
5 times, most recently
from
fac141c to
b2fe37f
Compare
vuil
approved these changes
Nov 8, 2021
Contributor
vuil
left a comment
vuil
left a comment
There was a problem hiding this comment.
Lgtm. Just a nit on the doc.
imikushin
reviewed
Nov 8, 2021
imikushin
reviewed
Nov 9, 2021
Contributor
imikushin
left a comment
imikushin
left a comment
There was a problem hiding this comment.
A couple comments still un-addressed. Otherwise looks good!
anujc25
force-pushed
the
plugin-discovery-commands
branch
from
b2fe37f to
444ae70
Compare
added 2 commits
November 8, 2021 17:26
- Implements `tanzu plugin source add/list/update/delete` commands - Currently, supported discovery types are oci and local - Add newly added noun, flag to cli-wordlist.yml file Signed-off-by: Anuj Chaudhari <anujc@vmware.com>
anujc25
force-pushed
the
plugin-discovery-commands
branch
from
444ae70 to
87abe10
Compare
imikushin
approved these changes
Nov 9, 2021
yharish991
pushed a commit
to yharish991/tanzu-framework
that referenced
this pull request
Nov 9, 2021
…mware-tanzu#950) * Implement plugin discovery source CRUD command - Implements `tanzu plugin source add/list/update/delete` commands - Currently, supported discovery types are oci and local - Add newly added noun, flag to cli-wordlist.yml file * Update cobra docs for the commands
This file contains hidden or 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
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
6 participants
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
tanzu plugin source add/list/update/deletecommandsWhich issue(s) this PR fixes
Fixes #947
Describe testing done for PR
Release note
PR Checklist
Additional information
Special notes for your reviewer