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

ServiceSDK create function should accept another struct than service.Service #1147

Closed
NicolasMahe opened this issue Jul 1, 2019 · 2 comments · Fixed by #1305
Closed

ServiceSDK create function should accept another struct than service.Service #1147

NicolasMahe opened this issue Jul 1, 2019 · 2 comments · Fixed by #1305
Labels

Comments

@NicolasMahe
Copy link
Member

Related to #1141 (comment)

The ServiceSDK create function should accept another struct than service.Service.
It doesn't make sense to create a resource by passing the already created struct to the function that create it.
It makes validation harder by having to make compatibility with fields that should be field only after real creation (eg: hash).

As there is too many parameters to pass to the service.create function, a nice new struct should be created.

The function fromProtoService functions in service api should be replaced by a function that convert the proto definition to this new service.create struct.

@krhubert
Copy link
Contributor

krhubert commented Jul 1, 2019

There is more option to discuss:

  • stay as it is (pass service.Service)
  • use gogo protobuf and get rid of service.Service and use an only protobuf definition
  • pass protobuf definition CreateServiceRequest
  • create a new struct to pass all args.

I've also put a low priority, as this one is not blocking us.

@antho1404 antho1404 added refactoring good first issue Good for newcomers help wanted Extra attention is needed and removed good first issue Good for newcomers labels Jul 9, 2019
@ilgooz
Copy link
Contributor

ilgooz commented Jul 9, 2019

I think protobuf types should be only available inside the gRPC server. SDK shouldn't be aware of protobuf and gRPC. If Engine exposes other types of APIs in future, SDK shouldn't be affected by this.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants