Import commands: copy, du, edit, meta, rotate-ca, support, wipe #44
Conversation
WalkthroughThis pull request introduces a series of new commands to enhance the Talos import functionality. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant CLI
participant CopyCommand
participant Client
participant Node
User->>CLI: Execute "copy <src-path> <local-path>"
CLI->>CopyCommand: Parse and validate arguments
CopyCommand->>Client: Prepare copy request
Client->>Node: Request .tar.gz archive of <src-path>
Node-->>Client: Return archive stream
Client-->>CopyCommand: Relay data stream/status
CopyCommand->>CLI: Output result or error
sequenceDiagram
participant User
participant CLI
participant RotateCACmd
participant Client
participant Cluster
participant CAHandler
User->>CLI: Execute "rotate-ca"
CLI->>RotateCACmd: Initialize command and process flags
RotateCACmd->>Client: Initialize cluster state and retrieve current CA info
Client->>Cluster: Request cluster CA info
Cluster-->>Client: Provide CA details
Client->>CAHandler: Invoke CA rotation (Talos/Kubernetes)
CAHandler-->>Client: Return new CA bundle and status
Client-->>RotateCACmd: Confirm rotation result
RotateCACmd->>CLI: Output summary of CA rotation
Poem
✨ 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:
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: 8
🧹 Nitpick comments (12)
pkg/commands/imported_copy.go (1)
87-106: Broken formatting in PreRunE functionThe formatting in the PreRunE function is inconsistent with unnecessary line breaks that make the code harder to read. This should be cleaned up for better readability.
- cpCmd.PreRunE = func(cmd *cobra.Command, args []string) error { - nodesFromArgs := - len(GlobalArgs. - Nodes, - ) > 0 - endpointsFromArgs := len(GlobalArgs. - Endpoints) > 0 + cpCmd.PreRunE = func(cmd *cobra.Command, args []string) error { + nodesFromArgs := len(GlobalArgs.Nodes) > 0 + endpointsFromArgs := len(GlobalArgs.Endpoints) > 0pkg/commands/imported_edit.go (4)
36-42: Consider grouping related flags or using a dedicated struct for maintainability.
editCmdFlagsis straightforward, but as the command grows, the struct could become crowded. If new flags are introduced in the future, it might be helpful to split them into smaller, logically grouped structs or apply a more descriptive naming scheme to enhance clarity.
56-63: Validate resource type checks early.Currently, the code checks whether
mc.Metadata().Type()equalsconfig.MachineConfigTypeonly inside theeditFn. If future expansions allow editing other resource types, consider gracefully handling them, or add a pre-check so users get immediate feedback before entering the editing cycle.
112-112: Handle theos.Remove(path)error.Here, the
os.Remove(path)call is wrapped in anolint:errcheck. While ignoring this error might be harmless in many cases, it's often safer to log or handle it (e.g., in case the file is locked or removed by another process). This avoids silent failures.
210-238: Add clarity on config file processing inPreRunE.The logic that processes
configFilesto update global arguments is important but slightly hidden. Documenting or naming these steps more clearly can help convey the difference between standard command flags and these user-supplied file-based patches. This can reduce confusion for new contributors.pkg/commands/imported_rotate-ca.go (3)
27-37: Validate flags cohesively inrotateCACmdFlags.
rotateCACmdFlagsincludes various fields (e.g.,clusterState,forceEndpoint, etc.). If the user provides conflicting or incomplete flags (e.g., specifyingforceEndpointbut not other necessary fields), it might lead to unexpected behavior. Consider adding an early validation step or usage help.
139-147: Highlight dry-run behavior.Printing the dry-run warning to stdout is helpful, but consider offering a more visible output, e.g., color-coded or directed to stderr. This ensures users don’t accidentally miss that the rotation was not actually applied.
209-210: Confirm default flags for rotating CAs.Both
--talosand--kubernetesflags default to true, meaning both rotations happen by default. This is convenient, but users might be surprised if they intended to rotate only one. A quick usage note or clarifying log might help reduce confusion.pkg/commands/imported_support.go (4)
10-25: Consider optimizing or pruning imports.A broad range of packages is imported, including concurrency, color, table writing, etc. While they appear relevant to your support bundle logic, review whether all are strictly necessary to keep the codebase lean (or consider grouping them if any static analysis tool flags them).
38-43: ValidatesupportCmdFlagson initialization.
output,numWorkers,verbose, andconfigFilescan lead to conflicting usage if a user sets inappropriate combinations (e.g., zero workers). Consider adding immediate validation or bounding checks.
199-230: Provide user confirmation or context before overwriting existing files.The prompt logic is sound, but if the user is running in a scripted environment (with no STDIN), they might unexpectedly fail. Consider a
--force-overwriteflag for automation, or a fallback approach if STDIN is unavailable.
284-340: Enhance or customize the progress UI.The
uiprogressbar is practical, but for large sets of nodes, bars can quickly scroll off screen. You might consider grouping them or summarizing partial progress in a single bar. This can improve readability during long support collection runs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
Makefile(1 hunks)go.mod(13 hunks)pkg/commands/imported_copy.go(1 hunks)pkg/commands/imported_diskusage.go(1 hunks)pkg/commands/imported_edit.go(1 hunks)pkg/commands/imported_image.go(2 hunks)pkg/commands/imported_meta.go(1 hunks)pkg/commands/imported_rotate-ca.go(1 hunks)pkg/commands/imported_support.go(1 hunks)pkg/commands/imported_wipe.go(1 hunks)tools/import_commands.go(7 hunks)
🧰 Additional context used
🧬 Code Definitions (3)
pkg/commands/imported_copy.go (1)
pkg/commands/root.go (2)
WithClient(71-90)GlobalArgs(24-24)
pkg/commands/imported_meta.go (1)
pkg/commands/root.go (3)
WithClientMaintenance(93-95)WithClient(71-90)GlobalArgs(24-24)
pkg/commands/imported_rotate-ca.go (2)
pkg/commands/root.go (2)
WithClient(71-90)GlobalArgs(24-24)pkg/engine/engine.go (1)
Options(39-57)
🔇 Additional comments (29)
go.mod (2)
72-72: Dependencies are now used directly in the codebaseThe PR upgrades
github.com/fatih/colorandgithub.com/siderolabs/go-talos-supportfrom indirect to direct dependencies and addsgithub.com/gosuri/uiprogressandk8s.io/kubectlas new direct dependencies. These are likely required by the newly introduced commands.Also applies to: 148-148, 192-192, 196-196
207-381: New indirect dependencies support the added commandsThese are transitive dependencies pulled in by the direct dependencies. Several Kubernetes-related packages, UI libraries, and utility packages are now included to support the new functionality.
Makefile (1)
40-47: New commands added to import-commands targetThe PR extends the functionality with eight new commands:
copy,meta,edit,rollback,rotate-ca,support,wipe, anddiskusage. This aligns with the PR title and expands the tool's capabilities.pkg/commands/imported_image.go (2)
246-248: Cosmetic code formatting improvementThe reformatting of the
StringSliceVarPmethod call is a style improvement for better readability. The functionality remains unchanged.
257-258: Consistent code formattingThis change is purely cosmetic, reformatting the assignment of
PreRunEfor consistency with the codebase style.pkg/commands/imported_diskusage.go (4)
32-125: Well-implemented disk usage commandThe new
duCmdprovides disk usage information similar to Unixducommand. It includes:
- Multiple path support
- Human-readable size output option
- Threshold filtering
- Recursion depth control
- Tab-formatted output with headers
- Path completion for shell
The implementation handles errors appropriately and follows the project's command pattern.
127-146: Config file processing matches other commandsThe command includes support for configuration files with proper validation. The pre-run function correctly processes modelines and updates global arguments, maintaining consistency with other commands in the codebase.
148-152: Comprehensive command-line optionsThe command provides a good set of flags that match the Unix
ducommand's functionality while adding Talos-specific enhancements. The flags are properly documented with clear descriptions.
155-157: Clean variable structure for config filesUsing a dedicated struct for command flags is a good practice for organization and maintainability.
tools/import_commands.go (6)
21-30: Well-implemented string transformation utilityThe
toCamelCasefunction is a clean implementation for transforming hyphenated strings into camel case with the option to export (capitalize) parts of the string beyond the first part. This will help standardize command name formatting.
145-153: Solid command name mapping implementationThese mappings provide good aliases for the commands, making them more familiar to users (e.g., "diskusage" → "du", "copy" → "cp") and ensuring proper capitalization for acronyms like "CA" in "rotateCA".
155-156: Good variable extraction for better maintainabilityCreating the
flagsVarimproves code maintainability by avoiding hardcoding the command flags variable name. This makes it easier to extend the functionality to new commands.
191-206: Consistent implementation of subcommand configurationThe pattern for configuring subcommands is applied consistently across "meta", "conformance", and "wipe" commands, ensuring a uniform approach to flag and PreRunE setup.
233-248: Well-structured file download utilityThe
downloadFilefunction is cleanly implemented with proper error handling, resource cleanup with deferred closes, and a clear return value. This function will be useful for downloading command files from the Talos repository.
260-278: Improved command processing workflowThe updated loop for command processing simplifies the workflow by using the raw command name directly and applying the new
toCamelCasefunction. This makes the code more readable and maintainable.pkg/commands/imported_wipe.go (2)
28-32: Good command structure and clear documentationThe command is well-structured with a descriptive short summary and no arguments validation. The documentation is clear about the purpose of the command.
33-59: Well-implemented wipe disk command with proper validationThe
wipeDiskCmdincludes proper argument validation (requiring at least one device name), clear error messaging for invalid wipe methods, and leveragesxslices.Mapfor elegant transformation of the device names into descriptor objects.pkg/commands/imported_meta.go (2)
36-52: Well-implemented write command with proper error handlingThe
metaWriteCmdimplementation includes proper key validation, error handling, and conditional client usage based on the insecure flag. The function effectively parses the key as an unsigned integer and sends the write request to the client.
106-110: Good security option with insecure flagThe implementation of the insecure flag as a persistent flag that applies to all subcommands is a good approach. It correctly allows the user to choose between secure and insecure modes for all meta commands.
pkg/commands/imported_copy.go (3)
28-35: Clear and comprehensive documentationThe command documentation clearly explains the functionality, including how the archive is created, streamed back, and the options for handling the local path (stdout or directory extraction). It also mentions important caveats about file ownership and permissions.
37-46: Excellent tab completion implementationThe
ValidArgsFunctionimplementation provides a good user experience by offering path completion from the node when entering the first argument, and using default completion for the second argument.
48-83: Well-implemented copy command with proper error handlingThe command implementation includes several notable good practices:
- Preventing multi-node operations for copy (which makes sense for this operation)
- Proper error wrapping for meaningful error messages
- Flexible output handling (stdout or file extraction)
- Path validation and directory creation if needed
- Descriptive error messages for each failure case
The code is well-structured and handles all edge cases appropriately.
pkg/commands/imported_edit.go (3)
1-6: Avoid modifying auto-generated note.These lines clearly indicate that the file is auto-generated and should not be edited manually. Any changes here would be overwritten by future runs of the generation tool, so it is best to keep these lines as-is.
44-45: Review cyclomatic complexity justification.The
//nolint:gocyclocomment disables the cyclomatic complexity check. Ensure that the complexity ofeditFnis truly necessary and not a sign that the function needs to be broken down into smaller, more focused parts.
138-144: Confirm the usage ofApplyConfigurationRequestwith the new resource data.Applying edited data is a critical step, but there’s no direct confirmation or post-check that the new config is valid in practice. You might consider verifying the resource post-apply (if that’s feasible) to confirm that the changes are persisted successfully.
pkg/commands/imported_rotate-ca.go (2)
72-79: Confirm correctness of time-based operations.The new secret bundle is generated using
secrets.NewFixedClock(time.Now()). Ensure that the local system clock doesn't introduce issues for ephemeral or distributed scenarios. In some cases, it may be preferable to use a standardized time source (e.g., an NTP-synchronized approach).
82-95: Ensure consistent cluster state post-rotation.After rotating Talos CA and recreating the client, consider verifying that all nodes indeed trust the new CA and that the user can continue issuing further commands. A partial rotation that leaves some nodes behind could cause connectivity issues.
pkg/commands/imported_support.go (2)
70-72: Enforce at least one node inGlobalArgs.Nodes.Relying on
return errors.New("please provide...")is straightforward, but if future expansions allow for remote or alternate data sources, the exit condition might need to handle more nuanced scenarios. Keep an eye on potential expansions down the line.
81-96: Ensure goroutine error handling remains robust.You use
errgroupto handle concurrency. Note that partial failures in one goroutine might be overshadowed by the logic in another. If data collection for one node fails catastrophically, confirm whether that should prevent continuing with other nodes or if partial success is desired behavior.
| var wipeCmdFlags struct { | ||
| configFiles []string | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Variable declaration after usage
The wipeCmdFlags struct is declared after it's used in the init function. It's better to declare this at the beginning of the file, near other flags structs.
-var wipeCmdFlags struct {
- configFiles []string
-}Move this declaration to line 27, just before wipeDiskCmdFlags:
var wipeCmdFlags struct {
configFiles []string
}| var cpCmdFlags struct { | ||
| configFiles []string | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Variable declaration after usage
The cpCmdFlags struct is declared after it's used in the init function. It's better to declare this at the beginning of the file, before it's used.
-var cpCmdFlags struct {
- configFiles []string
-}Move this declaration to line 22, just before the cpCmd declaration:
var cpCmdFlags struct {
configFiles []string
}| for { | ||
| var ( | ||
| buf bytes.Buffer | ||
| w io.Writer = &buf | ||
| ) | ||
|
|
||
| if runtime.GOOS == "windows" { | ||
| w = crlf.NewCRLFWriter(w) | ||
| } | ||
|
|
||
| _, err := w.Write([]byte( | ||
| fmt.Sprintf( | ||
| "# Editing %s/%s at node %s\n", mc.Metadata().Type(), id, node, | ||
| ), | ||
| )) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| if lastError != "" { | ||
| _, err = w.Write([]byte(addEditingComment(lastError))) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Guard against infinite edit attempts.
The loop retries on errors from c.ApplyConfiguration, and it only breaks on success or when no changes are detected. In theory, if the user repeatedly makes invalid edits, this can become an infinite loop. Consider implementing a maximum retry limit, or offering the user a clearer way to abort.
| func collectData(dest *os.File, progress chan bundle.Progress) error { | ||
| return WithClientNoNodes(func(ctx context.Context, c *client.Client) error { | ||
| clientset, err := getKubernetesClient(ctx, c) | ||
| if err != nil { | ||
| fmt.Fprintf(os.Stderr, "Failed to create kubernetes client %s\n", err) | ||
| } | ||
|
|
||
| opts := []bundle.Option{ | ||
| bundle.WithArchiveOutput(dest), | ||
| bundle.WithKubernetesClient(clientset), | ||
| bundle.WithTalosClient(c), | ||
| bundle.WithNodes(GlobalArgs.Nodes...), | ||
| bundle.WithNumWorkers(supportCmdFlags.numWorkers), | ||
| bundle.WithProgressChan(progress), | ||
| } | ||
|
|
||
| if !supportCmdFlags.verbose { | ||
| opts = append(opts, bundle.WithLogOutput(io.Discard)) | ||
| } | ||
|
|
||
| options := bundle.NewOptions(opts...) | ||
|
|
||
| collectors, err := collectors.GetForOptions(ctx, options) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| return support.CreateSupportBundle(ctx, options, collectors...) | ||
| }) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Review balancing worker usage.
The collectData function respects supportCmdFlags.numWorkers for concurrency across nodes. Confirm that the set concurrency level doesn’t overwhelm the cluster or lead to resource overconsumption. For large clusters, you might want a smarter concurrency strategy or user guidance.
Signed-off-by: Andrei Kvapil <kvapss@gmail.com>
kvaps
force-pushed
the
import-new-commands
branch
from
d833665 to
55fa995
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (5)
pkg/commands/imported_wipe.go (4)
109-111: Variable declaration after usageThe
wipeCmdFlagsstruct is declared after it's used in theinitfunction. It's better to declare this at the beginning of the file, near other flags structs.Move this declaration to line 27, just before
wipeDiskCmdFlags:var wipeCmdFlags struct { configFiles []string }
66-68:⚠️ Potential issueFix range loop in wipeMethodValues function
The range loop in
wipeMethodValueshas an error. It's usingidx := range method.Descriptor().Values().Len()which isn't valid syntax.- for idx := range method.Descriptor().Values().Len() { + for idx := 0; idx < method.Descriptor().Values().Len(); idx++ { values[idx] = storage.BlockDeviceWipeDescriptor_Method_name[int32(idx)] }
93-96:⚠️ Potential issueIncorrect flags configuration
The
wipeDiskCmd.Flags()is usingwipeCmdFlags.configFilesinstead of the command's own flags (wipeDiskCmdFlags). This could cause unexpected behavior if both commands are used in the same session.- wipeDiskCmd.Flags().StringSliceVarP(&wipeCmdFlags. - configFiles, "file", - "f", nil, "specify config files or patches in a YAML file (can specify multiple)", + wipeDiskCmd.Flags().StringSliceVarP(&wipeDiskCmdFlags.configFiles, "file", + "f", nil, "specify config files or patches in a YAML file (can specify multiple)",
97-98:⚠️ Potential issueIncorrect PreRunE assignment
The
wipeDiskCmd.PreRunEis set towipeCmd.PreRunE, which will usewipeCmdFlagsinstead ofwipeDiskCmdFlags. This could lead to unexpected behavior.- wipeDiskCmd. - PreRunE = wipeCmd.PreRunE + wipeDiskCmd.PreRunE = func(cmd *cobra.Command, args []string) error { + nodesFromArgs := len(GlobalArgs.Nodes) > 0 + endpointsFromArgs := len(GlobalArgs.Endpoints) > 0 + for _, configFile := range wipeDiskCmdFlags.configFiles { + if err := processModelineAndUpdateGlobals(configFile, nodesFromArgs, endpointsFromArgs, false); err != nil { + return err + } + } + return nil + }pkg/commands/imported_edit.go (1)
74-153: 🛠️ Refactor suggestionGuard against infinite edit attempts.
The loop retries on errors from
c.ApplyConfiguration, and it only breaks on success or when no changes are detected. In theory, if the user repeatedly makes invalid edits, this can become an infinite loop.Consider implementing a maximum retry limit, or offering the user a clearer way to abort the editing process.
+ maxRetries := 5 + retryCount := 0 for { + if retryCount >= maxRetries { + return fmt.Errorf("maximum number of retries (%d) reached, aborting", maxRetries) + } + retryCount++ var ( buf bytes.Buffer w io.Writer = &buf )
🧹 Nitpick comments (6)
pkg/commands/imported_diskusage.go (1)
155-157: Struct declaration after usage in code.The
duCmdFlagsstruct is declared after it's used in theinitfunction. While this works in Go, it would be more conventional to declare it before usage.Consider moving this struct declaration to before the
initfunction or at the beginning of the file with other variable declarations.tools/import_commands.go (2)
21-30: Consider avoidingstrings.Titlefor non-ASCII compatibility.
strings.Titleis known to be deprecated and may not properly handle certain Unicode strings. For deterministic behavior, consider manually uppercasing only the first letter of each part or usingstrings.ToUpperon the runes.func toCamelCase(s string, export bool) string { parts := strings.Split(s, "-") for i := range parts { if i == 0 && !export { continue } - parts[i] = strings.Title(parts[i]) + if len(parts[i]) > 0 { + parts[i] = strings.ToUpper(parts[i][:1]) + parts[i][1:] + } } return strings.Join(parts, "") }
158-170: Safeguard potential user inputs.
When building and injecting code strings, ensure upstream inputs (e.g., command names) are validated. This prevents unexpected code generation if a malicious or malformed command name is passed.pkg/commands/imported_rotate-ca.go (1)
60-104: Check concurrency or cancellation usage.
rotateCAuses standard logic to rotate Talos and Kubernetes CAs. Consider passing context cancellation or timeouts if rotation is lengthy.pkg/commands/imported_support.go (2)
46-116: Prevent partial archiving disclaimers.
While the command checks iflen(GlobalArgs.Nodes) == 0, consider also verifying that the user has permissions or connectivity for all specified nodes, to avoid partial or incomplete bundles.
284-341: Real-time progress is beneficial.
TheshowProgressfunction usesuiprogresseffectively. Consider a fallback non-interactive mode if progress bars aren’t available (e.g., automated scripts).
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (11)
Makefile(1 hunks)go.mod(13 hunks)pkg/commands/imported_copy.go(1 hunks)pkg/commands/imported_diskusage.go(1 hunks)pkg/commands/imported_edit.go(1 hunks)pkg/commands/imported_image.go(1 hunks)pkg/commands/imported_meta.go(1 hunks)pkg/commands/imported_rotate-ca.go(1 hunks)pkg/commands/imported_support.go(1 hunks)pkg/commands/imported_wipe.go(1 hunks)tools/import_commands.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- Makefile
- pkg/commands/imported_image.go
- pkg/commands/imported_copy.go
- go.mod
🧰 Additional context used
🧬 Code Definitions (3)
pkg/commands/imported_wipe.go (2)
pkg/commands/root.go (2)
WithClient(71-90)GlobalArgs(24-24)internal/pkg/pci/pci.go (1)
Device(13-19)
pkg/commands/imported_meta.go (1)
pkg/commands/root.go (3)
WithClientMaintenance(93-95)WithClient(71-90)GlobalArgs(24-24)
pkg/commands/imported_rotate-ca.go (2)
pkg/commands/root.go (2)
WithClient(71-90)GlobalArgs(24-24)pkg/engine/engine.go (1)
Options(39-57)
🔇 Additional comments (31)
pkg/commands/imported_diskusage.go (4)
38-52: The ValidArgsFunction restricts path completion to directories only.The completion function properly restricts path suggestions to directories when completing command arguments, which is appropriate for a disk usage command.
53-123: Command implementation handles output formatting well.The RunE function effectively:
- Processes paths (defaulting to "/" if none provided)
- Makes the gRPC request with appropriate parameters
- Sets up a tabwriter for clean output formatting
- Handles both humanized and raw size display
- Properly formats output for both single and multi-node scenarios
127-146: PreRunE implementation processes configuration files correctly.The implementation properly handles configuration files specified via the
-fflag, updating global arguments as needed.
148-152: Command flags are well-organized with clear descriptions.The flags provide appropriate options for customizing command behavior with human-readable output, filtering capabilities, and recursion depth control.
pkg/commands/imported_meta.go (4)
19-22: Well-designed flags structureThe
metaCmdFlagsstruct effectively encapsulates all configuration options for the command, including the insecure mode flag and configuration files.
31-52: Well-implemented write command with proper error handlingThe
metaWriteCmdimplementation:
- Properly parses the key as an unsigned integer
- Returns errors if parsing fails
- Handles both secure and insecure client modes
- Uses the correct client method for writing to META partition
54-75: Well-implemented delete command with proper error handlingThe
metaDeleteCmdimplementation:
- Properly parses the key as an unsigned integer
- Returns errors if parsing fails
- Handles both secure and insecure client modes
- Uses the correct client method for deleting from META partition
106-110: Good command hierarchy organizationThe code properly sets up the command hierarchy by:
- Adding the insecure flag to persistent flags, making it available to all subcommands
- Adding subcommands to the parent command
- Adding the parent command to the root command
pkg/commands/imported_edit.go (4)
36-42: Good flag structure with appropriate typesThe
editCmdFlagsstruct includes all necessary configuration options with appropriate types, including:
- Mode flags for configuration application
- Namespace string
- Dry run boolean
- Configuration timeout duration
- Configuration files array
159-172: Efficient comment stripping implementationThe
stripEditingCommentfunction efficiently removes the editing comments from the input byte slice using a simple iteration approach.
174-178: Well-structured comment formatting functionThe
addEditingCommentfunction properly formats error messages as comments, making them clear to the user in the editor interface.
191-207: Good command implementation with version checkThe
editCmd.RunEfunction:
- Performs a client version check before proceeding
- Iterates through each node to apply the edits
- Creates a node-specific context for each operation
- Uses the
helpers.ForEachResourceutility to handle resource processingtools/import_commands.go (6)
60-60: Logic to stop AST inspection appears intentional.
Returningfalsehalts the AST iteration after successfully adding the field, which is likely the intended behavior.
78-78: Repeated return ensures single match handling.
Similar to line 60, returningfalsehere ensures only the first matching struct is updated.
145-154: Confirm consistent naming for transformations.
Mapping “diskusage”→“du”, “copy”→“cp”, and “rotateCa”→“rotateCA” is valid. Verify that you intend to pass “rotate-ca” or “rotateCa” to this logic.
155-157: Field addition looks correct.
AssigningconfigFilesto each command’s flags struct is consistent with how other commands handle their flags.
171-206: Repetitive subcommand logic is well-structured.
These subcommand loops are consistent with your other logic for code generation. No issues detected.
260-277: Handle special command aliases consistently.
Renaming "list"→"ls" indicates selective short aliases. Confirm whether other commands also need short aliases or special mapping here.pkg/commands/imported_rotate-ca.go (6)
1-8: Header and package declaration.
No issues detected with licensing or package placement.
27-37: Review struct naming and usage.
rotateCACmdFlagsaligns consistently with other command-flag structs. Ensure no conflicting fields with existing commands.
39-58: Command logic definition is clear.
The help message clearly outlines usage and flags. Argument handling is straightforward withcobra.NoArgs.
106-149: Handle new Talos CA generation with clear error messages.
This function methodically updates the Talos config. No concerns beyond verifying that the user is aware a partial rotation might leave the cluster in a transitional state if any step fails.
150-177: Straightforward Kubernetes CA rotation.
Logic mirrors Talos CA rotation, ensuring consistent approach. Careful error reporting is present. Looks good.
179-211: Flag definitions and PreRunE usage.
Initialization code properly binds flags. The approach is consistent with other generated commands.pkg/commands/imported_support.go (7)
38-43: Struct naming and usage.
supportCmdFlagsfields match well with typical CLI conventions. No concerns here.
118-147:collectDataconcurrency strategy looks solid.
WithClientNoNodeswraps the logic cleanly. Might consider advanced concurrency if user sets a high--num-workersand the cluster is large.
149-177: Validate cluster readiness.
getKubernetesClientensures the "kube-system" namespace is reachable. Good check against misconfigured clusters.
179-197:getDiscoveryConfigusage.
No issues found. This function gracefully retrieves cluster config or fails early.
199-230: Confirmed user overwrite prompts.
openArchivecarefully handles user confirmation for overwriting the existing archive. This helps prevent accidental data loss.
232-282: Good error aggregation approach.
supportBundleErrorscollects all errors, then prints them. This is helpful for complex multi-node operations.
343-368: Flag bindings and initialization.
Matches the established pattern for config file handling and node membership updates.
| func downloadFile(srcName, dstName string, url string) error { | ||
| resp, err := http.Get(url + "/" + srcName) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer resp.Body.Close() | ||
|
|
||
| file, err := os.Create(dstName) | ||
| if err != nil { | ||
| return err | ||
| } | ||
| defer file.Close() | ||
|
|
||
| _, err = io.Copy(file, resp.Body) | ||
| return err | ||
| } |
There was a problem hiding this comment.
Check for non-200 status codes.
Currently, the HTTP response body is consumed regardless of status. A non-successful response code returns no error, causing potential silent failures.
func downloadFile(srcName, dstName string, url string) error {
resp, err := http.Get(url + "/" + srcName)
if err != nil {
return err
}
defer resp.Body.Close()
+ if resp.StatusCode != http.StatusOK {
+ return fmt.Errorf("request failed with status code %d", resp.StatusCode)
+ }
file, err := os.Create(dstName)
if err != nil {
return err
}
defer file.Close()
_, err = io.Copy(file, resp.Body)
return 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.
| func downloadFile(srcName, dstName string, url string) error { | |
| resp, err := http.Get(url + "/" + srcName) | |
| if err != nil { | |
| return err | |
| } | |
| defer resp.Body.Close() | |
| file, err := os.Create(dstName) | |
| if err != nil { | |
| return err | |
| } | |
| defer file.Close() | |
| _, err = io.Copy(file, resp.Body) | |
| return err | |
| } | |
| func downloadFile(srcName, dstName string, url string) error { | |
| resp, err := http.Get(url + "/" + srcName) | |
| if err != nil { | |
| return err | |
| } | |
| defer resp.Body.Close() | |
| if resp.StatusCode != http.StatusOK { | |
| return fmt.Errorf("request failed with status code %d", resp.StatusCode) | |
| } | |
| file, err := os.Create(dstName) | |
| if err != nil { | |
| return err | |
| } | |
| defer file.Close() | |
| _, err = io.Copy(file, resp.Body) | |
| return err | |
| } |
Signed-off-by: Andrei Kvapil kvapss@gmail.com
Summary by CodeRabbit
New Features
Chores