Add initial go client #227

RRethy · 2023-01-02T05:56:56Z

Follow-up to #226 so the first 16 commits are duplicated from that branch, unfortunately GitHub won't let me use that PR as the base since it's not under aurae-runtime.

This PR adds an initial Go client under client-go. It only has code generate by buf, a future PR (probably some time tmrw or tuesday) will have a more fleshed out client.

The PR boils down to adding this to buf.gen.yaml, and running make proto.

  - plugin: buf.build/protocolbuffers/go:v1.28.1
    out: client-go/pkg/api
    opt: paths=source_relative
  - plugin: buf.build/grpc/go:v1.2.0
    out: client-go/pkg/api
    opt: paths=source_relative

See https://developers.google.com/protocol-buffers/docs/style#packages

The previous structure was incompatible with the gRPC Go plugin for the buf tool since the generated Go code would get generated into a single directory with each file having a different package. This restructure also moves the (pod|spawn|instance|cell) services into their own proto file under the runtime package to simply future client code.

…idy`

dmah42 · 2023-01-02T07:11:23Z

meta comment: I don't think we should be committing generated code.

future-highway · 2023-01-02T13:00:42Z

crates/aurae-client/src/runtime/cell_service.rs


 macros::ops_generator!(
-    runtime,
+    cell,


This isn't right as the generated code will not result in a guaranteed unique, namespaced and predictable name.

We have: ae__runtime__cell_service__allocate

Here we get: ae__cell__cell_service__allocate

We want: ae__runtime_cell__cell_service__allocate

Side note: would we want "cell", "pod" etc. or should they be plural (e.g., "cells", "pods", etc.) which sounds more natural to me? (I don't know what the convention is)

Realizing I got this mixed up with auraescript's macro. I still suspect there will be an issue in the generated code. I'll have to pull down the PR to check.

future-highway · 2023-01-02T13:22:32Z

api/runtime/cell/v0/cell.proto

@@ -29,68 +29,10 @@
 \* -------------------------------------------------------------------------- */


I like that the services are broken into smaller files. Should there be a higher level runtime file? If we have shared messages, where would we put those?

the issue i have here is that each broken down part now has its own version, which i don't think is what we want.. we want aurae.. (where service is runtime or runtime.cells, or whatever).

having aurae.. implies that we might have runtime.cells.v1 and observe.v0 which i would like to avoid.

Definitely agree with the version being in the wrong spot. I'm also inclined to agree with your comment overall (more context).

My only reason for liking the multiple files is that smaller files are easier to grasp, but that is not a good reason to break things up if it causes other issues.

I posted a follow-up on the structure, #226 (comment).

future-highway · 2023-01-02T13:29:43Z

client-go/go.mod

@@ -0,0 +1,16 @@
+module github.com/aurae-runtime/aurae/client-go


I thought the go code would be in the ae repo. It doesn't matter to me; I'm just checking that the contributors working on it (I know that includes you) are in agreement that this is the right decision at this point in time.

Side note: @krisnova @dmah42 if we make this repo have more general code, maybe we should consider moving all the rust crates into the crates directory.

yeah i had expected this to be in a separate repo. i didn't notice this.

What's the rational behind putting it in a different repo?

My logic for putting it in aurae-runtime/aurae was the following:

Keep the source of truth in the same repo to avoid getting out sync

The rust and typescript generated code are in this repo as well

The proto files will have a option go_package = "..." which points to it's own repo instead of an external repo that can easily break

If we want to move the code into another repo, we could follow kubernetes/client-go which just syncs that code from https://github.com/kubernetes/kubernetes/tree/master/staging/src/k8s.io/client-go which acts as the source of truth (and lives alongside the types it exposes)

If you all don't want it here though I can close the PR and we can figure out a more appropriate place for it.

future-highway · 2023-01-02T13:35:33Z

auraescript/gen/cell.ts

+export class CellServiceClient implements CellService {
+allocate(request: CellServiceAllocateRequest): Promise<CellServiceAllocateResponse> {
+    // @ts-ignore
+    return Deno.core.ops.ae__cell__cell_service__allocate(request);


See my comment incorrectly placed on the aurae-client code

future-highway · 2023-01-02T13:37:13Z

auraescript/macros/src/ops.rs

@@ -166,7 +166,7 @@ fn typescript_generator(input: &OpsGeneratorInput) {

    let ts_path = {
        let mut out_dir = gen_dir.clone();
-        out_dir.push(format!("v0/{module}.ts"));
+        out_dir.push(format!("runtime/{module}/v0/{module}.ts"));


This isn't the correct way to fix the macro as we have more modules than runtime, but if it works in this PR and you are not comfortable making the change, I can fix it after if we merge this as is.

RRethy · 2023-01-02T14:37:41Z

meta comment: I don't think we should be committing generated code.

Would this apply to the rust/typescript generate code?

I've usually seen gRPC generate code being an exception to that rule. The issue I'd see with not committing the generated code is that if we provide a client wrapper around the generated code (similar to https://github.com/aurae-runtime/client-go/blob/main/client.go), it would break downloading the code with go get .../client-go since the code wouldn't be generated for downstream users.

dmah42 · 2023-01-02T14:53:50Z

meta comment: I don't think we should be committing generated code.

Would this apply to the rust/typescript generate code?

I've usually seen gRPC generate code being an exception to that rule. The issue I'd see with not committing the generated code is that if we provide a client wrapper around the generated code (similar to https://github.com/aurae-runtime/client-go/blob/main/client.go), it would break downloading the code with go get .../client-go since the code wouldn't be generated for downstream users.

it does apply to that, yes. in fact we don't have that generated code at head any more.

perhaps this removal was a mistake.. i wasn't aware that go required the generation to be checked in. that's less than ideal (it makes for noisy commits, merge issues, and sometimes people forget to generate the code before committing).

unless we have some github workflow that generates the code and checks it in instead, but that's an education problem.

future-highway · 2023-01-02T14:54:56Z

meta comment: I don't think we should be committing generated code.

Would this apply to the rust/typescript generate code?

Just answering ^^^

Yes, I would expect it to apply to all generated code. The only benefit I'm seeing from checking in generated code is that I can see that changes in the generators are outputting the expected result. However, that can't even be trusted. How can we be sure a PR generated the code as the last change before pushing? I think I've made the mistake before where I didn't include the changes in the generated code in the PR. If someone pulled the source at that point, the generated code would be correct only after running the build.

I don't know/haven't considered if an exception is warranted.

krisnova · 2023-01-02T17:54:05Z

Are we putting Go code in this repository now?

I thought we had decided to put the client-go here as per the aurae-runtime/ae#13 (comment).

RRethy · 2023-01-02T18:21:47Z

Are we putting Go code in this repository now?

I thought we had decided to put the client-go here as per the aurae-runtime/ae#13 (comment).

Ah I didn't see that issue, I'll close this and move discussion into that issue.

krisnova · 2023-01-02T18:31:45Z

@RRethy please mention either in the Discord or on the issue that you intend on working on it. I know other people are also considering this.

RRethy added 20 commits January 2, 2023 02:52

Rename proto packages based on style guide to be unique

e55b48d

See https://developers.google.com/protocol-buffers/docs/style#packages

Run rm -rf aurae-proto/src/gen auraescript/gen

7b48594

Run make proto

382488a

Fix aurae-proto/src/lib.rs paths in include macro

aae7c16

Generate auraescript/gen/helper.ts and auraescript/gen/runtime.ts

a756a69

Run make docs

06ea01a

Run rm -rf aurae-proto/src/gen auraescript/gen

3b66234

Run make proto

7c02e34

Fix include paths for aurae-proto/src/lib.rs

f0f1686

Fix auraescript which broke due to proto restructuring

cf550ec

Fix aurae-client which broke due to restructuring

3243b62

Fix auraed which broke due to proto restructuring

54a51ae

Fix make stdlibdocs

eb9e359

Run make docs

1349514

Fix make check-docs

19961a6

Add go_package option to proto files

c117cb4

Add Go protocolbuffers and grpc plugins to buf.gen.yaml

1447aa8

Run make proto

d3f89b8

Run `go mod init github.com/aurae-runtime/aurae/client-go && go mod t…

11cde8c

…idy`

cla-bot bot added the Access Granted label Jan 2, 2023

RRethy mentioned this pull request Jan 2, 2023

Restructure proto files #226

Merged

future-highway reviewed Jan 2, 2023

View reviewed changes

RRethy closed this Jan 2, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add initial go client #227

Add initial go client #227

RRethy commented Jan 2, 2023

dmah42 commented Jan 2, 2023

future-highway Jan 2, 2023

future-highway Jan 2, 2023

future-highway Jan 2, 2023

dmah42 Jan 2, 2023

future-highway Jan 2, 2023

RRethy Jan 2, 2023

future-highway Jan 2, 2023

dmah42 Jan 2, 2023

RRethy Jan 2, 2023

future-highway Jan 2, 2023

future-highway Jan 2, 2023

RRethy commented Jan 2, 2023

dmah42 commented Jan 2, 2023

future-highway commented Jan 2, 2023

krisnova commented Jan 2, 2023

RRethy commented Jan 2, 2023

krisnova commented Jan 2, 2023

		@@ -29,68 +29,10 @@
		\* -------------------------------------------------------------------------- */

		@@ -0,0 +1,16 @@
		module github.com/aurae-runtime/aurae/client-go

Add initial go client #227

Add initial go client #227

Conversation

RRethy commented Jan 2, 2023

dmah42 commented Jan 2, 2023

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

RRethy commented Jan 2, 2023

dmah42 commented Jan 2, 2023

future-highway commented Jan 2, 2023

krisnova commented Jan 2, 2023

RRethy commented Jan 2, 2023

krisnova commented Jan 2, 2023