move service deployment related server side logic from cli #371

ilgooz · 2018-08-16T21:38:43Z

@antho1404 can you please update the comments on api/core/api.proto to match with the changes?

about the update DeployService API

I have merged Git url and uploading service context options to same gRPC DeployService handler for deploying services.

What do you prefer? Current one or split this as DeployService (uploading service context) and create new DeployServiceFromURL handler?
One small advantage of splitting them might be to not requiring to deal with gRPC stream for simple url deployments.

about the new mesg package

please give feedbacks about new mesg package. goal of mesg package is to serve core's all features with a high level API. gRPC server handlers will use methods from mesg package, instead of directly accessing to internal packages like service, execution, etc. we shouldn't keep any business logic within api handlers and should move them under mesg package.
there are only two methods in the mesg package to deploy services for now. i'm thinking about adding more methods to cover all features used by gRPC server handlers.
i'm planning to move mesg/deploy_deployer.go to service package in future, while refactoring service to make it newable. APIs will be like: Manager.CreateService(), Manager.CreateServiceFromURL()

* update api/core/api.proto for the new version of DeployService API. * run latest version of protoc & protoc-gen-go on api/core and api/service proto files. * add Id field to service proto. * add ca-certificates to Dockerfile. * import path of spinner changed to github.com/mesg-foundation/spinner. * DeployService API updated to receive a Git URL or data chunks of service.tar.gz to deploy a service. * service deployment related server side logic removed from cmd/service and cmd/service updated to meet with new DeployService API. * some of the deploy status messages printed to stdout removed/changed. * update tests of api/core to meet with updated DeployService API. * a sample unit test added to api/core at deploy_test.go. * Server type of api/core made newable and MESGOption() added. * api/service tests updated to workaround different service.Hash() computations because of newly introduced Id field on Service type. (this will be reverted after Service type created by hand in service package.) * database/services updated to meet with changes on its Save() method and new Id field on Service type. * new package mesg added. mesg is responsible to expose all features of core with a nice API. api handlers will use methods from mesg package to interact with core's features.

krhubert · 2018-08-17T07:25:47Z

api/core/api.proto

-  rpc DeployService (DeployServiceRequest) returns (DeployServiceReply) {}
-
+  rpc DeployService (stream DeployServiceRequest) returns (stream DeployServiceReply) {}
+  


leading whitespaces

krhubert · 2018-08-17T07:35:58Z

api/core/deploy.go

+
+func (r *deployServiceStreamReader) GetURL() (url string, err error) {
+	message, err := r.stream.Recv()
+	r.data = message.GetChunk()


Check if message == nil (or error != nil then return). Someone editing Recv method in the future doesn't need to be aware of the fact, that Recv must always return not nil message. In Go it is rather common that func/method return err with all others returned values set to zero.

krhubert · 2018-08-17T07:53:14Z

api/server.go

@@ -63,8 +64,18 @@ func (s *Server) Stop() {

 // register all server
 func (s *Server) register() {


Why register call fatal instead of returning error up the stack? It's invoked in Serve method which returns err, maybe let's change it to returning an error and keep program flow in as little places as possible.

krhubert · 2018-08-17T07:55:38Z

cmd/service/deploy.go

+			sp.Stop()
+			fmt.Println(aurora.Red(validationError))
+			fmt.Println("Run the command 'service validate' for more details")
+			os.Exit(0)


Return non-zero code on Exit or even make a chan for passing an error.

we need better error handling in cmd package

krhubert · 2018-08-17T07:57:42Z

dev-core

-./scripts/build-test-core.sh
-./dev-cli stop
-./dev-cli start
+# chain these to stop execution on failure


Instead of chaining all commands use the -e flag in #!/bin/bash

krhubert · 2018-08-17T07:58:29Z

docs/api/core.md

@@ -747,7 +750,9 @@ The reply's data of `DeployService` API.

 | Field | Type | Description |
 | ----- | ---- | ----------- |
-| serviceID | [string](#string) | The generated identifier of the deployed service. Use this ID with other APIs. |
+| status | [string](#string) | status will be sent after each status change. |


s/each status change/each change/ ?

i guess we can keep this one as it is

krhubert · 2018-08-17T08:00:03Z

mesg/deploy_deployer.go

+	return d.deploy(path)
+}
+
+func (d *serviceDeployer) gitClone(repoURL string, path string) error {


I think gitClone should be moved to some utils pkg (maybe xgit?) but it's not part of service Deploeyr

missing docs

docs are done

krhubert · 2018-08-17T08:00:48Z

mesg/deploy_deployer.go

+	return err
+}
+
+func (d *serviceDeployer) FromGzippedTar(r io.Reader) (*service.Service, *importer.ValidationError, error) {


missing docs + few other methods.

krhubert · 2018-08-17T08:06:04Z

mesg/deploy_deployer.go

+	return d.deploy(path)
+}
+
+func (d *serviceDeployer) deploy(path string) (*service.Service, *importer.ValidationError, error) {


I know golint will not highlight this, but even for unexported methods/funcs it's nice to have some docs (the docs are for mainly us, not to satisfy linters :))

NicolasMahe · 2018-08-17T08:24:03Z

api/core/api.proto

+  oneof value {
+    // Git repo url of service. If url provided, stream will be closed after 
+    // first receive.
+    string url = 1;


The number ( '1' ) should not be the same even if your replace the previously declared variable with number '1'.
The reason is, gRPC manages way better the update of API when the number are not the same.
So we should never 'reused' previously used number.

https://developers.google.com/protocol-buffers/docs/proto3

NicolasMahe · 2018-08-17T08:24:17Z

api/core/api.proto

-  string serviceID = 1; // The generated identifier of the deployed service. Use this ID with other APIs.
+  oneof value {
+    // status will be sent after each status change.
+    string status = 1;


Same comment about the number.

krhubert · 2018-08-17T10:56:42Z

mesg/mesg.go

+		option(m)
+	}
+	var err error
+	if m.container == nil {


Avoid nested if's

if m.container != nil { return m, nil } var err error m.container, err = container.New() if err ....

thanks, in this case i kinda don't like making an early return here because we might add some more lines under these code that not related with initializing a container.

* respect to protobuf field numbers * add more docs * return with an err in register() of api/ Server * cmd/service don't os.Exit() in goroutine * use -e on dev-core script * update proto docs

* tests for this will be added within #371

…elated with #380, #371

…yment

ilgooz · 2018-08-23T16:26:42Z

needs review

NicolasMahe · 2018-08-24T05:22:51Z

cmd/service/deploy.go

+		return err
+	}
+
+	buf := make([]byte, 1024)


1ko seems a bit small.

Spoiler alert: it's 16KiB - 64KiB, according to oral tradition.
grpc/grpc.github.io#371

please check this out https://ops.tips/blog/sending-files-via-grpc/

It seems like at 1024bytes (1KB) we have a decent throughput.

I was expecting something like 32KB: (1 << 15) to be the most optimal but it looks like it’s one of the worst scenarios regarding variance.

NicolasMahe · 2018-08-24T05:26:04Z

cmd/service/deploy.go

+}
+
+func readDeployReply(stream core.Core_DeployServiceClient, deployment chan deploymentResult) {
+	sp := spinner.New(utils.SpinnerCharset, utils.SpinnerDuration)


What don't you use utils.StartSpinner or utils.ShowSpinnerForFunc?

If the existing helper function are not useful for what you are doing, I suggest to create a helper function in order to reuse it if it make sense.

I had to create it manually in the first for some reason... I'll see if i can use current util for this

make dev-core script to exit on fail

21dcdb1

ilgooz added this to the v1.2.0 milestone Aug 16, 2018

ilgooz added 2 commits August 17, 2018 01:16

update vendor

181cd6b

ilgooz force-pushed the feature/simplify-cli-deploy branch from 7fbae9a to 9bcd005 Compare August 16, 2018 22:20

ilgooz added 2 commits August 17, 2018 01:26

add more comments & minor improvements

7bf910f

update vendor

d9e9603

ilgooz force-pushed the feature/simplify-cli-deploy branch from 9bcd005 to d9e9603 Compare August 16, 2018 22:27

add mesg/service-test-invalid/

db10cb9

ilgooz force-pushed the feature/simplify-cli-deploy branch from a9c4818 to db10cb9 Compare August 17, 2018 05:39

ilgooz added 3 commits August 17, 2018 08:46

merge dev

535decf

fix: add missing return

b5761ab

improve comments

a18ab8d

ilgooz requested a review from a team August 17, 2018 07:52

krhubert reviewed Aug 17, 2018

View reviewed changes

NicolasMahe requested changes Aug 17, 2018

View reviewed changes

ilgooz mentioned this pull request Aug 17, 2018

refactor utils #373

Closed

krhubert reviewed Aug 17, 2018

View reviewed changes

minor fixes and improvements

b22027a

* respect to protobuf field numbers * add more docs * return with an err in register() of api/ Server * cmd/service don't os.Exit() in goroutine * use -e on dev-core script * update proto docs

ilgooz force-pushed the feature/simplify-cli-deploy branch from 3261b15 to b22027a Compare August 17, 2018 15:08

ilgooz added 2 commits August 20, 2018 08:23

Merge branch 'dev' into feature/simplify-cli-deploy

b17faa6

Merge branch 'dev' into feature/simplify-cli-deploy

0886687

ilgooz added a commit that referenced this pull request Aug 21, 2018

return err if dependency.Start() fails in service.Start()

29f0017

* tests for this will be added within #371

ilgooz mentioned this pull request Aug 21, 2018

return err if dependency.Start() fails in service.Start() #380

Merged

NicolasMahe pushed a commit that referenced this pull request Aug 22, 2018

[fix] return err if dependency.Start() fails in service.Start() (#380)

3ecfa9a

* tests for this will be added within #371

Merge branch 'dev' into feature/simplify-cli-deploy

e67a51e

ilgooz added a commit that referenced this pull request Aug 22, 2018

TestDependenciesListensFromSamePort test added for service package, r…

b9c98d4

…elated with #380, #371

ilgooz added 2 commits August 23, 2018 12:27

update codeclimate.yml for return statements count

facc4ed

Dockerfile: improve apt-get

33a1d78

ilgooz force-pushed the feature/simplify-cli-deploy branch from 65bfc6d to 33a1d78 Compare August 23, 2018 09:44

ilgooz added 2 commits August 23, 2018 15:19

mesg: update docs

3ad1587

cmd/service: [fix] return true by default for isValid of deployService()

ce8cd50

ilgooz force-pushed the feature/simplify-cli-deploy branch 3 times, most recently from 1f83701 to 05d059d Compare August 23, 2018 13:48

reapply all the removed status messages produced during service deplo…

d9234fd

…yment

ilgooz force-pushed the feature/simplify-cli-deploy branch from 05d059d to d9234fd Compare August 23, 2018 14:06

Generate proto with previous version

b119262

antho1404 previously approved these changes Aug 24, 2018

View reviewed changes

Merge branch 'dev' into feature/simplify-cli-deploy

43e8fa5

NicolasMahe reviewed Aug 24, 2018

View reviewed changes

ilgooz dismissed antho1404’s stale review via 2bfb190 August 24, 2018 05:54

ilgooz force-pushed the feature/simplify-cli-deploy branch 4 times, most recently from 71bed60 to dd3f37b Compare August 24, 2018 06:08

cmd: use spinner util & fix err result on readDeployReply()

1b11a0e

ilgooz force-pushed the feature/simplify-cli-deploy branch from dd3f37b to 1b11a0e Compare August 24, 2018 06:09

fix dev-core script and integration test in api/core

d2e50c8

antho1404 approved these changes Aug 24, 2018

View reviewed changes

NicolasMahe approved these changes Aug 24, 2018

View reviewed changes

NicolasMahe merged commit 0d43e34 into dev Aug 24, 2018

NicolasMahe deleted the feature/simplify-cli-deploy branch August 24, 2018 08:27

ilgooz restored the feature/simplify-cli-deploy branch August 24, 2018 14:22

ilgooz deleted the feature/simplify-cli-deploy branch August 24, 2018 14:22

		rpc DeployService (DeployServiceRequest) returns (DeployServiceReply) {}

		rpc DeployService (stream DeployServiceRequest) returns (stream DeployServiceReply) {}

		@@ -63,8 +64,18 @@ func (s *Server) Stop() {

		// register all server
		func (s *Server) register() {

move service deployment related server side logic from cli #371

move service deployment related server side logic from cli #371

Conversation

ilgooz commented Aug 16, 2018 • edited Loading

about the update DeployService API

about the new mesg package

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

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

ilgooz Aug 17, 2018 • edited Loading

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

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

ilgooz commented Aug 23, 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

ilgooz commented Aug 16, 2018 •

edited

Loading

ilgooz Aug 17, 2018 •

edited

Loading