feat(trace): add watch for trace#7015
Conversation
|
Great stuff, this will come in super handy! |
📝 WalkthroughWalkthroughAdds a Watch flag to the trace command and a watch-mode flow: initializes a watch-capable Kubernetes client and, for single-resource traces with --watch, launches a Bubble Tea UI and background producer to continuously fetch and render the resource tree until quit or deletion. Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as Trace CLI
participant K8s as Kubernetes Client
participant Producer as Background Producer
participant UI as Bubble Tea UI
User->>CLI: run `trace --watch`
CLI->>K8s: setupKubeClient (NewWithWatch)
CLI->>UI: start Bubble Tea program
CLI->>Producer: spawn watchProducer goroutine
loop every 2s
Producer->>K8s: GetResource / getResourceTree
K8s-->>Producer: snapshot or NotFound/error
Producer->>Producer: renderTreeToString
Producer->>UI: send treeUpdateMsg / treeErrMsg / treeQuitMsg
UI->>UI: update model & render
UI-->>User: display updated tree or error
end
alt Resource deleted or ctx cancelled
Producer->>UI: treeQuitMsg
UI-->>CLI: exit program
else Error (non-NotFound)
Producer->>UI: treeErrMsg
UI-->>User: display error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Thanks — would you like me to flag specific lines for review or suggest unit/integration tests for the new watch flow? 🚥 Pre-merge checks | ✅ 6✅ Passed checks (6 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/crank/beta/trace/watch.go`:
- Around line 110-121: Replace the terse error returns around the watch setup
with user-friendly messages: when the type assertion for client.WithWatch on
kClient fails, return a message like "watching this resource is not supported by
your cluster client" (referencing kClient and client.WithWatch) so users know
the client lacks watch support; when watchClient.Watch(ctx, obj, opts) returns
err, wrap it into a clear CLI-friendly message that omits internal stack details
but includes the underlying error text for context and a suggested next step
(e.g., "cannot start watching resource: <underlying error>; verify cluster
permissions and API support"), using the same watchClient.Watch call and errors
wrapping logic to preserve the underlying error string without exposing raw
internals.
🧹 Nitpick comments (3)
cmd/crank/beta/trace/watch.go (2)
1-2: Minor: Copyright year may need updating.The copyright year is listed as 2023, but this is new code being added in 2026. Would you like to update this to 2026 to reflect when the file was created?
138-145: Consider making the refresh interval configurable.The 2-second ticker is hardcoded. For users watching resources that change frequently or infrequently, having this configurable could improve the experience. This could be a nice enhancement for a follow-up PR if you think it's valuable.
cmd/crank/beta/trace/trace.go (1)
238-242: Consider warning users when--watchis used without a specific resource name.The condition
!shouldPrintAsListsilently falls through to list printing if--watchis used without specifying a resource name. This might confuse users who expect watch behavior.Would it be helpful to add a warning message (via logger) or return an error when
--watchis used in list mode? Something like:if c.Watch && shouldPrintAsList { logger.Info("--watch is only supported for a single resource, ignoring") }What do you think? I'd love to understand the intended UX here.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
go.modis excluded by none and included by nonego.sumis excluded by!**/*.sumand included by none
📒 Files selected for processing (2)
cmd/crank/beta/trace/trace.gocmd/crank/beta/trace/watch.go
🧰 Additional context used
📓 Path-based instructions (3)
{apis,cmd}/**
📄 CodeRabbit inference engine (Custom checks)
{apis,cmd}/**: Public fields/flags in apis/** or cmd/** must not be removed or renamed without a 'breaking-change' label
New required public fields/flags in apis/** or cmd/** must not be added without a 'breaking-change' label
Behavior removals in apis/** or cmd/** must be labeled with 'breaking-change'
Files:
cmd/crank/beta/trace/trace.gocmd/crank/beta/trace/watch.go
**/*.go
⚙️ CodeRabbit configuration file
**/*.go: Enforce Crossplane-specific patterns: Use crossplane-runtime/pkg/errors
for wrapping. Check variable naming (short for local scope, descriptive
for wider scope). Ensure 'return early' pattern. Verify error scoping
(declare in conditionals when possible). For nolint directives, require
specific linter names and explanations. CRITICAL: Ensure all error
messages are meaningful to end users, not just developers - avoid
technical jargon, include context about what the user was trying to do,
and suggest next steps when possible.
Files:
cmd/crank/beta/trace/trace.gocmd/crank/beta/trace/watch.go
**/cmd/**
⚙️ CodeRabbit configuration file
**/cmd/**: Review CLI commands for proper flag handling, help text, and error
messages. Ensure commands follow Crossplane CLI conventions. Ask about
backward compatibility and user experience. CLI error messages must be
especially user-friendly - avoid internal error details, provide
actionable guidance.
Files:
cmd/crank/beta/trace/trace.gocmd/crank/beta/trace/watch.go
🧠 Learnings (4)
📓 Common learnings
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Follow Crossplane's Observability Developer Guide
📚 Learning: 2025-09-16T08:32:26.684Z
Learnt from: twobiers
Repo: crossplane/crossplane PR: 6552
File: cmd/crank/beta/trace/trace.go:223-223
Timestamp: 2025-09-16T08:32:26.684Z
Learning: In cmd/crank/beta/trace/trace.go, when processing resources in the loop, the assignment of the returned root from getResourceTree back to resourceList.Items[i] is not needed according to the PR author twobiers.
Applied to files:
cmd/crank/beta/trace/trace.gocmd/crank/beta/trace/watch.go
📚 Learning: 2025-09-16T08:32:26.684Z
Learnt from: twobiers
Repo: crossplane/crossplane PR: 6552
File: cmd/crank/beta/trace/trace.go:223-223
Timestamp: 2025-09-16T08:32:26.684Z
Learning: In cmd/crank/beta/trace/trace.go, when processing resources in the getResourceTree loop, the assignment of the returned root from getResourceTree back to resourceList.Items[i] is not needed according to the PR author twobiers.
Applied to files:
cmd/crank/beta/trace/trace.gocmd/crank/beta/trace/watch.go
📚 Learning: 2025-12-02T14:41:54.485Z
Learnt from: CR
Repo: crossplane/crossplane PR: 0
File: contributing/README.md:0-0
Timestamp: 2025-12-02T14:41:54.485Z
Learning: Applies to contributing/**/*.go : Follow Crossplane's Observability Developer Guide
Applied to files:
cmd/crank/beta/trace/trace.gocmd/crank/beta/trace/watch.go
🧬 Code graph analysis (2)
cmd/crank/beta/trace/trace.go (1)
internal/engine/engine.go (1)
Watch(432-437)
cmd/crank/beta/trace/watch.go (4)
cmd/crank/beta/trace/trace.go (1)
Cmd(60-74)cmd/crank/beta/trace/internal/printer/printer.go (2)
Printer(45-48)Type(34-34)internal/engine/engine.go (1)
Watch(432-437)cmd/crank/common/resource/client.go (1)
GetResource(35-49)
🔇 Additional comments (9)
cmd/crank/beta/trace/watch.go (6)
41-56: LGTM!Clean message type definitions and model structure. The separation of update, error, and quit messages provides clear control flow for the Bubble Tea program.
58-91: LGTM!The Bubble Tea model implementation is clean and follows the framework's conventions well. I appreciate the thoughtful keyboard handling that allows users to quit with
q,esc, orctrl+c- that's great UX!
93-100: LGTM!Nice helper function that cleanly separates the rendering concern from the watch logic.
146-183: LGTM!The
renderAndSendclosure is well-structured with proper error handling. I like the optimization to only send updates when the rendered output actually changes - that's a nice touch to avoid unnecessary re-renders!
191-219: LGTM!The watch loop has proper exit conditions covering all scenarios: watch channel closure, resource deletion, periodic refresh, and context cancellation. The select statement cleanly handles all these cases.
222-229: LGTM!Good error handling that treats
NotFoundas a graceful exit condition (resource was deleted) while properly wrapping other errors for user visibility.cmd/crank/beta/trace/trace.go (3)
65-73: LGTM!The new
Watchfield is well-defined with a sensible default (false) ensuring backward compatibility. The-wshort flag is intuitive and doesn't conflict with existing flags. Thank you for the clear help text!
108-110: LGTM!Nice addition of the watch example! The help text is consistent with the other examples and clearly shows the intended use case.
152-157: LGTM!Using
client.NewWithWatchis the right approach to get watch capability while maintaining compatibility with all existing operations.
✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.
|
Good call using |
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/crank/beta/trace/trace.go`:
- Around line 244-256: The code prints a watch warning to k.Stdout after calling
p.PrintList in the shouldPrintAsList branch which corrupts machine-readable
output; change the write target to k.Stderr (use k.Stderr.Write instead of
k.Stdout.Write) so the warning goes to stderr and stdout remains clean; keep the
existing behavior/return values and error wrapping (errors.Wrap(err,
errCliOutput)) unchanged; locate this in the block handling shouldPrintAsList
that calls p.PrintList and checks c.Watch.
|
LGTM too, but IIRC we were discussing with @adamwg about merging this to the new CLI house, to avoid having the bubbletea dependency on crossplane, but I'm fine eitherway, as long as we move the cli before the 2.2 release |
adamwg
left a comment
adamwg
left a comment
There was a problem hiding this comment.
One thought on the error handling, otherwise lgtm. I like the simple bubbletea usage.
Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
haarchri
force-pushed
the
feature/trace-watch
branch
from
e792829 to
2488613
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In `@cmd/crank/beta/trace/trace.go`:
- Around line 140-148: The call to pkg.AddToScheme is currently ignoring its
returned error which can cause unpredictable decoding later; update the
initialization after creating cl (client.NewWithWatch) to check the error from
pkg.AddToScheme(cl.Scheme()) and propagate it with context similar to the
existing pattern (wrap and return using errors.Wrap and the existing
errInitKubeClient or a new contextual constant), so that scheme registration
failures are surfaced instead of being discarded.
🧹 Nitpick comments (1)
cmd/crank/beta/trace/trace.go (1)
247-251: Guard watch mode against non-interactive output formats. The watch check only validates that a specific resource is selected (!shouldPrintAsList), but doesn't prevent combining--watchwith non-interactive output formats like-o jsonor-o dot. Using Bubble Tea's interactive UI with json/dot output would produce unusable results. Consider adding a guard to either block the combination or override the output format todefaultwhen watch is enabled.
| cl, err := client.NewWithWatch(kubeconfig, client.Options{ | ||
| Scheme: scheme.Scheme, | ||
| }) | ||
| if err != nil { | ||
| return errors.Wrap(err, errInitKubeClient) | ||
| return nil, nil, nil, errors.Wrap(err, errInitKubeClient) | ||
| } | ||
|
|
||
| // add package scheme | ||
| _ = pkg.AddToScheme(client.Scheme()) | ||
| _ = pkg.AddToScheme(cl.Scheme()) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check AddToScheme implementations and usage patterns
rg -n "func AddToScheme" -g '*.go'Repository: crossplane/crossplane
Length of output: 410
🏁 Script executed:
#!/bin/bash
# Check how AddToScheme is called elsewhere in the codebase
rg -n "AddToScheme\(" -g '*.go' -A 2 -B 1Repository: crossplane/crossplane
Length of output: 6792
🏁 Script executed:
#!/bin/bash
# Look at the actual file context around line 140-148
head -160 cmd/crank/beta/trace/trace.go | tail -40Repository: crossplane/crossplane
Length of output: 1358
Handle pkg.AddToScheme errors instead of ignoring them. The function returns error, and other critical initialization code throughout the codebase (particularly in internal/xpkg/scheme.go and cmd/crossplane/main.go) properly checks these errors. If scheme registration fails, resource decoding will behave unpredictably downstream.
The fix mirrors your existing error handling pattern in this function—would you be able to propagate the error with context?
🛠️ Suggested fix
- // add package scheme
- _ = pkg.AddToScheme(cl.Scheme())
+ // add package scheme
+ if err := pkg.AddToScheme(cl.Scheme()); err != nil {
+ return nil, nil, nil, errors.Wrap(err, errInitKubeClient)
+ }🤖 Prompt for AI Agents
In `@cmd/crank/beta/trace/trace.go` around lines 140 - 148, The call to
pkg.AddToScheme is currently ignoring its returned error which can cause
unpredictable decoding later; update the initialization after creating cl
(client.NewWithWatch) to check the error from pkg.AddToScheme(cl.Scheme()) and
propagate it with context similar to the existing pattern (wrap and return using
errors.Wrap and the existing errInitKubeClient or a new contextual constant), so
that scheme registration failures are surfaced instead of being discarded.
Signed-off-by: Christopher Haar <christopher.haar@upbound.io>
5 participants
Description of your changes
Adds
-w/--watchflag tocrossplane beta tracecommand.Continuously monitors and displays resource status changes until the resource is deleted (makes most sense if using Foreground deletion).
Implements watch loop for given resource with periodically refresh to catch child resource changes.
Fixes #5364
I have:
earthly +reviewableto ensure this PR is ready for review.backport release-x.ylabels to auto-backport this PR.Need help with this checklist? See the cheat sheet.