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

gRPC client for auraed #27

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

manuelcoppotelli
Copy link

@manuelcoppotelli manuelcoppotelli commented Jan 8, 2023

Base gRPC client for auraed, as defined in #13

This is a work-in-progress PR so that everyone can provide feedback.
At the moment only the Cell service is implemented.

The client can successfully connect to auraed with gRPC with mTLS over socket.

@dmah42
Copy link
Contributor

dmah42 commented Jan 8, 2023

do we need to commit the results of go mod vendor or can this be run as part of the build?

@manuelcoppotelli
Copy link
Author

@dmah42 Good point, usually it is not to be committed... in some cases where is required the compatibility among different go versions it might be usefult.

However, it's up to the project mantainers

@dmah42
Copy link
Contributor

dmah42 commented Jan 8, 2023

this looks good so far.. is there much more you want to do?

@krisnova krisnova marked this pull request as ready for review January 9, 2023 03:06
@krisnova krisnova marked this pull request as draft January 9, 2023 03:08
@krisnova
Copy link
Contributor

krisnova commented Jan 9, 2023

Sorry! I accidentally hit the button. Please ignore that.

@manuelcoppotelli
Copy link
Author

@dmah42 yeah, I'm going on implementing other RPC calls.
The aim of this draft PR was to collect feedback on the "basic structure" of the client.

Here's some point I'd like to go deeper with:

  1. Should we wether to consider the ae/client directory as a separate golang workspace (thus having its own go.mod) or leaving as a simple sub-module of the ae CLI
  2. Committing the vendor directory would bring any benefit for the project?
  3. Do we like the naming convention for public client interface (e.g. AllocateCell(), FreeCell()) or we prefer to stick with the naming used internally (i.e. CellServiceAllocate(...))
  4. Are we fine in using the grpc generated structs as Request and Response of the client interface (e.g. AllocateCell(runtimev0.CellServiceAllocateRequest{...})) or we prefer to hide the complexity inside the client logic (exposing something like AllocateCell(runtime.Cell{...}))?

@dmah42
Copy link
Contributor

dmah42 commented Jan 9, 2023

opinions:

  1. i don't know what the trade-off is. for Reasons™ i would like to be able to depend on a client library from another project (to save duplicating a bunch of stuff here like the proto generation) so if this helps, i'm all for it.
  2. i don't think so.. as long as semver holds we should be safe right?
  3. i have no strong opinion here.. i think mapping to underlying API is generally a good idea unless there's some behavioural difference that needs highlighting, but then Go has its own style and naming guide so...
  4. i don't see any need to hide it.. unless there's some future migration safety we gain? even then i don't think it's a big deal.

@RRethy
Copy link
Contributor

RRethy commented Jan 11, 2023

  1. Yes imo, separate workspace (that can be a separate PR though), if a client wants to use ae/client-go, they shouldn't need to bring in the entirety of ae.
  2. +1 to no
  3. +1 to mapping to underlying api
  4. +1 to using grpc generated structs for message types

@j0shgrant
Copy link

  1. Managing these in a separate and having to bump versions/address security concerns in two separate places does feel like an additional pain, so I'd be a +1 towards having this be a child module on the project's root go.mod.
  2. I'm very keen to learn more about the perceived benefits of this - I've not seen explicit vendoring in a while in a major Go project, but not averse if the reasoning backs it up. Neutral on this one.
  3. +1 for mapping to the underlying API, if the naming is unclear, we should address that at the API level instead of renaming it in a higher level client.
  4. +1 for using gRPC codegen, the less mapping we do, the easier it is to keep track of - again if the generated structs don't make sense, that feels more reflective of the API imo

@krisnova
Copy link
Contributor

Ping -- do we have an update on this?

@manuelcoppotelli
Copy link
Author

manuelcoppotelli commented Jan 23, 2023

@krisnova I have to refactor since the protobuf definition changed in the past week.
I am going on it...

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

Successfully merging this pull request may close these issues.

5 participants