-
Notifications
You must be signed in to change notification settings - Fork 729
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
Add remote execution for codegen #2214
Conversation
|
||
remote-proto: | ||
protoc \ | ||
--go_out=. --go_opt="Minternal/remote/gen.proto=github.com/kyleconroy/sqlc/internal/remote" --go_opt=module=github.com/kyleconroy/sqlc \ | ||
--go-grpc_out=. --go-grpc_opt="Minternal/remote/gen.proto=github.com/kyleconroy/sqlc/internal/remote" --go-grpc_opt=module=github.com/kyleconroy/sqlc \ | ||
internal/remote/gen.proto |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should get this working using buf generate
to ensure that we're consistently using the same code for gRPC. This isn't blocking.
internal/cmd/generate.go
Outdated
@@ -140,6 +144,69 @@ func Generate(ctx context.Context, e Env, dir, filename string, stderr io.Writer | |||
return nil, err | |||
} | |||
|
|||
if conf.Cloud.Hostname != "" && !e.NoRemote { | |||
rpcClient, err := remote.NewClient(conf.Cloud) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Instead of inlining this inside Generate
, could we move it to a separate remoteGenerate
function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's a good idea. The function signature is a bit long but I think it's better. See changes incoming.
grpc.WithPerRPCCredentials(bearer.NewPerRPCCredentials(os.Getenv("SQLC_AUTH_TOKEN"))), | ||
} | ||
|
||
conn, err := grpc.Dial(cloudConfig.Hostname+":443", opts...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The hostname currently doesn't default to anything other than an empty string. Should we pick a default now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The empty string default ends up as the equivalent of "localhost" in the current implementation. We could add an explicit default though. To "localhost", or something else?
2e084c9
to
2b9d4ee
Compare
Enable remote execution for
sqlc generate
when the"cloud"
configuration block exists. To disable remote execution without altering your config, a new--no-remote
flag is available.While developing this I ran the entire endtoend test suite against the remote codegen service to confirm correctness. In this patch I include a single remote-specific test that covers both a successful code generation and an expected error. This test passes on my machine but is disabled by default, since it will always fail unless a locally-running implementation of the remote execution service is available.