-
Notifications
You must be signed in to change notification settings - Fork 2
add a sidekick_address param to managed exporter templates #24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
add a sidekick_address param to managed exporter templates #24
Conversation
WalkthroughPasses a rendered ExporterHost through exporter host processing into the exporter instance templater. Adds a new field and setter on ExporterInstanceTemplater to accept the rendered host, and updates processExporterInstance and its call sites to forward the renderedHost (including retry paths) so templates can use sidekick_address from that host. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Caller
participant HostSyncer as ExporterHostSyncer
participant Template as ExporterInstanceTemplater
participant Retry as RetryLogic
Caller->>HostSyncer: processExporterInstance(exporterInstance, hostSsh, renderedHost)
HostSyncer->>Template: New ExporterInstanceTemplater(...)
HostSyncer->>Template: SetRenderedExporterHost(renderedHost)
HostSyncer->>Template: Render template (uses renderedExporterHost to set sidekick_address)
alt render success
Template-->>HostSyncer: rendered instance
else render fails
HostSyncer->>Retry: enqueue retry with RenderedHost
Retry->>HostSyncer: retry trigger
HostSyncer->>HostSyncer: processExporterInstance(..., renderedHost from retry)
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
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 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (2)
internal/exporter/template/exporter_instance.go (2)
69-75: Consider adding a comment explaining the sidekick_address parameter.The purpose and usage of
sidekick_addressisn't immediately clear from the code. Adding a brief comment would improve maintainability.// Add ExporterHost addresses if available (managed devices only) + // sidekick_address provides the exporter host address back to the managed device if exporterInstanceCopy.Spec.ExporterHostRef.Name != "" {
70-70: ExporterHostRef is a value type—no nil check needed
- Add a comment explaining why the first address is chosen for
sidekick_address.- Ensure code passes
make lint,make build, andmake fmt.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
internal/exporter/template/exporter_instance.go(1 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.cursorrules)
**/*.go: Run golangci-lint viamake lintand fix all linting errors before proceeding
Runmake buildto ensure the binary compiles successfully and fix any build errors
Runmake fmtto ensure code formatting (go fmt) before finalizing changes
Files:
internal/exporter/template/exporter_instance.go
🧬 Code graph analysis (1)
internal/exporter/template/exporter_instance.go (1)
api/v1alpha1/exporterinstance_types.go (1)
ExporterHostRef(46-48)
🔇 Additional comments (2)
internal/exporter/template/exporter_instance.go (2)
68-75: Reminder: Run linting and build checks before merging.As per coding guidelines, ensure you run:
make lintto check for linting errorsmake buildto verify the binary compilesmake fmtto format the codeBased on coding guidelines.
68-75: The overall implementation looks solid.The logic correctly handles:
- Optional ExporterHostRef (silently skips when not set)
- Missing or invalid host reference (using
okpattern)- Empty addresses array (defensive length check)
This is appropriate behavior for an optional feature that applies only to managed devices.
81672fc to
a804485
Compare
when we need a reference back to the exporter host address
a804485 to
7c2a54f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actionable comments posted: 0
🧹 Nitpick comments (1)
internal/exporter/template/exporter_instance.go (1)
73-77: Consider documenting the first-address selection.The logic correctly checks for nil and non-empty addresses before setting
sidekick_address. However, consider adding a brief inline comment explaining why the first address (Spec.Addresses[0]) is selected when multiple addresses might exist.Example addition:
// Add ExporterHost addresses if available (managed devices only) if e.renderedExporterHost != nil && len(e.renderedExporterHost.Spec.Addresses) > 0 { + // Use the first address as the primary sidekick address templateParametersMap["sidekick_address"] = e.renderedExporterHost.Spec.Addresses[0] }Note: This parameter will override any manually-set
sidekick_addressin ConfigTemplateRef.Parameters, which is likely intentional for managed devices.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
internal/exporter/host/host.go(4 hunks)internal/exporter/template/exporter_instance.go(3 hunks)
🧰 Additional context used
📓 Path-based instructions (1)
**/*.go
📄 CodeRabbit inference engine (.cursorrules)
**/*.go: Run golangci-lint viamake lintand fix all linting errors before proceeding
Runmake buildto ensure the binary compiles successfully and fix any build errors
Runmake fmtto ensure code formatting (go fmt) before finalizing changes
Files:
internal/exporter/template/exporter_instance.gointernal/exporter/host/host.go
🧬 Code graph analysis (2)
internal/exporter/template/exporter_instance.go (1)
api/v1alpha1/exporterhost_types.go (1)
ExporterHost(102-108)
internal/exporter/host/host.go (3)
api/v1alpha1/exporterinstance_types.go (1)
ExporterInstance(71-77)internal/exporter/ssh/ssh.go (1)
HostManager(37-46)api/v1alpha1/exporterhost_types.go (1)
ExporterHost(102-108)
🔇 Additional comments (7)
internal/exporter/template/exporter_instance.go (2)
20-20: LGTM! New field properly typed.The
renderedExporterHostfield is correctly typed as a pointer to support nil values when not dealing with managed devices.
39-41: LGTM! Setter follows established patterns.The
SetRenderedExporterHostmethod follows the same pattern asSetServiceParametersand appropriately accepts nil values.internal/exporter/host/host.go (5)
132-132: LGTM! Signature correctly extended.The
processExporterInstancemethod signature properly adds therenderedHostparameter to enable template access to host information.
152-152: LGTM! Rendered host correctly propagated to templater.The rendered host is set on the templater after service parameters, ensuring templates have access to the host context during rendering.
245-245: LGTM! Initial processing correctly passes rendered host.The call site in the initial processing loop properly passes the
renderedHostparameter.
347-347: LGTM! Retry logic maintains host context.The retry logic correctly uses
retryItem.RenderedHostto ensure retries operate with the same host context as the initial attempt.
1-433: Verify Go formatting, linting, and buildExecute
make fmt,make lint, andmake build(or locallygo fmt ./…,golangci-lint run ./…,go build ./…) and resolve any errors before merging.
|
too ugly |
when we need a reference back to the exporter host address
Summary by CodeRabbit
New Features
Behavior