add interactive form to skipper add#13
Open
kwabenadarkwa wants to merge 1 commit intoJerryAgbesi:mainfrom
Open
add interactive form to skipper add#13kwabenadarkwa wants to merge 1 commit intoJerryAgbesi:mainfrom
kwabenadarkwa wants to merge 1 commit intoJerryAgbesi:mainfrom
Conversation
Running 'skipper add' with no positional args now opens a huh form prompting for alias, user, host name, and port. Alias stays optional and is derived from host[:port] when blank. Existing two-arg non-interactive flow is unchanged; passing only one arg now errors explicitly instead of failing on arg count.
📝 WalkthroughWalkthroughThe pull request enhances the Changes
Sequence Diagram(s)sequenceDiagram
actor User
participant CLI as add Command
participant Form as addform.Run
participant Validator as Form Validators
participant Config as sshconfig
User->>CLI: skipper add (no args)
CLI->>Form: addFormRunner(Input)
Form->>User: Display form fields
User->>Form: Enter alias, user@host, port
Form->>Validator: Validate each field
Validator-->>Form: Validation result
alt Validation succeeds
Form-->>CLI: Result{Alias, User, Hostname, Port}
CLI->>Config: AddHost(Host{...})
Config-->>CLI: success
CLI->>User: Print add result
else Validation fails or User cancels
Form-->>CLI: Cancelled=true OR error
CLI-->>User: Exit or error message
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related issues
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
cmd/add.go (1)
29-42:⚠️ Potential issue | 🟡 MinorCheck argument shape before resolving config path.
At Line 30, config path resolution runs before the one-arg guard. This can return a path error instead of the explicit argument error for invalid invocations.
💡 Suggested adjustment
func runAdd(_ *cobra.Command, args []string) error { - path, err := resolveConfigPath(configPath) - if err != nil { - return err - } - switch len(args) { case 2: + path, err := resolveConfigPath(configPath) + if err != nil { + return err + } return addFromArgs(path, args[0], args[1]) case 1: return fmt.Errorf("expected either no arguments or both <alias> and <user@host[:port]>") default: + path, err := resolveConfigPath(configPath) + if err != nil { + return err + } return addInteractive(path) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/add.go` around lines 29 - 42, The function runAdd calls resolveConfigPath(configPath) before validating args which lets path errors surface for the one-argument invalid invocation; update runAdd to validate the argument shape first (check len(args) and immediately return the explicit error when len==1) and only call resolveConfigPath(configPath) for the branches that need it (the two-arg branch that calls addFromArgs and the interactive branch that calls addInteractive), keeping references to runAdd, resolveConfigPath, addFromArgs, and addInteractive to locate the change.
🧹 Nitpick comments (1)
cmd/add_test.go (1)
53-149: Consider adding one test for form-runner error propagation.You cover success, alias derivation, and cancellation well. A small extra case where
addFormRunnerreturns a non-cancel error would round out interactive-path coverage.As per coding guidelines
**/*_test.go: "Add or update tests whenever behavior changes, and keepgo test ./...green".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/add_test.go` around lines 53 - 149, Add a test (e.g., TestAddCommandInteractiveFormError) that sets addFormRunner to return a non-cancel error (return addform.Result{}, errors.New("form failed")), runs the command via rootCmd with args {"add","-c", path}, asserts that rootCmd.Execute() returns that error (or at least a non-nil error), and verifies no config file was created by checking sshconfig.ParseHosts(path) returns an error; reference addFormRunner, rootCmd.Execute, and sshconfig.ParseHosts to locate the relevant code paths to exercise.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@cmd/add.go`:
- Around line 29-42: The function runAdd calls resolveConfigPath(configPath)
before validating args which lets path errors surface for the one-argument
invalid invocation; update runAdd to validate the argument shape first (check
len(args) and immediately return the explicit error when len==1) and only call
resolveConfigPath(configPath) for the branches that need it (the two-arg branch
that calls addFromArgs and the interactive branch that calls addInteractive),
keeping references to runAdd, resolveConfigPath, addFromArgs, and addInteractive
to locate the change.
---
Nitpick comments:
In `@cmd/add_test.go`:
- Around line 53-149: Add a test (e.g., TestAddCommandInteractiveFormError) that
sets addFormRunner to return a non-cancel error (return addform.Result{},
errors.New("form failed")), runs the command via rootCmd with args {"add","-c",
path}, asserts that rootCmd.Execute() returns that error (or at least a non-nil
error), and verifies no config file was created by checking
sshconfig.ParseHosts(path) returns an error; reference addFormRunner,
rootCmd.Execute, and sshconfig.ParseHosts to locate the relevant code paths to
exercise.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 66e4ab69-7b90-432f-9045-ab7851de9b88
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
README.mdcmd/add.gocmd/add_test.gogo.modinternal/ui/addform/form.gointernal/ui/addform/form_test.go
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
1 participant
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
Running 'skipper add' with no positional args now opens a huh form prompting for alias, user, host name, and port. Alias stays optional and is derived from host[:port] when blank. Existing two-arg non-interactive flow is unchanged; passing only one arg now errors explicitly instead of failing on arg count.
Related issue
Closes #11 and #7
Checklist
make lintpassesgo test ./...passesSummary by CodeRabbit
addcommand now supports two modes: runskipper addwith no arguments to launch an interactive form for entering SSH connection details, or useskipper add <alias> <user@host[:port]>for non-interactive mode as before.