Conversation
❌ Deploy Preview for testkube-docs-preview failed.
|
exu
requested review from
dejanzele,
nicufk,
povilasv,
vLia and
vsukhin
as code owners
|
@greptile |
Greptile SummaryThis PR successfully namespaces all agent-side NATS event subjects under an Key changes:
All NATS topic strings in publishers and subscribers have been updated consistently. Two minor issues remain: (1) one hardcoded literal in Confidence Score: 4/5
Sequence DiagramsequenceDiagram
participant Agent
participant NATS
participant ControlPlane
Note over Agent,NATS: Before this PR — both Agent and CP used "events.*" prefix
Agent->>NATS: publish events.test.start
ControlPlane->>NATS: subscribe events.>
NATS-->>ControlPlane: (unintended delivery — event ping-pong)
Note over Agent,NATS: After this PR — Agent uses "agent.*" prefix
Agent->>NATS: publish agent.test.start
Agent->>NATS: publish agent.logs.start
Agent->>NATS: publish agent.executor.create
NATS-->>Agent: deliver agent.> (Emitter / LogsService)
Note over ControlPlane,NATS: CP still subscribes its own "events.*" namespace
ControlPlane->>NATS: subscribe events.>
Note right of ControlPlane: No cross-contamination
Last reviewed commit: a61016e |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
|
|
||
| func (s *executorsService) SubscribeList(ctx context.Context, selector string) (<-chan []testkube.ExecutorDetails, error) { | ||
| return HandleSubscription(ctx, "events.executor.>", s, func() ([]testkube.ExecutorDetails, error) { | ||
| return HandleSubscription(ctx, "agent.executor.>", s, func() ([]testkube.ExecutorDetails, error) { |
There was a problem hiding this comment.
The topic string "agent.executor.>" is hardcoded, but the testkube package is already imported and now exports a Subscription constant with value "agent". Using the constant prevents future prefix drift if the value ever changes.
| return HandleSubscription(ctx, "agent.executor.>", s, func() ([]testkube.ExecutorDetails, error) { | |
| return HandleSubscription(ctx, testkube.Subscription+".executor.>", s, func() ([]testkube.ExecutorDetails, error) { |
| func (n *NATSBus) TraceEvents() { | ||
| s, err := n.nc.Subscribe(SubscriptionName+".>", func(event testkube.Event) { | ||
| log.Tracew(log.DefaultLogger, "all events.> trace", event.Log()...) | ||
| log.Tracew(log.DefaultLogger, "got message on events", event.Log()...) |
There was a problem hiding this comment.
The subscription subject was renamed from events.> to agent.> (via SubscriptionName), but the log message still says "got message on events". This is confusing for operators reading trace logs who aren't aware of the rename.
| log.Tracew(log.DefaultLogger, "got message on events", event.Log()...) | |
| log.Tracew(log.DefaultLogger, "got message on agent", event.Log()...) |
mgazza
left a comment
mgazza
left a comment
There was a problem hiding this comment.
A couple more suggestions beyond what Greptile flagged.
| TestStartSubject = "events.test.start" | ||
| TestStopSubject = "events.test.stop" | ||
| Subscription = "agent" | ||
| TestStartSubject = Subscription + ".test.start" |
There was a problem hiding this comment.
The "agent" prefix is now defined in three places: here, pkg/logs/client/interface.go, and pkg/event/bus/nats.go (SubscriptionName). If any of them drifts, topics silently stop matching.
Consider importing from a single source of truth. If circular imports are a concern, a small shared package (e.g. pkg/event/constants) would work.
|
|
||
| if e.ResourceId == "" { | ||
| return "events." + string(*e.Resource) | ||
| return Subscription + "." + string(*e.Resource) |
There was a problem hiding this comment.
Runtime string concatenation with + across multiple segments is harder to scan than strings.Join. The compile-time const expressions (like Subscription + ".test.start") are fine, but these runtime ones would benefit:
| return Subscription + "." + string(*e.Resource) | |
| return strings.Join([]string{Subscription, string(*e.Resource), e.ResourceId}, ".") |
4 participants
Pull request description
Control Plane and Agent are on same
eventsprefix - some edge cases (like recent invalid NATS selector settings caused strange events ping pongs. to allow even use single NATS for On prem setup I've changed prefix toagentin agentChecklist (choose whats happened)
Breaking changes
Changes
Fixes