Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Split proto service #850

Merged
merged 19 commits into from
Apr 7, 2019
Merged

Split proto service #850

merged 19 commits into from
Apr 7, 2019

Conversation

antho1404
Copy link
Member

A first step to the discussion yesterday about having a proto definition for the service that we use everywhere and that is totally independent of the local environment.

Here I splitted the service into it's own proto definition and update the apis:

  • ListServices
  • GetService

They return now a ServiceDetail that contains 2 data:

  • Status
  • Service

I'm not really fan of the naming for the api so feel free to propose something better

@antho1404 antho1404 requested review from krhubert, ilgooz and NicolasMahe and removed request for krhubert, ilgooz and NicolasMahe April 3, 2019 04:55
Copy link
Contributor

@ilgooz ilgooz left a comment

Choose a reason for hiding this comment

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

Really nice, But I'd like to change naming.

Maybe we can create a new dir at protobuf/definition and have service.proto and workflow.proto(in future) there with Service and Workflow type names. Because this way they'll feel generic by not having the coreapi namespace. This is good specially when we create some systemservices and they can use this proto definitions under definition namespace.

And we can have definition.Service inside coreapi.Service this way not coreapi.ServiceDetail. Which is a better name in my thinking.

krhubert
krhubert previously approved these changes Apr 3, 2019
@antho1404
Copy link
Member Author

antho1404 commented Apr 3, 2019

Thanks for the feedback, I agree naming was bad. I was able to change few things:

  • definitions/service.proto and coreapi/api.proto does not share the same proto package anymore
  • service.proto has its own proto package definitions
  • api.proto include definitions package and uses now definitions.Service instead of just Service
  • Now api.proto can have its own Service message
  • New API is Service { Status: enum, Definition: definitions.Service } so we can access like response.service.definition

Only problem with that is definitions/service.proto has to define the go package option go_package = "github.com/mesg-foundation/core/protobuf/definitions"; in order to generate correctly the pb.go file but it also automatically export the service.pb.go in this directory so I added a hack to mv the file after generated in the build-proto.sh

If anyone knows a better solution i would be really happy :)

@NicolasMahe
Copy link
Member

NicolasMahe commented Apr 5, 2019

Only problem with that is definitions/service.proto has to define the go package option go_package = "github.com/mesg-foundation/core/protobuf/definitions"; in order to generate correctly the pb.go file but it also automatically export the service.pb.go in this directory so I added a hack to mv the file after generated in the build-proto.sh

Fixed in b09e24f

Copy link
Contributor

@ilgooz ilgooz left a comment

Choose a reason for hiding this comment

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

I prefer singular name definition instead of definitions

other than that lgtm

NicolasMahe
NicolasMahe previously approved these changes Apr 5, 2019
Copy link
Member

@NicolasMahe NicolasMahe left a comment

Choose a reason for hiding this comment

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

I approve the concept of this PR but i would like definitions to be renamed to definition.
I already did it in this following PR: #852.
So we can merge in whatever way.

Copy link
Contributor

@ilgooz ilgooz left a comment

Choose a reason for hiding this comment

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

manual tests are ok 👍

@ilgooz ilgooz merged commit 0228c4b into dev Apr 7, 2019
@NicolasMahe NicolasMahe deleted the feature/split-proto-service branch May 22, 2019 07:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants