Improve service catalog related stuff from PR 413 #680

surajnarwade · 2018-08-30T08:28:56Z

Since PR 413 is merged, there were few minor comments charlie mentioned later.
Those comments are addressed in this PR.

anmolbabu · 2018-08-30T08:58:11Z

@surajnarwade I would be glad if you could also resolve #678 as part of this PR

cmoulliard · 2018-08-30T09:01:44Z

cmd/catalog.go

@@ -34,7 +34,7 @@ var catalogListCmd = &cobra.Command{

 var catalogListComponentCmd = &cobra.Command{
 	Use:   "components",
-	Short: "List all available component types.",
+	Short: "List all components available.",
 	Long:  "List all available component types from OpenShift's Image Builder.",
 	Example: `  # Get the supported components
  odo catalog list components


Is the word components appropriate here as the goal is to list the builder images available. I propose/suggest then that you use this command -> List all builder's images available and odo catalog list builders

From user perspective, it is component, right ?

cmoulliard · 2018-08-30T09:03:55Z

cmd/service.go

@@ -34,7 +34,7 @@ var serviceCreateCmd = &cobra.Command{

 If service name is not provided, service type value will be used.

-A full list of service types that can be deployed is available using: 'odo service catalog'`,
+A full list of service types that can be deployed are available using: 'odo catalog list services'`,
 	Example: `  # Create new mysql-persistent service from service catalog.
  odo service create mysql-persistent


To be consistent, the syntax of this command should be

creation : odo catalog service create <name_of_the_service_class>

deletion : odo catalog service delete mysql-persistent

WDYT ?

odo catalog is supposed to only list and search images and services
while odo create or odo delete will deal with components and
odo service xxx will deal with services

cmoulliard · 2018-08-30T09:06:48Z

cmd/catalog.go

 	Example: `  # List all services
  odo catalog list services

- # Search for a supported services
+ # Search for a supported a service
  odo catalog search services mysql


Should we use the plural or singular form ? If the odo's command can do a search to retrieve all the services containing the word passed as parameter, then go ahead to use the plural services word otherwise that should be the singular -> odo catalog search service mysql

I agree with @cmoulliard maybe we should use just service instead.

cmoulliard · 2018-08-30T09:08:51Z

cmd/service.go

@@ -34,7 +34,7 @@ var serviceCreateCmd = &cobra.Command{

 If service name is not provided, service type value will be used.

-A full list of service types that can be deployed is available using: 'odo service catalog'`,
+A full list of service types that can be deployed are available using: 'odo catalog list services'`,


Do you also envision to list the plans, brokers, instances and bindings created ?

odo catalog list services|plans|brokers|bindings

I think,

odo catalog list services should show service name, plans & required parameters

odo service create service_name --plan plan_name -p parameters

odo link component_name --service service_name which will do the binding stuff

WDYT ?

a) Plan detail

You need a command to show the detail of a plan as we can do with svcat
We could then use by example : odo catalog plan <serviceName>/<planName>

b) Create service
I agree with you for the command to create the service excepted that you must also pass the class name

odo service create service_name --class class_name --plan plan_name -p parameters

Remark : As some parameters are json typed, then it will be very difficult to pass them using a one line command and -p, -p ... This is why, we have in our prototype, defined such parameters/info into a yaml config file

c) Mount the secret to component/

To do the binding, then you must also pass the name of the secret

odo link component_name --secret secret_name

component_name corresponds to the DeploymentConfig's name
secret_name : secret containing the parameters

Remark : There is a step that you don't convert which is "generating the secret with the parameters of the plan selected. Is it something that you will do automatically during the creation of the service ? If this is what you plan to do, then you must also pass the name of the secret ->

odo service create service_name --class class_name --plan plan_name --secret secret_name -p parameters

anmolbabu

Looks like you missed #413 (comment)

anmolbabu · 2018-08-30T09:03:49Z

cmd/catalog.go

@@ -34,7 +34,7 @@ var catalogListCmd = &cobra.Command{

 var catalogListComponentCmd = &cobra.Command{
 	Use:   "components",
-	Short: "List all available component types.",
+	Short: "List all components available.",


I seem to get confused between component and component type
My current understanding is that a component is an application built using a builder imagestream of type same as component type.. which makes me arrive at
component === app
component type === builder image stream of respective type..
It is possible that I am missing something
Please correct me if I am wrong...

App is your application which consist of multiple components

Components is one of the part of your app

component type is type of component, for example, java or python

anmolbabu · 2018-08-30T09:05:08Z

cmd/catalog.go

 	Example: `  # List all services
  odo catalog list services

- # Search for a supported services
+ # Search for a supported a service


Search for a supported service :)

anmolbabu · 2018-08-30T09:05:47Z

cmd/catalog.go

-	Short: "Lists all the services from service catalog",
-	Long:  "Lists all the services from service catalog",
+	Short: "Lists all services available",
+	Long:  "Lists all services available",


Can this be : List all available services ? @cdrage

Yup. That's correct

cdrage · 2018-08-30T13:47:23Z

May be good to change the title / name as well to not include my name haha. Maybe "Improve documentation for catalog" or something along those lines.

cdrage · 2018-08-31T15:58:00Z

cmd/catalog.go

-	Short: "Lists all the services from service catalog",
-	Long:  "Lists all the services from service catalog",
+	Short: "Lists all services available",
+	Long:  "Lists all services available",


Yup. That's correct

cdrage · 2018-08-31T15:58:40Z

cmd/catalog.go

 	Example: `  # List all services
  odo catalog list services

- # Search for a supported services
+ # Search for a supported a service
  odo catalog search services mysql


I agree with @cmoulliard maybe we should use just service instead.

cdrage · 2018-08-31T16:00:09Z

cmd/service.go

  odo service delete mysql-persistent
 	`,
 	Args: cobra.MaximumNArgs(1),
 	Run: func(cmd *cobra.Command, args []string) {
-		glog.V(4).Infof("service delete called")
+
+		glog.V(4).Infof("service delete called\n")


similar to my previous comment, can we merge these two debug / info messages?

cdrage · 2018-09-04T16:38:35Z

This LGTM. Need more reviews however before merging.

cdrage · 2018-09-06T16:37:03Z

@surajnarwade needs another rebase and LGTM.

mik-dass

This PR LGTM, merge once travis is green though

Since PR 413 is merged, there were few minor comments charlie mentioned later. Those comments are addressed in this PR.

surajnarwade requested review from cdrage, anmolbabu and mik-dass August 30, 2018 08:29

cmoulliard reviewed Aug 30, 2018

View reviewed changes

anmolbabu reviewed Aug 30, 2018

View reviewed changes

surajnarwade force-pushed the fix-svccatalog-413 branch from b568f64 to 948f367 Compare August 30, 2018 09:20

surajnarwade changed the title ~~Added charlie's comment from PR 413~~ Improve service catalog related stuff from PR 413 Aug 30, 2018

cdrage suggested changes Aug 31, 2018

View reviewed changes

surajnarwade force-pushed the fix-svccatalog-413 branch from 948f367 to 4ec8f4a Compare September 3, 2018 07:31

surajnarwade mentioned this pull request Sep 3, 2018

update example for service catalog search #685

Closed

cdrage approved these changes Sep 4, 2018

View reviewed changes

anmolbabu approved these changes Sep 5, 2018

View reviewed changes

surajnarwade force-pushed the fix-svccatalog-413 branch from 4ec8f4a to 6bbedb1 Compare September 7, 2018 11:00

mik-dass approved these changes Sep 7, 2018

View reviewed changes

Improved servicecatalog comment from PR 413

00b85a5

Since PR 413 is merged, there were few minor comments charlie mentioned later. Those comments are addressed in this PR.

surajnarwade force-pushed the fix-svccatalog-413 branch from 6bbedb1 to 00b85a5 Compare September 10, 2018 09:11

mik-dass merged commit 82d6be7 into redhat-developer:master Sep 10, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve service catalog related stuff from PR 413 #680

Improve service catalog related stuff from PR 413 #680

surajnarwade commented Aug 30, 2018

anmolbabu commented Aug 30, 2018

cmoulliard Aug 30, 2018

surajnarwade Aug 30, 2018

cmoulliard Aug 30, 2018

surajnarwade Aug 31, 2018

cmoulliard Aug 30, 2018

cdrage Aug 31, 2018

cmoulliard Aug 30, 2018

surajnarwade Aug 31, 2018

cmoulliard Aug 31, 2018 •

edited

anmolbabu left a comment

anmolbabu Aug 30, 2018

surajnarwade Sep 3, 2018

anmolbabu Aug 30, 2018

anmolbabu Aug 30, 2018

cdrage Aug 31, 2018

cdrage commented Aug 30, 2018

cdrage Aug 31, 2018

cdrage Aug 31, 2018

cdrage Aug 31, 2018

cdrage commented Sep 4, 2018

cdrage commented Sep 6, 2018

mik-dass left a comment

Improve service catalog related stuff from PR 413 #680

Improve service catalog related stuff from PR 413 #680

Conversation

surajnarwade commented Aug 30, 2018

anmolbabu commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cmoulliard Aug 31, 2018 • edited

Choose a reason for hiding this comment

anmolbabu left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrage commented Aug 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cdrage commented Sep 4, 2018

cdrage commented Sep 6, 2018

mik-dass left a comment

Choose a reason for hiding this comment

cmoulliard Aug 31, 2018 •

edited