-
Notifications
You must be signed in to change notification settings - Fork 381
Conversation
39a3ec1
to
e2db2fc
Compare
3686695
to
7f9da1c
Compare
limitations under the License. | ||
*/ | ||
|
||
package extra |
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.
What do you think about putting this under cmd/svcat/plan
which has other verbs related to displaying and interacting with plans?
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.
so, I considered about where to put this - it's only sort of related to only classes or only plans. I figured we might as well have a place for commands that aren't strictly related to one of the types, and I didn't want to name it commands
because we already have a command
dir in there. I was planning on moving the other commands like completion and install here at some point. I'm open to suggestions for a better name.
cmd/svcat/extra/marketplace_cmd.go
Outdated
} | ||
plans := make([][]servicecatalog.Plan, len(classes)) | ||
for i, class := range classes { | ||
classPlans, err := c.App.RetrievePlans(class.GetName(), opts) |
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.
This will result in a lot of requests. Let's change this to do a single call to c.App.RetrievePlans
passing in ""
for the class. Then iterate over the results to populate your map of classes to plans.
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.
true facts. will fix this.
Everything looks good, but I'd really like to see the # of API calls reduced before we merge this. |
7f9da1c
to
1d5e404
Compare
@carolynvs fixed |
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.
/approve
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: carolynvs The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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
This PR is a
What this PR does / why we need it:
This PR implements
svcat marketplace
, a command for users to browse the catalog of services available to them without having to manually page through the service-catalog abstractions.Which issue(s) this PR fixes
Fixes #2033
Please leave this checklist in the PR comment so that maintainers can ensure a good PR.
Merge Checklist:
breaking the chart release and existing clients who provide a
flag that will get an error when they try to update