feat(cli): add API v2 AgentInstance workflows - #2559
Conversation
EItanya
left a comment
There was a problem hiding this comment.
Initial review, I'm still looking for more simplifications, but this is a good start
85151d8 to
f4d2a51
Compare
Signed-off-by: Cody Hartsook <cody.hartsook@solo.io>
Signed-off-by: Cody Hartsook <cody.hartsook@solo.io>
Signed-off-by: Cody Hartsook <cody.hartsook@solo.io>
Signed-off-by: Cody Hartsook <cody.hartsook@solo.io>
Signed-off-by: Cody Hartsook <cody.hartsook@solo.io>
Signed-off-by: Cody Hartsook <cody.hartsook@solo.io>
f2bdea9 to
2c71aee
Compare
| checkServer func(context.Context, *client.ClientSet) error | ||
| commandContext func(context.Context, string, ...string) *exec.Cmd |
There was a problem hiding this comment.
I really really hate callbacks unless absolutely necessary. Can you explain this component and why we need these?
There was a problem hiding this comment.
Yeah, fair point. These callbacks mainly exist for tests and make the production path harder to follow. I’ll remove the runtime wrapper, call the real functions directly.
| func buildKagentCLI(t *testing.T) string { | ||
| t.Helper() | ||
| _, source, _, ok := goruntime.Caller(0) | ||
| if !ok { | ||
| t.Fatal("locate interaction test source") | ||
| } | ||
| moduleRoot := filepath.Clean(filepath.Join(filepath.Dir(source), "../../..")) | ||
| binary := filepath.Join(t.TempDir(), "kagent") | ||
| command := exec.CommandContext(t.Context(), "go", "build", "-o", binary, "./core/cli/cmd/kagent") | ||
| command.Dir = moduleRoot | ||
| if output, err := command.CombinedOutput(); err != nil { | ||
| t.Fatalf("build CLI: %v\n%s", err, output) | ||
| } | ||
| return binary | ||
| } |
There was a problem hiding this comment.
I'd rather not build from the tests, can we use pre-built?
There was a problem hiding this comment.
Done, the E2E target builds the CLI once and passes it through KAGENT_E2E_CLI. CI uses the same path
| } | ||
| } | ||
|
|
||
| func TestCLIAgentInstanceDiscoveryAndInvoke(t *testing.T) { |
There was a problem hiding this comment.
All e2e tests should be prefixed with TestE2E
There was a problem hiding this comment.
Updated the two E2E tests added by this PR to use TestE2E
| "--kagent-grpc-url", target, | ||
| "--kagent-grpc-tls=false", |
There was a problem hiding this comment.
Why do these flags need the kagent prefix?
There was a problem hiding this comment.
Removed the prefix from the new gRPC flags: --grpc-url, --grpc-tls, --grpc-ca-file, and --grpc-server-name.
| } | ||
| } | ||
|
|
||
| func TestCLIAgentInstanceDiscoveryAndInvoke(t *testing.T) { |
There was a problem hiding this comment.
Can we group all of the cli tests in the same file
There was a problem hiding this comment.
both CLI E2E tests are now in cli_catalog_lifecycle_test.go
| type Config struct { | ||
| KAgentURL string `mapstructure:"kagent_url"` | ||
| KAgentGRPCURL string `mapstructure:"kagent_grpc_url"` | ||
| KAgentGRPCTLS bool `mapstructure:"kagent_grpc_tls"` | ||
| KAgentGRPCCAFile string `mapstructure:"kagent_grpc_ca_file"` | ||
| KAgentGRPCServerName string `mapstructure:"kagent_grpc_server_name"` | ||
| Namespace string `mapstructure:"namespace"` | ||
| OutputFormat string `mapstructure:"output_format"` | ||
| Verbose bool `mapstructure:"verbose"` | ||
| Timeout time.Duration `mapstructure:"timeout"` | ||
| } |
There was a problem hiding this comment.
I think we still need this config to be embedded in the other cmd configs.
type AgentInstanceConfig struct {
BaseConfig
}
Something like the above
There was a problem hiding this comment.
I ended up addressing this in PR #2606 by reading shared connection and output flags at each command boundary, so BaseConfig would no longer be needed. Would it be okay to leave this as-is here and handle it in that follow-up or prefer we address here?
Signed-off-by: Cody Hartsook <cody.hartsook@solo.io>
Signed-off-by: Cody Hartsook <cody.hartsook@solo.io>
Signed-off-by: Cody Hartsook <cody.hartsook@solo.io>
Summary
mcpNote that of the 64 files changed, 35 were removals from the legacy CLI project workflow ; no server-side APIs or controllers are removed.
This is the first of three related CLI PRs:
main.goand align the command structure with agentregistry.Suspend and resume are deliberately not exposed by this CLI work.
Related to #2366 and the K13 CLI cutover in
docs/plans/api-v2-execution-plan.md.Test plan
make -C go testgo test ./core/cli/...make -C go lintmake -C go build