feat(cli): add CLI entrypoint and Cobra root#127
Open
rogirun wants to merge 4 commits into
Open
Conversation
rogirun
requested review from
AviadHayumi,
Isan-Rivkin,
ronlv10,
shaked-bouktus and
yuval-gr
as code owners
Add the karta CLI as its own Go module (github.com/run-ai/karta/cli), mirroring the operator module, so cobra and future CLI dependencies stay out of the embeddable github.com/run-ai/karta library module. Wire the Cobra root with global persistent flags (--kubeconfig, -n/--namespace, -o/--output) and the two-noun command structure (workload, definition). Workload commands require -n/--namespace (single-namespace MVP); the output format is validated against table|wide|json|yaml. New dependency: github.com/spf13/cobra v1.10.2 (Apache-2.0; pulls github.com/spf13/pflag BSD-3 and github.com/inconshreveable/mousetrap BSD-3). Part of #94. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Roee Gil <roee.gil@run.ai>
rogirun
force-pushed
the
feat/cli-cobra-root
branch
from
92e2c9c to
4884711
Compare
Contributor
WalkthroughAdds the initial ChangesKarta CLI initial scaffolding
Estimated code review effort: 2 (Simple) | ~10 minutes Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
cli/cmd/root_test.go (1)
51-53: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAssert the invalid
-oerror text, not just that an error happened.The acceptance criteria call for a clear failure when the format is invalid. This test still passes if Cobra returns some unrelated error.
Proposed test tightening
if _, err := execute(t, "-o", "bogus", "noop"); err == nil { t.Error("expected error for invalid output format, got nil") + } else if !strings.Contains(err.Error(), "must be one of table, wide, json, yaml") { + t.Fatalf("unexpected error message: %v", err) } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/cmd/root_test.go` around lines 51 - 53, The test around execute currently only checks that an error occurs for an invalid -o value, which is too loose. Tighten the assertion in root_test.go by checking the actual error text returned from execute when passing "-o bogus noop", using the existing execute helper and the command setup in rootCmd so the test fails unless the invalid output-format message is specifically returned.cli/cmd/workload/workload_test.go (1)
44-49: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd the parent-command help-path test from the acceptance criteria.
These tests only cover
workload dummy. They do not lock in the promisedkarta workload -n <ns>behavior, where the parent command should print help instead of erroring.Possible follow-up diff
import ( "bytes" "errors" + "strings" "testing" "github.com/spf13/cobra" ) @@ -func newTestTree() *cobra.Command { +func newTestTree() (*cobra.Command, *bytes.Buffer) { cobra.EnableTraverseRunHooks = true root := &cobra.Command{Use: "root", SilenceUsage: true, SilenceErrors: true} root.PersistentFlags().StringP("namespace", "n", "", "") @@ - var out bytes.Buffer - root.SetOut(&out) - root.SetErr(&out) - return root + out := &bytes.Buffer{} + root.SetOut(out) + root.SetErr(out) + return root, out } func TestWorkloadRequiresNamespace(t *testing.T) { - root := newTestTree() + root, _ := newTestTree() root.SetArgs([]string{"workload", "dummy"}) if err := root.Execute(); !errors.Is(err, ErrNamespaceRequired) { t.Fatalf("expected ErrNamespaceRequired, got %v", err) } } func TestWorkloadWithNamespace(t *testing.T) { - root := newTestTree() + root, _ := newTestTree() root.SetArgs([]string{"-n", "ml-team", "workload", "dummy"}) if err := root.Execute(); err != nil { t.Fatalf("unexpected error with namespace set: %v", err) } } + +func TestWorkloadParentShowsHelpWithNamespace(t *testing.T) { + root, out := newTestTree() + root.SetArgs([]string{"-n", "ml-team", "workload"}) + if err := root.Execute(); err != nil { + t.Fatalf("unexpected error with namespace set: %v", err) + } + if !strings.Contains(out.String(), "Inspect workloads running in a namespace") { + t.Fatalf("expected workload help output, got %q", out.String()) + } +}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/cmd/workload/workload_test.go` around lines 44 - 49, Add the missing parent-command help-path test in TestWorkloadWithNamespace so it covers the accepted `karta workload -n <ns>` behavior, not just `workload dummy`. Use the existing `newTestTree()` setup and `root.Execute()` flow to assert that invoking the workload parent command with `-n` and no subcommand prints help instead of returning an error, alongside the current dummy subcommand coverage.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cli/cmd/root_test.go`:
- Around line 51-53: The test around execute currently only checks that an error
occurs for an invalid -o value, which is too loose. Tighten the assertion in
root_test.go by checking the actual error text returned from execute when
passing "-o bogus noop", using the existing execute helper and the command setup
in rootCmd so the test fails unless the invalid output-format message is
specifically returned.
In `@cli/cmd/workload/workload_test.go`:
- Around line 44-49: Add the missing parent-command help-path test in
TestWorkloadWithNamespace so it covers the accepted `karta workload -n <ns>`
behavior, not just `workload dummy`. Use the existing `newTestTree()` setup and
`root.Execute()` flow to assert that invoking the workload parent command with
`-n` and no subcommand prints help instead of returning an error, alongside the
current dummy subcommand coverage.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: dac27c04-7f70-40f1-82a1-6ecbc744c899
⛔ Files ignored due to path filters (1)
cli/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
cli/cmd/definition/definition.gocli/cmd/root.gocli/cmd/root_test.gocli/cmd/workload/workload.gocli/cmd/workload/workload_test.gocli/go.modcli/main.go
Isan-Rivkin
requested changes
Jun 30, 2026
Isan-Rivkin
requested changes
Jun 30, 2026
Move -n/--namespace onto the workload command as a required flag (was a persistent root flag guarded by a pre-run hook) and turn -o/--output into a proper enum validated at parse time. Introduce two domains: cli/internal/flagtypes holds the flag value structures (Output enum implementing pflag.Value), and cli/cmd/flags registers the CLI flags on cobra commands using those types. This removes the root PersistentPreRunE output check and the now-unused EnableTraverseRunHooks. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Roee Gil <roee.gil@run.ai>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cli/internal/flagtypes/output.go (1)
29-37: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winDerive the error message from
OutputValuesto avoid drift.The error string in
Sethardcodes the same list already defined inOutputValues(lines 23-25). If a new output format is added later, it's easy to update one and forget the other.♻️ Proposed fix
-import "fmt" +import ( + "fmt" + "strings" +)default: - return fmt.Errorf("must be one of table, wide, json, yaml") + return fmt.Errorf("must be one of %s", strings.Join(OutputValues, ", ")) }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/internal/flagtypes/output.go` around lines 29 - 37, The error message in Output.Set is hardcoded and can drift from the canonical list in OutputValues. Update Set to build the validation error from OutputValues instead of repeating the format names, so any future output type changes are reflected automatically. Use the existing OutputValues symbol as the source of truth and keep the assignment logic in Set unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cli/internal/flagtypes/output.go`:
- Around line 29-37: The error message in Output.Set is hardcoded and can drift
from the canonical list in OutputValues. Update Set to build the validation
error from OutputValues instead of repeating the format names, so any future
output type changes are reflected automatically. Use the existing OutputValues
symbol as the source of truth and keep the assignment logic in Set unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 6fc103e5-6f2f-4261-93e1-43b3776e61aa
📒 Files selected for processing (7)
cli/cmd/flags/flags.gocli/cmd/root.gocli/cmd/root_test.gocli/cmd/workload/workload.gocli/cmd/workload/workload_test.gocli/internal/flagtypes/output.gocli/internal/flagtypes/output_test.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cli/cmd/root_test.go
Isan-Rivkin
requested changes
Jul 2, 2026
| // Package definition holds the "karta definition" command tree: inspection of | ||
| // the Karta definitions the CLI understands. Definitions are cluster-scoped, so | ||
| // these commands do not require a namespace. | ||
| package definition |
Contributor
There was a problem hiding this comment.
Aiming for simplicity - why do we need there a dedicated package here?
Contributor
Author
There was a problem hiding this comment.
Each subcommand has its own package.
for start we will need
karta workload describe ...
and
karta desribe ...
Contributor
There was a problem hiding this comment.
That doesn't explain, why do we need a dedicated package for commands?
Why can't the commands be:
cli/cmd/
workload.go
workload_describe.go // optional if its really big...
describe.go
| *o = Output(v) | ||
| return nil | ||
| default: | ||
| return fmt.Errorf("must be one of table, wide, json, yaml") |
Contributor
There was a problem hiding this comment.
convert to sentinel error
Contributor
Author
There was a problem hiding this comment.
But it's a dynamic error based on the enum.
Replace flags.AddGlobals/AddNamespace with one WithKubeconfig, WithOutput, and WithNamespace function per flag, adopting the With* naming convention. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Roee Gil <roee.gil@run.ai>
Address review feedback on PR #127: - Introduce a reusable generic Enum[T ~string] implementing pflag.Value; define Output via it. Validation error wraps a sentinel (ErrInvalidValue) and derives the allowed-values list from the enum, removing the hardcoded message. - Trim verbose comments on the workload command. - Tighten tests: assert the invalid -o error text, cover the workload parent help path, and check errors.Is(ErrInvalidValue). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com> Signed-off-by: Roee Gil <roee.gil@run.ai>
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
cli/cmd/workload/workload_test.go (1)
53-64: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueConsider testing
workloadalone with no-n.
TestWorkloadRequiresNamespacecoversworkload dummywithout-n, and this test coversworkload -n ml-team(parent, no subcommand). The remaining combination — invokingworkloaditself with no subcommand and no-n— is untested, though it's a reasonable acceptance-criteria case since required-flag validation runs before the parent's ownRunE.🧪 Optional test to add
func TestWorkloadParentRequiresNamespace(t *testing.T) { root, _ := newTestTree() root.SetArgs([]string{"workload"}) if err := root.Execute(); err == nil { t.Fatal("expected an error when namespace is omitted, got nil") } }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@cli/cmd/workload/workload_test.go` around lines 53 - 64, Add coverage for the missing `workload` parent-command case in `TestWorkloadParentShowsHelp`/a new test alongside `TestWorkloadRequiresNamespace`: invoke `root.Execute()` with just `workload` and no `-n`, and assert it fails with the required-namespace validation instead of reaching the parent help path. Use the existing `newTestTree()` setup and the `workload` command registration to verify the behavior before `RunE` on the parent command is considered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@cli/cmd/workload/workload_test.go`:
- Around line 53-64: Add coverage for the missing `workload` parent-command case
in `TestWorkloadParentShowsHelp`/a new test alongside
`TestWorkloadRequiresNamespace`: invoke `root.Execute()` with just `workload`
and no `-n`, and assert it fails with the required-namespace validation instead
of reaching the parent help path. Use the existing `newTestTree()` setup and the
`workload` command registration to verify the behavior before `RunE` on the
parent command is considered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Enterprise
Run ID: 738accf9-ef81-454c-92af-d35edd3f522b
📒 Files selected for processing (8)
cli/cmd/flags/flags.gocli/cmd/root.gocli/cmd/root_test.gocli/cmd/workload/workload.gocli/cmd/workload/workload_test.gocli/internal/flagtypes/enum.gocli/internal/flagtypes/output.gocli/internal/flagtypes/output_test.go
🚧 Files skipped from review as they are similar to previous changes (3)
- cli/cmd/workload/workload.go
- cli/cmd/root_test.go
- cli/internal/flagtypes/output_test.go
Isan-Rivkin
requested changes
Jul 2, 2026
| // Package definition holds the "karta definition" command tree: inspection of | ||
| // the Karta definitions the CLI understands. Definitions are cluster-scoped, so | ||
| // these commands do not require a namespace. | ||
| package definition |
Contributor
There was a problem hiding this comment.
That doesn't explain, why do we need a dedicated package for commands?
Why can't the commands be:
cli/cmd/
workload.go
workload_describe.go // optional if its really big...
describe.go
|
|
||
| // Package flags defines and registers the karta CLI's flags on cobra commands, | ||
| // backing them with the value structures in the flagtypes package. | ||
| package flags |
Contributor
There was a problem hiding this comment.
Let's simplify this, keep package cmd.
Simply put it in root.go or a dedicated flags.go file - Whatever
Packages heres just bloat the code
| return root, out | ||
| } | ||
|
|
||
| func TestWorkloadRequiresNamespace(t *testing.T) { |
Contributor
There was a problem hiding this comment.
isn't this test redundant just testing the build-in framework requiredflags?
(not a blocker but not sure it provides value or confident)
| // SPDX-License-Identifier: Apache-2.0 | ||
| // Copyright (c) 2026 NVIDIA Corporation | ||
|
|
||
| package flagtypes |
Contributor
There was a problem hiding this comment.
Path complexity cli/internal/flagtypes - This can simply be a file under cli/cmd/enumflag.go
Don't see why we need to introduce internal + a dedicated go package for flagtypes
| // Package flagtypes holds the flag value structures for the karta CLI: types | ||
| // that implement pflag.Value and define a flag's shape and behavior (parse, | ||
| // validate, print). It starts with the output-format enum. | ||
| package flagtypes |
Contributor
There was a problem hiding this comment.
bloat, Path complexity cli/internal/flagtypes - This can simply be a file under cli/cmd/enumflag.go
Don't see why we need to introduce internal + a dedicated go package for flagtypes
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this PR do?
Adds the foundation for the Karta CLI: a Cobra root command for the
kartabinary, built as its own Go module (github.com/run-ai/karta/cli) that mirrors the existingoperatormodule. Keeping it a separate module means cobra and future CLI dependencies stay out of the embeddablegithub.com/run-ai/kartalibrary module (it references the library viareplace => ../, same asoperator/).--kubeconfig,-n/--namespace,-o/--output(validated againsttable|wide|json|yaml).karta workloadandkarta definition(parent commands only; their subcommands land in later tasks of the CLI epic).workloadcommands require-n/--namespace(single-namespace MVP; no all-namespaces mode).github.com/spf13/cobra v1.10.2(Apache-2.0; pullsgithub.com/spf13/pflagBSD-3 andgithub.com/inconshreveable/mousetrapBSD-3).Acceptance:
karta --helpworks and lists both nouns plus the global flags;karta workloaderrors with a clear message and non-zero exit when-nis omitted; with-nit renders help.Related issue(s)
Part of #94
Checklist
git commit -s)make check)🤖 Generated with Claude Code
Summary by CodeRabbit
kartaCLI withdefinitionandworkloadcommand groups.--kubeconfigand-o/--outputflags, plus required-n/--namespacefor workload commands.table,wide,json,yaml.--namespace, and the parent command shows help when no subcommand is provided.