[client provision via catalog] create client using service catalog #128

witmicko · 2018-06-12T16:31:57Z

Describe what this PR does and why we need it:

provision via catalog
unit test

Does this PR depend on another PR (Use this to track when PRs should be merged)
aerogearcatalog/android-app-apb#11

Which issue this PR fixes (This will close that issue when PR gets merged)
https://issues.jboss.org/browse/AEROGEAR-3023

Verification steps*:

pull this changes and build binary
run different variants of mobile create client ...
ensure that each mobile client has corresponding service instance
MAR view annotated service_instance_id should match service instance id

*needs:
aerogearcatalog/android-app-apb#11
aerogearcatalog/cordova-app-apb#9
aerogearcatalog/ios-app-apb#10
aerogearcatalog/xamarin-app-apb#5

philipgough · 2018-06-13T10:01:34Z

pkg/cmd/clients.go

 	"github.com/spf13/cobra"
 	metav1 "k8s.io/apimachinery/pkg/apis/meta/v1"
 	"k8s.io/apimachinery/pkg/types"
+	"encoding/json"


I think its good practice to list imports from stdlib first followed by external dependencies but this is just a nit.

philipgough · 2018-06-13T10:03:08Z

pkg/cmd/clients.go

-  kubectl plugin mobile get clients
-  oc plugin mobile get clients`,
+  					kubectl plugin mobile get clients
+  					oc plugin mobile get clients`,


This indentation is preferred I would say but I'd suggest change others to this format for consistency since we are touching the file here anyway

philipgough · 2018-06-13T10:22:40Z

Gopkg.lock

-  name = "github.com/satori/go.uuid"
-  packages = ["."]
-  revision = "879c5887cd475cd7864858769793b2ceb0d44feb"
-  version = "v1.1.0"


I think you can remove the following from Gopkg.toml now

[[constraint]] name = "github.com/satori/go.uuid" version = "1.1.0"

philipgough · 2018-06-13T10:29:27Z

pkg/cmd/clients.go

-				Spec: v1alpha1.MobileClientSpec{Name: name, ApiKey: appKey, ClientType: clientType, AppIdentifier: appIdentifier},
+
+			clientId := name + "-" + clientType
+			client, _ := cc.mobileClient.MobileV1alpha1().MobileClients(namespace).Get(clientId, metav1.GetOptions{})


Not sure about not handling the error here, think some basic handling required at least since this calls the server

we should handle the error

philipgough · 2018-06-13T10:31:22Z

pkg/cmd/clients.go

 				break
 			case "xamarin":
-				app.Annotations["icon"] = "font-icon icon-xamarin"
+				apbName = "xamarin"


Just wondering about the formatting here why android-app while the other types just inherit the client type, should they be ${clientType}-app also?

philipgough · 2018-06-13T10:36:34Z

pkg/cmd/clients.go

 			if err != nil {
-				return errors.Wrap(err, "failed to create mobile client")
+				return errors.WithStack(err)


Might be better to wrap this and the above error like with the previous. I feel we are exposing more than needed to the caller here, wdyt?

prob depends what we are logging also. I think the stack trace only shows if we use "%+v". As a general point though I prefer having the context added to the err as the err can be obscure sometimes

philipgough · 2018-06-13T10:37:08Z

pkg/cmd/clients.go

+			extMeta := clusterServiceClass.Spec.ExternalMetadata.Raw
+			var extServiceClass ExternalServiceMetaData
+			if err := json.Unmarshal(extMeta, &extServiceClass); err != nil {
+				return errors.WithStack(err)


philipgough · 2018-06-13T10:37:16Z

pkg/cmd/clients.go

+			instParams := &ServiceParams{}
+
+			if err := json.Unmarshal(clusterServicePlan.Spec.ServiceInstanceCreateParameterSchema.Raw, instParams); err != nil {
+				return errors.WithStack(err)


philipgough · 2018-06-13T10:40:55Z

pkg/cmd/clients.go

+				return errors.WithStack(err)
+			}
+
+			si := v1beta1.ServiceInstance{


Do you think this might be better as a call to a constructor. I see similar creation in same pkg in at least one place https://github.com/aerogear/mobile-cli/blob/master/pkg/cmd/services.go#L231 have not checked for more but do you think it makes sense to move this out?

I havent looked too deeply into so ignore if it complicates things or provides no warranted dryness

philipgough · 2018-06-13T11:00:12Z

pkg/cmd/clients.go

+			pSecret.Data["parameters"] = secretData
+			if _, err := cc.k8Client.CoreV1().Secrets(namespace).Create(&pSecret); err != nil {
+				return errors.WithStack(err)
+			}


I'd probably arrange the above like

parameters := map[string]string{ "appName": name, "appIdentifier": appIdentifier, } secretData, err := json.Marshal(parameters) if err != nil { return errors.WithStack(err) } pSecret := v1.Secret{ ObjectMeta: metav1.ObjectMeta{ Name: clientId + "-apb-" + "params", }, Data: map[string][]byte{ "parameters": secretData, }, }

we could fail early this way but its personal preferece, existing code is fine except for the above comments in regards to error wrapping

philipgough · 2018-06-13T11:12:51Z

pkg/cmd/clients.go

+								w.Stop()
+								return errors.New("Failed to provision " + extServiceClass.ServiceName + ". " + c.Message)
+							}
+						}


@witmicko I' unsure about this behaviour because it doesn't seem consistent with other cli behaviour from oc or kubectl. IMO the request should just end with error or ok if the server was happy to accept and the user can initiate a watch later if they wish but there may be reasons for the above, dont have enough context to say if this should be removed or remain

We do this for other services and allow a --no-wait flag to be passed. So I think this is ok for now. It may be something we want to change where no-wait is the default and you can pass an option to wait. But not something for this issue I think

We do this for other services

@witmicko, @maleck13 Cool, fine with this then if its consistent with others but I was coming from the angle where we would go something like oc get pods -w to initiate a watch after a request

philipgough

Some comments and requested changes. Not sure about the vendored changes think there might be some issues there

maleck13 · 2018-06-13T11:28:57Z

pkg/cmd/clients.go

+			if _, err := cc.scClient.ServicecatalogV1beta1().ServiceInstances(namespace).Create(&si); err != nil {
+				return errors.WithStack(err)
+			}
+			fmt.Println("creating service")


I don't think we need this

maleck13 · 2018-06-13T11:30:45Z

pkg/cmd/clients.go

+				select {
+				case msg, ok := <-w.ResultChan():
+					if !ok {
+						fmt.Println("Timedout waiting. It seems to be taking a long time for the service to provision. Your service may still be provisioning.")


I think we should make this error call less about a service (I know we are provisioning a service instance) and say timedout creating the mobile client

maleck13

some minor changes but generally looks pretty close. Not sure why we ended up with vendor changes?

philipgough

@witmicko Build is failing on fmt, run go fmt -w on touched files. Unit tests are passing, reviewing other changes now also

philipgough · 2018-06-14T08:45:33Z

pkg/cmd/clients.go

 	cc.Out.AddRenderer("create"+cmd.Name(), "table", func(writer io.Writer, mobileClient interface{}) error {
-		mClient := mobileClient.(*v1alpha1.MobileClient)
+		mClient, _ := cc.mobileClient.MobileV1alpha1().MobileClients(namespace).Get(clientId, metav1.GetOptions{})


@witmicko We should handle the error here also

philipgough · 2018-06-14T08:56:27Z

pkg/cmd/clients.go

-			outType := outputType(cmd.Flags())
-			if err := cc.Out.Render("create"+cmd.Name(), outType, createdClient); err != nil {
-				return errors.Wrap(err, fmt.Sprintf(output.FailedToOutPutInFormat, "mobile client", outType))
+			fmt.Println("Creating Mobile Client")


Should this be here?

it takes a while to provision, so I followed the pattern from creating service instance and log it out to let the user know what is happening

philipgough · 2018-06-14T08:57:30Z

pkg/cmd/clients_test.go

+			//		t.Fatal("failed to unmarshal mobile client")
+			//	}
+			//	tc.Validate(t, mobileClient)
+			//}


I'm assuming this should be removed?

philipgough · 2018-06-14T08:58:11Z

pkg/cmd/services.go

+				},
+			},
+		},
+	}


philipgough · 2018-06-14T08:59:46Z

pkg/cmd/clients_test.go

+	"path/filepath"
+	"k8s.io/client-go/tools/clientcmd"
+	"os"
+


Another nit on the ordering of imports here

philipgough · 2018-06-14T09:02:57Z

pkg/cmd/clients_test.go

+	}
+	k8Client, mobileClient, scClient := NewClientsOrDie(config)
+
+	fmt.Println(k8Client, mobileClient, scClient)


probably want this removed

philipgough · 2018-06-14T09:03:52Z

pkg/cmd/clients_test.go

 func TestMobileClientsCmd_TestCreateClient(t *testing.T) {
+	var kubeconfig *string


@maleck13 @witmicko I'm trying to wrap my head around these tests but in this particular test I think we are creating a real client and I don't see us doing that anywhere else, any reason why?

sorry this is irrelevant, forgot to delete it, had it in as a way into debugger

witmicko requested a review from maleck13 June 12, 2018 16:32

philipgough reviewed Jun 13, 2018

View reviewed changes

maleck13 reviewed Jun 13, 2018

View reviewed changes

maleck13 suggested changes Jun 13, 2018

View reviewed changes

philipgough suggested changes Jun 14, 2018

View reviewed changes

philipgough reviewed Jun 14, 2018

View reviewed changes

pkg/cmd/services.go

},

},

},

}

Copy link

Contributor

philipgough Jun 14, 2018

Choose a reason for hiding this comment

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

+1

philipgough reviewed Jun 14, 2018

View reviewed changes

create client using service catalog

3f0d6b0

witmicko force-pushed the client-create-catalog branch from e1c7e75 to 3f0d6b0 Compare June 14, 2018 13:16

witmicko added 2 commits June 14, 2018 16:34

lower case secret

f7ae990

secret fix

c2048f9

philipgough force-pushed the client-create-catalog branch 2 times, most recently from fddfdd4 to 6a4983e Compare June 15, 2018 10:47

Small code change for secret fix and removed unused vendored pkg

302e2ba

philipgough force-pushed the client-create-catalog branch from 6a4983e to 302e2ba Compare June 15, 2018 12:19

witmicko added 2 commits June 18, 2018 11:19

fix integration tests

1025fd4

fix integration tests

6d2d974

witmicko merged commit b5867b6 into aerogear-attic:master Jun 18, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[client provision via catalog] create client using service catalog #128

[client provision via catalog] create client using service catalog #128

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

		func TestMobileClientsCmd_TestCreateClient(t *testing.T) {
		var kubeconfig *string

[client provision via catalog] create client using service catalog #128

[client provision via catalog] create client using service catalog #128

Uh oh!

Conversation

Uh oh!

Verification steps*:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment The reason will be displayed to describe this comment to others. Learn more. Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Hide comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.