-
Notifications
You must be signed in to change notification settings - Fork 156
POC - Support Multiple VPSes #77
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
base: main
Are you sure you want to change the base?
Conversation
- New kubeconfig-like yaml config for sidekick - Add config subcommands - Centralize config parsing and validation in rootcmd - Update all commands to handle the new config
|
Hey dude, The switch can happen in a TUI from the launch command yeah. That would be great. I like the current approach. Happy to merge and finish it off! |
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.
Pull request overview
This PR introduces support for managing multiple VPS servers in Sidekick by implementing a kubectl-inspired configuration schema. The changes refactor the existing single-server viper-based configuration to a new multi-server YAML config structure with contexts, enabling users to manage and switch between multiple VPS deployments.
Key Changes:
- New config schema with
SidekickConfig,SidekickServer, andSidekickContexttypes that support multiple servers and contexts - New
configcommand with subcommands for viewing current context, switching contexts, and migrating from old config format - Refactored all commands (launch, deploy, preview, initialize) to use the new config system instead of viper globals
Reviewed changes
Copilot reviewed 11 out of 12 changed files in this pull request and generated 19 comments.
Show a summary per file
| File | Description |
|---|---|
| utils/types.go | Adds new config types (SidekickConfig, SidekickServer, SidekickContext) for multi-server support |
| utils/config.go | Implements config management utilities including find, add/replace operations, and file I/O |
| utils/utils.go | Updates HandleEnvFile to accept publicKey as parameter instead of reading from viper |
| utils/utils_test.go | Updates test to pass publicKey directly to HandleEnvFile |
| cmd/root.go | Adds config initialization in PersistentPreRun, loads config into command context |
| cmd/config/config.go | New command providing subcommands for config management (current, use, migrate) |
| cmd/initialize/initialize.go | Refactored to use new config system, adds server name flag, updates server selection logic |
| cmd/deploy/deploy.go | Refactored to retrieve server from config context, adds IP validation for multi-server safety |
| cmd/launch/launch.go | Refactored to use server from config context instead of viper, fixes typo in error message |
| cmd/preview/preview.go | Refactored to use server from config context instead of viper |
| go.mod | Adds moby/moby dependency (indirect) |
| go.sum | Updates checksums for new dependency |
Comments suppressed due to low confidence (3)
cmd/initialize/initialize.go:204
- The condition logic is incorrect. It checks if the server name matches AND the address differs AND publicKey is not empty, but this condition will never be true when err is nil because if err is nil, the server was found with the given name. The logic should check if a server with this name already exists (err == nil) and has a different address. Consider restructuring to:
if err == nil && sidekickServer.Address != server && sidekickServer.PublicKey != "" && !skipPromptsFlag
cmd/initialize/initialize.go:204 - Extra space before the logical AND operator. Should be
sidekickServer.PublicKey != ""without the trailing space.
cmd/initialize/initialize.go:283 - Extra space after the opening brace in struct initialization. Should be
utils.SidekickContext{without the space after the brace.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
|
||
| if viper.GetString("secretKey") == "" { | ||
| if sidekickServer.SecretKey == "" { | ||
| render.GetLogger(log.Options{Prefix: "Backward Compat"}).Error("Recent changes to how Sidekick handles secrets prevents you from launcing a new application.") |
Copilot
AI
Dec 13, 2025
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.
Typo in error message: "launcing" should be "launching".
| render.GetLogger(log.Options{Prefix: "Backward Compat"}).Error("Recent changes to how Sidekick handles secrets prevents you from launcing a new application.") | |
| render.GetLogger(log.Options{Prefix: "Backward Compat"}).Error("Recent changes to how Sidekick handles secrets prevents you from launching a new application.") |
| if err != nil { | ||
| return err | ||
| } | ||
| return os.WriteFile(path, data, os.ModePerm) |
Copilot
AI
Dec 13, 2025
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.
Using os.ModePerm (0777) for config file permissions is overly permissive and a security risk. The config file contains sensitive information like secret keys and should have restricted permissions. Consider using 0600 (read/write for owner only) or 0644 (read/write for owner, read-only for others) instead.
| return os.WriteFile(path, data, os.ModePerm) | |
| return os.WriteFile(path, data, 0o600) |
cmd/config/config.go
Outdated
| newConfig := utils.SidekickConfig{ | ||
| Version: "1", | ||
| Servers: []utils.SidekickServer { serverConfig }, | ||
| Contexts: []utils.SidekickContext { defaultContext }, |
Copilot
AI
Dec 13, 2025
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.
Extra space after the opening brace in the slice literal. Should be []utils.SidekickContext{defaultContext} without the space after the brace.
| Contexts: []utils.SidekickContext { defaultContext }, | |
| Contexts: []utils.SidekickContext{defaultContext}, |
| var migrateCmd = &cobra.Command{ | ||
| Use: "migrate", | ||
| Short: "Migrate the old sidekick config to the new config", | ||
| Run: func(cmd *cobra.Command, args []string) { | ||
| var serverConfig utils.SidekickServer | ||
| content, err := os.ReadFile(viper.GetString("config")) | ||
| if err != nil { | ||
| pterm.Fatal.Println(err) | ||
| } | ||
| err = yaml.Unmarshal(content, &serverConfig) | ||
| if err != nil { | ||
| pterm.Fatal.Println(err) | ||
| } | ||
|
|
||
| serverConfig.Name = "default" | ||
| defaultContext := utils.SidekickContext{ | ||
| Name: "default", | ||
| Server: "default", | ||
| } | ||
| newConfig := utils.SidekickConfig{ | ||
| Version: "1", | ||
| Servers: []utils.SidekickServer { serverConfig }, | ||
| Contexts: []utils.SidekickContext { defaultContext }, | ||
| CurrentContext: defaultContext.Name, | ||
| } | ||
| err = newConfig.Print() | ||
| if err != nil { | ||
| pterm.Fatal.Println(err) | ||
| } | ||
| }, |
Copilot
AI
Dec 13, 2025
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.
The migrate command only prints the new config to stdout but doesn't actually save it to the file. Users would need to manually redirect the output. Consider either saving the migrated config directly to the config file or clearly documenting in the command's Long description that users need to redirect output (e.g., sidekick config migrate > ~/.config/sidekick/default.yaml).
utils/config.go
Outdated
| } | ||
| } | ||
|
|
||
| if idx == - 1 { |
Copilot
AI
Dec 13, 2025
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.
There is a spacing issue in the comparison. The condition checks idx == - 1 with a space between - and 1, which should be idx == -1 without a space.
| if viper.GetString("secretKey") == "" { | ||
| func prelude(server *utils.SidekickServer) string { | ||
| if server.SecretKey == "" { | ||
| render.GetLogger(log.Options{Prefix: "Backward Compat"}).Error("Recent changes to how Sidekick handles secrets prevents you from launcing a new application.") |
Copilot
AI
Dec 13, 2025
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.
Typo in error message: "launcing" should be "launching".
| render.GetLogger(log.Options{Prefix: "Backward Compat"}).Error("Recent changes to how Sidekick handles secrets prevents you from launcing a new application.") | |
| render.GetLogger(log.Options{Prefix: "Backward Compat"}).Error("Recent changes to how Sidekick handles secrets prevents you from launching a new application.") |
| } else { | ||
| err := yaml.Unmarshal(content, &config) | ||
| if err != nil { | ||
| pterm.Fatal.Sprintf("Error unmarshaling the config yaml file: %s", err) |
Copilot
AI
Dec 13, 2025
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.
The pterm.Fatal.Sprintf is incorrect usage. pterm.Fatal.Sprintf returns a string but doesn't print or exit. It should be either pterm.Fatal.Printfln to print and exit, or pterm.Fatal.PrintOnError(fmt.Errorf(...)) to handle the error properly.
| pterm.Fatal.Sprintf("Error unmarshaling the config yaml file: %s", err) | |
| pterm.Fatal.Printfln("Error unmarshaling the config yaml file: %s", err) |
cmd/deploy/deploy.go
Outdated
|
|
||
| ips, err := net.LookupIP(appConfig.Url) | ||
| if err != nil { | ||
| panic(err) |
Copilot
AI
Dec 13, 2025
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.
The IP validation logic uses panic for error handling, which is not appropriate for expected error conditions like DNS lookup failures. Consider using proper error handling with pterm.Error.Println and os.Exit(1) to maintain consistency with the rest of the error handling in this function.
| panic(err) | |
| pterm.Error.Println(fmt.Sprintf("Failed to resolve IP for %s: %v", appConfig.Url, err)) | |
| os.Exit(1) |
| github.com/mitchellh/hashstructure/v2 v2.0.2 // indirect | ||
| github.com/mitchellh/mapstructure v1.5.0 // indirect | ||
| github.com/moby/docker-image-spec v1.3.1 // indirect | ||
| github.com/moby/moby v28.5.2+incompatible // indirect |
Copilot
AI
Dec 13, 2025
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.
The version string "28.5.2+incompatible" appears unusual. The "+incompatible" suffix in Go modules indicates the package doesn't have proper semantic import versioning for v2+. Consider verifying this is the correct and intended version of moby/moby to use, or if a properly versioned alternative exists.
| github.com/moby/moby v28.5.2+incompatible // indirect |
| } | ||
| func prelude(server *utils.SidekickServer) utils.SidekickAppConfig { | ||
| if !utils.FileExists("./sidekick.yml") { | ||
| pterm.Error.Println(`Sidekick config not found in current directory Run sidekick launch`) |
Copilot
AI
Dec 13, 2025
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.
Inconsistent error message formatting. The error message lacks proper punctuation between clauses. Consider changing to "Sidekick config not found in current directory. Please run sidekick launch." for better readability and consistency with other error messages.
| pterm.Error.Println(`Sidekick config not found in current directory Run sidekick launch`) | |
| pterm.Error.Println("Sidekick config not found in current directory. Please run `sidekick launch`.") |
|
@MightyMoud I've added the selection on the launch command and add the server info in deploy banner. The app yml config will now contain server. I used this package for the selection in the launch wizard https://github.com/charmbracelet/huh. This is quite nice and we can probably use it for the text prompts as well for consistency but that can be done later. |
Goal
Add support for managing multiple VPSes with Sidekick.
Approach
I quite like the
kubectlcli experience when managing different clusters. I think the kubeconfig is intuitive and very easy to extend so I'm using that as a motivation for the new config schema.It will look like this
The idea of having contexts instead of dealing with servers directly is to leave room for extensions. For example, if we want to configure ssh credentials here, we might add a credentials section and the context object will accept a credential key.
However, if this is too overkill, we can remove it for now.
Switching servers is done simply by switching the current context. E.g.
But if a wizard experience is preferred, we can add the selection in the TUI when executing
sidekick launch.TODOS
We will likely have to also change the application sidekick config
sidekick.ymlfile after this change.