feat: sync nodes and add --selector arg#61
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughExtracts Kubernetes resource collection into a centralized sync processor ( Changes
Sequence DiagramsequenceDiagram
participant CLI as CLI Command
participant Sync as Sync Processor
participant API as Cluster API
participant Client as Kubernetes Clientset
CLI->>Sync: newSync(clusterIdentifier, workspaceId, ctrlplaneClient, config, clusterName)
CLI->>Sync: process(ctx, selectors)
Sync->>API: Get cluster resource
API-->>Sync: cluster metadata / response
Sync->>Client: Create clientset from kubeConfig
alt selectors.ShouldFetch(namespaces)
Sync->>Client: List Namespaces
Client-->>Sync: Namespace list
Sync->>Sync: processNamespace() per namespace
end
alt selectors.ShouldFetch(deployments)
Sync->>Client: List Deployments (all namespaces)
Client-->>Sync: Deployment list
Sync->>Sync: processDeployment() per deployment
end
alt selectors.ShouldFetch(nodes)
Sync->>Client: List Nodes
Client-->>Sync: Node list
Sync->>Sync: processNode() per node
end
Sync->>Sync: Merge cluster metadata into resources
Sync-->>CLI: []ResourceProviderResource
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
📝 Coding Plan
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}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (3)
cmd/ctrlc/root/sync/kubernetes/resource_types.go (1)
52-55: Stale usage comment inSetcan mislead.The comment still references
--resourceand singular forms; the flag is--selectorand values are plural.🧹 Suggested comment update
func (r *ResourceTypes) Set(value string) error { // supports repeated flags like: - // --resource namespace --resource node + // --selector namespaces --selector nodes rt, err := ParseResourceType(value)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ctrlc/root/sync/kubernetes/resource_types.go` around lines 52 - 55, The comment above ResourceTypes.Set is stale: update it to reference the actual flag name and expected value form (replace references to "--resource" and singular forms with "--selector" and plural values), e.g. explain that the setter supports repeated --selector flags and plural resource type values parsed by ParseResourceType; keep the note concise and colocated with the Set(value string) method.cmd/ctrlc/root/sync/kubernetes/kubernetes.go (1)
28-29: Usecmd.Context()for upsert as well (avoid detached background context).Right now only sync processing is cancelable; upsert is not.
♻️ Suggested fix
-import ( - "context" - "fmt" +import ( + "fmt" @@ - ctx := context.Background() log.Info("Syncing Kubernetes resources on a cluster") @@ - return ctrlp.UpsertResources(ctx, resources, &providerName) + return ctrlp.UpsertResources(cmd.Context(), resources, &providerName)Also applies to: 59-59
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ctrlc/root/sync/kubernetes/kubernetes.go` around lines 28 - 29, Replace the detached context usage with the command's cancellable context: change the ctx variable initialization from context.Background() to cmd.Context(), and ensure the same ctx is passed into the Kubernetes resource upsert call (the function/method that performs the upsert) so both sync processing and the upsert use the cancellable cmd.Context() instead of a background context.cmd/ctrlc/root/sync/kubernetes/sync_processor.go (1)
26-27: Use the passedctxconsistently instead ofTODO/Background.Current calls bypass cancellation/deadlines from the command context.
♻️ Suggested fix
func (s *syncConfig) FetchNodes(ctx context.Context, clientset *kubernetes.Clientset) error { - nodes, err := clientset.CoreV1().Nodes().List(context.TODO(), metav1.ListOptions{}) + nodes, err := clientset.CoreV1().Nodes().List(ctx, metav1.ListOptions{}) if err != nil { return err } @@ for _, namespace := range namespaces.Items { - resource := processNamespace(context.Background(), s.clusterName, namespace) + resource := processNamespace(ctx, s.clusterName, namespace) resources = append(resources, resource) } @@ for _, deployment := range deployments.Items { - resource := processDeployment(context.Background(), s.clusterName, deployment) + resource := processDeployment(ctx, s.clusterName, deployment) resources = append(resources, resource) }Also applies to: 48-49, 63-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ctrlc/root/sync/kubernetes/sync_processor.go` around lines 26 - 27, The code is using context.TODO() instead of the passed ctx causing cancellations/deadlines to be ignored; update all Kubernetes APIList/List/watch calls to use the incoming ctx parameter (e.g., in FetchNodes and the other fetch methods in this file) instead of context.TODO()/context.Background() so they honor the caller's context; search for occurrences of context.TODO() or context.Background() in sync_processor.go and replace them with the function's ctx variable (e.g., the ctx parameter of FetchNodes/other fetch functions) ensuring consistent context propagation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/ctrlc/root/sync/kubernetes/resource_types.go`:
- Around line 12-25: ParseResourceType currently accepts ResourceService but the
sync processor only implements namespaces, deployments and nodes; update
ParseResourceType to reject services until sync logic exists by removing
ResourceService from the allowed switch (or explicitly return an error when s ==
string(ResourceService)). Modify the ParseResourceType function so only
ResourceNamespace, ResourceNode and ResourceDeployment are accepted and return a
clear error referencing ResourceService when "services" is passed, so callers
cannot select an unsupported ResourceType.
In `@cmd/ctrlc/root/sync/kubernetes/sync_processor.go`:
- Around line 196-205: The fetch error handling in the conditional blocks using
selectors.ShouldFetch(ResourceDeployment)/ResourceNode swallows errors by
returning s.resources, nil; change these to return s.resources, err so fetch
failures propagate (mirror the namespace path behavior). Update the
FetchDeployments and FetchNodes call sites in sync_processor.go to return the
encountered err instead of nil when the call fails, ensuring callers receive the
error.
- Around line 172-179: The call to clusterResource.StatusCode() can panic when
s.client.GetResourceByIdentifierWithResponse returns (nil, err); add a nil guard
for clusterResource immediately after the GetResourceByIdentifierWithResponse
call (check clusterResource == nil) and handle/return the error (or log and
return) before calling clusterResource.StatusCode(); update the block around
s.client.GetResourceByIdentifierWithResponse, clusterResource, and the
StatusCode() check (referencing s.client.GetResourceByIdentifierWithResponse,
clusterResource, StatusCode(), s.workspaceID, s.clusterIdentifier) so execution
stops and a proper error is returned when clusterResource is nil.
---
Nitpick comments:
In `@cmd/ctrlc/root/sync/kubernetes/kubernetes.go`:
- Around line 28-29: Replace the detached context usage with the command's
cancellable context: change the ctx variable initialization from
context.Background() to cmd.Context(), and ensure the same ctx is passed into
the Kubernetes resource upsert call (the function/method that performs the
upsert) so both sync processing and the upsert use the cancellable cmd.Context()
instead of a background context.
In `@cmd/ctrlc/root/sync/kubernetes/resource_types.go`:
- Around line 52-55: The comment above ResourceTypes.Set is stale: update it to
reference the actual flag name and expected value form (replace references to
"--resource" and singular forms with "--selector" and plural values), e.g.
explain that the setter supports repeated --selector flags and plural resource
type values parsed by ParseResourceType; keep the note concise and colocated
with the Set(value string) method.
In `@cmd/ctrlc/root/sync/kubernetes/sync_processor.go`:
- Around line 26-27: The code is using context.TODO() instead of the passed ctx
causing cancellations/deadlines to be ignored; update all Kubernetes
APIList/List/watch calls to use the incoming ctx parameter (e.g., in FetchNodes
and the other fetch methods in this file) instead of
context.TODO()/context.Background() so they honor the caller's context; search
for occurrences of context.TODO() or context.Background() in sync_processor.go
and replace them with the function's ctx variable (e.g., the ctx parameter of
FetchNodes/other fetch functions) ensuring consistent context propagation.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: bb713093-fd04-41d5-9c57-075050450571
📒 Files selected for processing (3)
cmd/ctrlc/root/sync/kubernetes/kubernetes.gocmd/ctrlc/root/sync/kubernetes/resource_types.gocmd/ctrlc/root/sync/kubernetes/sync_processor.go
* adds repeatable selector arg or short -s if left blank all resources are synced fix: remove unused resource type fix: not returning errors fix: nil check
mleonidas
force-pushed
the
mleonidas/add-node-sync-and-selector-arg
branch
from
df65769 to
46d2160
Compare
* adding a comment directive to ignore this until I get context on if this is actually needed for future work
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
cmd/ctrlc/root/sync/kubernetes/resource_types.go (1)
52-53: Update the inline example to the new flag/value format.These comments still show
--resource namespace --resource node, but the CLI now exposes--selectorincmd/ctrlc/root/sync/kubernetes/kubernetes.go, Line 65, andParseResourceTypehere only accepts plural values. Keeping the example current will avoid old syntax leaking back into docs or completions.📝 Proposed fix
- // supports repeated flags like: - // --resource namespace --resource node + // supports repeated flags like: + // --selector namespaces --selector nodes🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ctrlc/root/sync/kubernetes/resource_types.go` around lines 52 - 53, Update the inline comment in resource_types.go that currently shows the old repeated-flag example ("--resource namespace --resource node") to reflect the new flag/value format used by the CLI: use the --selector flag and plural resource names that ParseResourceType accepts (e.g., examples showing --selector "resource=namespaces" or the appropriate selector syntax and plural values like "namespaces,nodes"); change only the comment text so it matches the selector-based usage shown in cmd/ctrlc/root/sync/kubernetes/kubernetes.go and the ParseResourceType expectations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@cmd/ctrlc/root/sync/kubernetes/kubernetes.go`:
- Line 65: The help text currently promises that passing a blank value will sync
all resources but ParseResourceType (used in resource_types.go) rejects empty
strings; either change the help text in the flag registration (the
cmd.Flags().VarP call that registers selectors in kubernetes.go) to accurately
say "omit --selector to sync all resources" or alter ParseResourceType to treat
an empty string as meaning "all" (no error) so "--selector \"\""/empty input is
accepted; update the code paths that iterate over selectors to handle the
empty/all case accordingly and ensure selectors variable parsing remains
consistent.
- Around line 53-55: The sync currently fetches using cmd.Context() via
newSync/process but then calls ctrlp.UpsertResources with a background context,
so cancellation (Ctrl-C/deadline) won't stop the write phase; change the code to
create a single cancellable context (use ctx := cmd.Context() or accept/pass ctx
into newSync and sync.process) and pass that same ctx into ctrlp.UpsertResources
instead of the background context so the entire sync (discovery + upsert) honors
cancellation; update newSync/signature or call sites as needed to propagate ctx
consistently.
---
Nitpick comments:
In `@cmd/ctrlc/root/sync/kubernetes/resource_types.go`:
- Around line 52-53: Update the inline comment in resource_types.go that
currently shows the old repeated-flag example ("--resource namespace --resource
node") to reflect the new flag/value format used by the CLI: use the --selector
flag and plural resource names that ParseResourceType accepts (e.g., examples
showing --selector "resource=namespaces" or the appropriate selector syntax and
plural values like "namespaces,nodes"); change only the comment text so it
matches the selector-based usage shown in
cmd/ctrlc/root/sync/kubernetes/kubernetes.go and the ParseResourceType
expectations.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 2f034762-3798-46de-91bb-fd9f7dcb4dae
📒 Files selected for processing (4)
cmd/ctrlc/root/sync/kubernetes/kubernetes.gocmd/ctrlc/root/sync/kubernetes/resource_types.gocmd/ctrlc/root/sync/kubernetes/sync_processor.gocmd/ctrlc/root/sync/netbox/devices/types.go
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/ctrlc/root/sync/kubernetes/sync_processor.go
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
cmd/ctrlc/root/sync/kubernetes/kubernetes.go (1)
53-54:⚠️ Potential issue | 🟠 MajorUse one cancellable context for both discovery and upsert.
Line 54 uses
cmd.Context()for fetch, but Line 59 still uses a background-derivedctx. This can allow the write phase to continue after Ctrl-C/deadline.♻️ Proposed fix
import ( - "context" "fmt" @@ - ctx := context.Background() + ctx := cmd.Context() @@ - resources, err := sync.process(cmd.Context(), selectors) + resources, err := sync.process(ctx, selectors) @@ return ctrlp.UpsertResources(ctx, resources, &providerName)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/ctrlc/root/sync/kubernetes/kubernetes.go` around lines 53 - 54, The discovery and upsert phases are using different contexts (cmd.Context() for discovery via sync.process and a background-derived ctx for the upsert), which lets the write phase ignore Ctrl-C/deadline; change the code to create a single cancellable context from cmd.Context() (e.g., ctx, cancel := context.WithCancel(cmd.Context()) with defer cancel()) and pass that same ctx into newSync/sync.process and into the upsert/write calls (replace any background-derived ctx). Ensure all references to the old background ctx are updated to this single ctx so cancelation from cmd.Context() applies to both discovery and upsert.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@cmd/ctrlc/root/sync/kubernetes/kubernetes.go`:
- Around line 53-54: The discovery and upsert phases are using different
contexts (cmd.Context() for discovery via sync.process and a background-derived
ctx for the upsert), which lets the write phase ignore Ctrl-C/deadline; change
the code to create a single cancellable context from cmd.Context() (e.g., ctx,
cancel := context.WithCancel(cmd.Context()) with defer cancel()) and pass that
same ctx into newSync/sync.process and into the upsert/write calls (replace any
background-derived ctx). Ensure all references to the old background ctx are
updated to this single ctx so cancelation from cmd.Context() applies to both
discovery and upsert.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4273ce90-6b97-4b20-820b-b4db6ec363d6
📒 Files selected for processing (1)
cmd/ctrlc/root/sync/kubernetes/kubernetes.go
jsbroks
approved these changes
Mar 18, 2026
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.
Summary by CodeRabbit
New Features
Improvements
Chores