feat: add pipelines ls cmd (+client API) #2063
Conversation
| type Client struct { | ||
| conn *grpc.ClientConn | ||
| pipelineService apiv1.PipelineServiceClient | ||
| HealthService healthgrpc.HealthClient | ||
| } |
There was a problem hiding this comment.
Thinking out loud - do we need a client abstraction here? Seems like it will contain just lots of boilerplate code. Can we get away with just giving the grpc client to the commands? 🤔 Or maybe, for convenience, it could just combine all service clients and make them available as public struct fields (same as HealthService), then we don't have to write methods that forward calls to the specific service client.
There was a problem hiding this comment.
IMHO it plays a role how we manage to inject the client into commands. Having a single client that holds all services could be handy, because it might be easier to instantiate once and inject into all commands that need it.
|
|
||
| conduitGRPCAddr := c.RunCmd.GRPCAddress() | ||
|
|
||
| conduitClient, err := api.NewClient(ctx, conduitGRPCAddr) |
There was a problem hiding this comment.
Not sure if possible, but we could think about injecting the client with a decorator through ecdysis. Similarly how we will inject a logger. Didn't we do something similar in the meroxa CLI?
| @@ -76,7 +76,7 @@ func (c *RootCommand) SubCommands() []ecdysis.Command { | |||
| &config.ConfigCommand{RunCmd: runCmd}, | |||
| &initialize.InitCommand{Cfg: &runCmd.Cfg}, | |||
| &version.VersionCommand{}, | |||
| &pipelines.PipelinesCommand{}, | |||
| &run.RunCommand{}, | |||
| &pipelines.PipelinesCommand{RunCmd: runCmd}, | |||
There was a problem hiding this comment.
I guess you're trying to inherit the flags from the run command? That won't work, since the pipelines command is not a subcommand of run. It would only work if RootCommand had the flags attached and they were marked as persistent (see the example).
| if !c.Cfg.API.Enabled { | ||
| fmt.Print("Warning: API is currently disabled. Most Conduit CLI commands won't work without the API enabled." + | ||
| "To enable it, run conduit with `--api.enabled=true` or set `CONDUIT_API_ENABLED=true` in your environment.") | ||
| } |
| _ ecdysis.CommandWithExecute = (*ListCommand)(nil) | ||
| _ ecdysis.CommandWithAliases = (*ListCommand)(nil) | ||
| _ ecdysis.CommandWithDocs = (*ListCommand)(nil) | ||
| // with API client ? |
There was a problem hiding this comment.
IMO that would be really nice and would make writing new commands easier.
Description
Quick checks