Sync instance clients, with diff on dryRun and prunecontrol #1
Conversation
WalkthroughThe changes introduce dry-run support and enhanced resource synchronization for Jumpstarter instances and their clients. The control flow in the application logic is refactored for clarity, with new methods for listing, retrieving, and syncing clients, as well as printing diffs for resource updates. Tests are updated to accommodate new constructor parameters and to cover the new diff-printing functionality. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant ApplyCmd
participant Instance
participant K8sAPI
User->>ApplyCmd: Run apply command (with or without --dry-run)
ApplyCmd->>Instance: Create Instance (dryRun, prune flags)
alt dry-run mode
ApplyCmd->>User: Print dry-run placeholder message
else normal mode
ApplyCmd->>Instance: Apply templates, create resources
Instance->>K8sAPI: Create/Update/Delete Exporters/Clients
Instance->>Instance: SyncClients (compare, create, update, delete clients)
ApplyCmd->>User: Output results/errors
end
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 golangci-lint (1.64.8)Error: you are using a configuration file for golangci-lint v2 with golangci-lint v1: please use golangci-lint v2 ✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (4)
cmd/apply.go (2)
67-84: Remove or complete the orphaned comment.The comment at line 79 appears to be incomplete or misplaced. It should either be removed or expanded to explain what's happening.
err = instanceClient.SyncClients(context.Background(), cfg) - // list all exporters in the instance - if err != nil {
86-86: Consider consistent section formatting.The exporter hosts section uses "===" as a separator while the instances section above doesn't. Consider either adding a similar separator for instances or removing this one for consistency.
internal/instance/instance.go (2)
20-22: Consider using the full module name for the managed-by annotation.The
managedByAnnotationconstant uses a shortened name. Consider using the full module path for better clarity and to avoid potential conflicts.const ( - managedByAnnotation = "jumpstarter-lab-config" + managedByAnnotation = "github.com/jumpstarter-dev/jumpstarter-lab-config" )
364-383: Consider collecting errors instead of failing fast.The current implementation stops on the first error. Consider collecting all errors and returning them together for better visibility into multiple issues.
Example approach:
var errs []error // In each loop: if err != nil { errs = append(errs, fmt.Errorf("failed to create client %s: %w", cfgClient.Name, err)) continue } // After all operations: if len(errs) > 0 { return fmt.Errorf("sync completed with %d errors: %v", len(errs), errs) }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
cmd/apply.go(1 hunks)internal/instance/instance.go(11 hunks)internal/instance/instance_test.go(15 hunks)
🔇 Additional comments (5)
cmd/apply.go (1)
59-65: Good refactoring of dry-run logic.The simplification makes the code cleaner by delegating dry-run behavior to the instance methods.
internal/instance/instance_test.go (2)
26-26: Test updates correctly match the new constructor signature.All NewInstance calls have been properly updated to include the new dryRun and prune parameters.
Also applies to: 35-35, 58-58, 83-83, 108-108, 141-141, 162-162, 194-194, 218-218, 240-240, 272-272
305-360: Good test coverage for the new printDiff method.The test properly covers both scenarios - when objects differ and when they're identical.
internal/instance/instance.go (2)
289-308: Well-designed metadata preparation helper.The method properly handles nil maps, merges annotations correctly, and ensures required fields are set.
322-337: Excellent diff implementation for Kubernetes resources.The method correctly ignores volatile Kubernetes metadata fields and provides clear output for dry-run mode.
| // | ||
| //nolint:unused | ||
| func (i *Instance) createExporter(ctx context.Context, exporter *v1alpha1.Exporter) error { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify if these exporter methods are used.
Both createExporter and deleteExporter have //nolint:unused directives. If these are not used, consider removing them.
Also applies to: 187-189
🏁 Script executed:
#!/bin/bash
# Description: Check if createExporter and deleteExporter methods are used
# Search for calls to these methods
rg "(createExporter|deleteExporter)\(" --type go -A 2Length of output: 593
Unused exporter methods detected – remove or implement
Both createExporter (lines 173–175) and deleteExporter (lines 187–189) aren’t referenced anywhere in the repo. You can either:
- Remove these methods entirely if they’re no longer needed
- Or remove the
//nolint:unuseddirectives and wire them up where intended
• File: internal/instance/instance.go
– Lines 173–175 (createExporter)
– Lines 187–189 (deleteExporter)
🤖 Prompt for AI Agents
In internal/instance/instance.go around lines 173 to 175 and 187 to 189, the
methods createExporter and deleteExporter are unused despite having
nolint:unused directives. Review whether these methods are needed; if not,
remove them entirely. If they are intended for future use, remove the nolint
directives and integrate these methods properly into the codebase where they
should be called.
| // | ||
| //nolint:unused | ||
| func (i *Instance) updateExporter(ctx context.Context, oldExporter, exporter *v1alpha1.Exporter) error { |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify if this method is actually used.
The //nolint:unused directive suggests this method might be dead code. If it's not used, consider removing it to reduce maintenance burden.
🏁 Script executed:
#!/bin/bash
# Description: Check if updateExporter method is used anywhere in the codebase
# Search for calls to updateExporter
rg "updateExporter\(" --type go -A 2Length of output: 307
Remove unused updateExporter method
A codebase-wide search for updateExporter( returned only its definition in internal/instance/instance.go (lines 151–153) and no invocations elsewhere. This indicates the method is dead code. To reduce maintenance burden:
- Remove the entire
updateExportermethod (lines 151–170) frominternal/instance/instance.go - Delete the
//nolint:unuseddirective associated with it
🤖 Prompt for AI Agents
In internal/instance/instance.go from lines 151 to 170, the updateExporter
method is defined but never called anywhere in the codebase, making it dead
code. Remove the entire updateExporter method including its signature and body,
and also delete the associated //nolint:unused directive above it to clean up
unused code and reduce maintenance overhead.
| // delete clients that are not in config | ||
| for _, instanceClient := range instanceClients.Items { | ||
| if _, ok := configClientMap[instanceClient.Name]; !ok { | ||
| err := i.deleteClient(ctx, instanceClient.Name) | ||
| if err != nil { | ||
| return fmt.Errorf("[%s] failed to delete client %s: %w", i.config.Name, instanceClient.Name, err) | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Add prune flag check before deleting clients.
The deletion logic doesn't respect the prune flag. Clients should only be deleted when prune is true.
// delete clients that are not in config
for _, instanceClient := range instanceClients.Items {
if _, ok := configClientMap[instanceClient.Name]; !ok {
+ if !i.prune {
+ fmt.Printf("⚠️ [%s] Client %s exists in cluster but not in config (use --prune to delete)\n", i.config.Name, instanceClient.Name)
+ continue
+ }
err := i.deleteClient(ctx, instanceClient.Name)
if err != nil {
return fmt.Errorf("[%s] failed to delete client %s: %w", i.config.Name, instanceClient.Name, err)
}
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // delete clients that are not in config | |
| for _, instanceClient := range instanceClients.Items { | |
| if _, ok := configClientMap[instanceClient.Name]; !ok { | |
| err := i.deleteClient(ctx, instanceClient.Name) | |
| if err != nil { | |
| return fmt.Errorf("[%s] failed to delete client %s: %w", i.config.Name, instanceClient.Name, err) | |
| } | |
| } | |
| } | |
| // delete clients that are not in config | |
| for _, instanceClient := range instanceClients.Items { | |
| if _, ok := configClientMap[instanceClient.Name]; !ok { | |
| if !i.prune { | |
| fmt.Printf("⚠️ [%s] Client %s exists in cluster but not in config (use --prune to delete)\n", i.config.Name, instanceClient.Name) | |
| continue | |
| } | |
| err := i.deleteClient(ctx, instanceClient.Name) | |
| if err != nil { | |
| return fmt.Errorf("[%s] failed to delete client %s: %w", i.config.Name, instanceClient.Name, err) | |
| } | |
| } | |
| } |
🤖 Prompt for AI Agents
In internal/instance/instance.go around lines 354 to 362, the code deletes
clients without checking the prune flag. Modify the loop to first check if the
prune flag is true before attempting to delete any clients. Only proceed with
deletion when prune is enabled to respect the intended behavior.
| // Prepare metadata (annotations, namespace, etc.) | ||
| // For updates, we want to preserve existing annotations and merge new ones | ||
| i.prepareMetadata(&updatedClient.ObjectMeta, clientObj.Annotations) | ||
| i.printDiff(oldClientObj, updatedClient, "client", updatedClient.Name) |
There was a problem hiding this comment.
Fix inconsistent printDiff call - type mismatch.
The printDiff call passes the full oldClientObj as the first argument, but in updateExporter (line 164), only TypeMeta is passed. This inconsistency could cause runtime issues.
- i.printDiff(oldClientObj, updatedClient, "client", updatedClient.Name)
+ i.printDiff(oldClientObj.TypeMeta, updatedClient, "client", updatedClient.Name)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| i.printDiff(oldClientObj, updatedClient, "client", updatedClient.Name) | |
| i.printDiff(oldClientObj.TypeMeta, updatedClient, "client", updatedClient.Name) |
🤖 Prompt for AI Agents
In internal/instance/instance.go at line 247, the call to printDiff passes the
entire oldClientObj, whereas in updateExporter at line 164 only the TypeMeta
part is passed, causing a type mismatch. Modify the printDiff call here to pass
only the TypeMeta portion of oldClientObj to ensure consistency and prevent
runtime errors.
Summary by CodeRabbit
New Features
Bug Fixes
Tests