-
Notifications
You must be signed in to change notification settings - Fork 245
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
feat: wsh export-config
command
#1641
base: main
Are you sure you want to change the base?
Conversation
WalkthroughThis pull request introduces a new Key functions added include 🪧 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 (
|
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: 1
🧹 Nitpick comments (3)
cmd/wsh/cmd/wshcmd-export-config.go (3)
15-22
: Consider adding default directory logic and argument validations
Currently, the command strictly requires an output path. If the user runswsh export-config
without providing an argument, it raises an error. You could supply a default path (e.g.,export.zip
) and rely oncobra.ExactArgs(0)
or the built-inArgs
validators for improved user experience.
25-27
: Make error message more consistent
The error message "export-config requires an output path" is correct but might be more consistent with other CLI errors if rephrased, for example: "Error: output path is required. Usage: wsh export-config [output-path]"- return fmt.Errorf("export-config requires an output path") + return fmt.Errorf("Error: output path is required. Usage: wsh export-config [output-path]")
82-88
: Return an error if home directory cannot be determined
Whenos.UserHomeDir()
fails, returning an empty string might cause confusion or errors downstream. Instead, return an error or a fallback path so that callers can handle the situation properly.func getWaveConfigDir() (string, error) { homeDir, err := os.UserHomeDir() if err != nil { - return "" + return "", fmt.Errorf("unable to determine user home directory: %v", err) } return filepath.Join(homeDir, ".config", "waveterm"), nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/wsh/cmd/wshcmd-export-config.go
(1 hunks)
🔇 Additional comments (1)
cmd/wsh/cmd/wshcmd-export-config.go (1)
43-59
: Preserve directory structure within the zip
The current approach skips subdirectories by immediately returning nil
if info.IsDir()
. This won’t add empty subdirectories to the archive. While this may be acceptable, confirm whether you want to preserve the directory hierarchy in the generated zip.
I added a check for the existence of config directory, please take a look |
Looks good to me! Can you add documentation to the |
0fa065c
to
9011102
Compare
I added to the documentation and I modified the command from |
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: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
cmd/wsh/cmd/wshcmd-export-config.go
(1 hunks)docs/docs/wsh-reference.mdx
(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmd/wsh/cmd/wshcmd-export-config.go
## exportconfig | ||
|
||
You can export your Wave's config files into a zip archive using this command. | ||
|
||
``` | ||
wsh exportconfig [output-path] | ||
``` | ||
|
||
--- |
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.
🛠️ Refactor suggestion
Enhance documentation for the exportconfig
command.
While the basic documentation is clear, it would be more helpful to users if it included:
- Default behavior when no output path is specified
- List of configuration files that get included in the zip
- Usage examples
- Any available command flags/options
Consider expanding the documentation like this:
## exportconfig
-You can export your Wave's config files into a zip archive using this command.
+You can export your Wave Terminal's configuration files into a zip archive. This is useful for backing up your settings or transferring them to another machine.
wsh exportconfig [output-path]
+
+If no output path is specified, it defaults to `wave-config-<timestamp>.zip` in the current directory.
+
+The exported archive includes:
+- settings.json
+- presets.json
+- widgets.json
+- AI presets
+- Other configuration files
+
+Examples:
+```bash
+# Export to the default location
+wsh exportconfig
+
+# Export to a specific file
+wsh exportconfig ~/backups/wave-config.zip
+
+# Export to a specific directory (filename will be auto-generated)
+wsh exportconfig ~/backups/
+```
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.
good suggestion @weilirs
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.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
}) | ||
} | ||
|
||
func getWaveConfigDir() string { |
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.
This is not reliable as the user may have a different config directory defined, such as on dev where it's ~/.config/waveterm-dev
. You should use wavebase.GetWaveConfigDir
instead.
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.
However, because this is called from the wsh side, which doesn't have the environment variables defined, you should instead call the wshclient.PathCommand
function, since that will get you whatever the value is in wavesrv
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.
actually, this won't work at all if you're on a remote machine. The logic for zipping the config should be moved to wavesrv and called through the wshserver mechanism, you can look at the other commands to see how we do this. Then, if the user is invoking this command from a remote machine, we should copy the zipped archive to the output path on the remote machine
feat #1608
export-config [output-path]
to export config files into a zip archive.