feat(cmd)!: Add add sub command to replace previous --add flag#11
Merged
JerryAgbesi merged 3 commits intomainfrom Apr 21, 2026
Merged
feat(cmd)!: Add add sub command to replace previous --add flag#11JerryAgbesi merged 3 commits intomainfrom
add sub command to replace previous --add flag#11JerryAgbesi merged 3 commits intomainfrom
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (3)
✅ Files skipped from review due to trivial changes (2)
📝 WalkthroughWalkthroughReplaces the root Changes
Sequence DiagramsequenceDiagram
actor User
participant CLI as "skipper add"
participant Validator as "Arg Validator"
participant Resolver as "Config Resolver"
participant AddHost as "addHost()"
participant Writer as "sshconfig.AddHost"
participant File as "SSH Config File"
User->>CLI: skipper add devone user@10.0.0.8:9000 -c /path
CLI->>Validator: validate arg count & formats
Validator-->>CLI: valid
CLI->>Resolver: resolveConfigPath(configPath)
Resolver-->>CLI: /path/to/config
CLI->>AddHost: addHost(path, "devone", "user@...:9000")
AddHost->>Writer: AddHost(path, host)
Writer->>File: read/parse existing entries
Writer->>File: check for duplicate alias/identity
alt Entry exists (identical)
Writer-->>AddHost: (host, false, nil)
else New entry
Writer->>File: append new host entry
Writer-->>AddHost: (host, true, nil)
end
AddHost-->>CLI: (host, created)
alt created == true
CLI->>User: "added host devone …"
else
CLI->>User: "host already exists (no change)"
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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.
Actionable comments posted: 1
🧹 Nitpick comments (5)
internal/sshconfig/writer_test.go (1)
59-59: Consider adding assertions for thecreatedboolean.The tests correctly capture three return values but discard the
createdboolean. Whilecmd/root_test.gocovers this behavior at the integration level, adding explicit unit-level assertions here would strengthen coverage.For example, in
TestAddHostWritesSafeIdentityFile:_, created, err := AddHost(configPath, Host{...}) if !created { t.Fatal("expected created=true for new host") }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/sshconfig/writer_test.go` at line 59, Add an assertion for the boolean return from AddHost in writer_test.go: when calling AddHost (the function under test) capture the created bool (e.g., _, created, err := AddHost(...)) and assert that created is true for cases expecting a new host and false when expecting no creation; update TestAddHostWritesSafeIdentityFile and other relevant tests to check the created value and fail the test (t.Fatal/t.Fatalf) when the value is not as expected.cmd/add_test.go (2)
34-50: Consider adding test for malformed target.Per PR objectives, tests should cover "malformed target" scenarios. This would verify the
ParseTargetvalidation path.Suggested additional test
func TestAddCommandRejectsMalformedTarget(t *testing.T) { path := filepath.Join(t.TempDir(), "config") out := new(bytes.Buffer) rootCmd.SetOut(out) rootCmd.SetErr(out) rootCmd.SetArgs([]string{"add", "devone", "invalid-target", "-c", path}) err := rootCmd.Execute() if err == nil { t.Fatal("expected error for malformed target") } if !strings.Contains(err.Error(), "user@host") { t.Fatalf("expected format error, got %v", err) } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/add_test.go` around lines 34 - 50, Add a new test that verifies the ParseTarget validation path by asserting the add command returns a malformed-target error when given an invalid target string: create a test similar to TestAddCommandRejectsWrongArgCount (e.g., TestAddCommandRejectsMalformedTarget) that sets rootCmd args to {"add","devone","invalid-target","-c", path}, calls rootCmd.Execute(), asserts err is non-nil and that err.Error() contains the expected "user@host" (or the ParseTarget format hint); place the test alongside TestAddCommandRejectsWrongArgCount so it exercises ParseTarget and rootCmd.Execute paths.
12-50: Test isolation: sharedrootCmdstate may cause flakiness.Both tests mutate the global
rootCmd(viaSetOut,SetErr,SetArgs) without resetting state between tests. This can cause issues if tests run in parallel or if other tests in the package also userootCmd.Consider resetting state after each test:
Suggested cleanup pattern
func TestAddCommandWritesHost(t *testing.T) { path := filepath.Join(t.TempDir(), "config") out := new(bytes.Buffer) rootCmd.SetOut(out) rootCmd.SetErr(out) rootCmd.SetArgs([]string{"add", "devone", "user@10.0.0.8:9000", "-c", path}) + defer func() { + rootCmd.SetOut(nil) + rootCmd.SetErr(nil) + rootCmd.SetArgs(nil) + }()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@cmd/add_test.go` around lines 12 - 50, The tests are mutating the shared global rootCmd via SetOut/SetErr/SetArgs which can cause flakiness; update each test (TestAddCommandWritesHost and TestAddCommandRejectsWrongArgCount) to use a fresh command instance or to fully reset rootCmd state before/after the test: create or call a factory that returns a new rootCmd, call SetOut/SetErr/SetArgs on that instance, run Execute on it, and ensure any global flags/args are cleared (or defer a reset) so subsequent tests are unaffected; reference rootCmd, SetOut, SetErr, SetArgs, and Execute when making the change.CHANGELOG.md (2)
8-8: Add a date placeholder and reference link for v0.2.0.Per Keep a Changelog format, version headers should include release dates (e.g.,
## [v0.2.0] - YYYY-MM-DDor## [v0.2.0] - Unreleased). Additionally, the reference link at the bottom is missing forv0.2.0.Suggested changes
-## [v0.2.0] +## [v0.2.0] - Unreleased ... [0.1.5]: https://github.com/JerryAgbesi/skipper/releases/tag/v0.1.5 +[v0.2.0]: https://github.com/JerryAgbesi/skipper/releases/tag/v0.2.0🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` at line 8, Update the CHANGELOG entry for "## [v0.2.0]" to include a date placeholder per Keep a Changelog (e.g., change "## [v0.2.0]" to "## [v0.2.0] - Unreleased" or "## [v0.2.0] - YYYY-MM-DD") and add the missing reference link for v0.2.0 at the bottom of the file (add a link like "[v0.2.0]: <compare-or-release-URL>" matching the other release link formats so the header and footnote reference are consistent).
8-35: Inconsistent version format:[v0.2.0]vs[0.1.5].The v0.2.0 header uses a
vprefix while v0.1.5 omits it. Consider using a consistent format throughout the changelog.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@CHANGELOG.md` around lines 8 - 35, The changelog has inconsistent version tags: `[v0.2.0]` vs `[0.1.5]`; normalize them by removing the leading "v" from `[v0.2.0]` (change to `[0.2.0]`) and ensure any corresponding reference/link labels or anchor lines match the same format (e.g., update any `[v0.2.0]: ...` entries if present) so both headers and link footers use the same plain numeric style as `[0.1.5]`.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@AGENTS.md`:
- Line 29: Correct the spelling of "unnesessary" to "unnecessary" in the
contributor guideline phrase "avoid using unnesessary emojis."; update the
sentence to read "avoid using unnecessary emojis." and ensure any other
occurrences of "unnesessary" in AGENTS.md are similarly fixed for consistency.
---
Nitpick comments:
In `@CHANGELOG.md`:
- Line 8: Update the CHANGELOG entry for "## [v0.2.0]" to include a date
placeholder per Keep a Changelog (e.g., change "## [v0.2.0]" to "## [v0.2.0] -
Unreleased" or "## [v0.2.0] - YYYY-MM-DD") and add the missing reference link
for v0.2.0 at the bottom of the file (add a link like "[v0.2.0]:
<compare-or-release-URL>" matching the other release link formats so the header
and footnote reference are consistent).
- Around line 8-35: The changelog has inconsistent version tags: `[v0.2.0]` vs
`[0.1.5]`; normalize them by removing the leading "v" from `[v0.2.0]` (change to
`[0.2.0]`) and ensure any corresponding reference/link labels or anchor lines
match the same format (e.g., update any `[v0.2.0]: ...` entries if present) so
both headers and link footers use the same plain numeric style as `[0.1.5]`.
In `@cmd/add_test.go`:
- Around line 34-50: Add a new test that verifies the ParseTarget validation
path by asserting the add command returns a malformed-target error when given an
invalid target string: create a test similar to
TestAddCommandRejectsWrongArgCount (e.g., TestAddCommandRejectsMalformedTarget)
that sets rootCmd args to {"add","devone","invalid-target","-c", path}, calls
rootCmd.Execute(), asserts err is non-nil and that err.Error() contains the
expected "user@host" (or the ParseTarget format hint); place the test alongside
TestAddCommandRejectsWrongArgCount so it exercises ParseTarget and
rootCmd.Execute paths.
- Around line 12-50: The tests are mutating the shared global rootCmd via
SetOut/SetErr/SetArgs which can cause flakiness; update each test
(TestAddCommandWritesHost and TestAddCommandRejectsWrongArgCount) to use a fresh
command instance or to fully reset rootCmd state before/after the test: create
or call a factory that returns a new rootCmd, call SetOut/SetErr/SetArgs on that
instance, run Execute on it, and ensure any global flags/args are cleared (or
defer a reset) so subsequent tests are unaffected; reference rootCmd, SetOut,
SetErr, SetArgs, and Execute when making the change.
In `@internal/sshconfig/writer_test.go`:
- Line 59: Add an assertion for the boolean return from AddHost in
writer_test.go: when calling AddHost (the function under test) capture the
created bool (e.g., _, created, err := AddHost(...)) and assert that created is
true for cases expecting a new host and false when expecting no creation; update
TestAddHostWritesSafeIdentityFile and other relevant tests to check the created
value and fail the test (t.Fatal/t.Fatalf) when the value is not as expected.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 129bedae-b69e-4cef-86d1-31c8a9bd8b20
📒 Files selected for processing (10)
.goreleaser.yamlAGENTS.mdCHANGELOG.mdREADME.mdcmd/add.gocmd/add_test.gocmd/root.gocmd/root_test.gointernal/sshconfig/writer.gointernal/sshconfig/writer_test.go
| - Every feature must ship with tests covering it; no feature lands without test coverage. | ||
| - Add or update tests whenever behavior changes, and keep `go test ./...` green. | ||
| - Update `README.md` when user-facing behavior or flags change. | ||
| - avoid using unnesessary emojis. |
There was a problem hiding this comment.
Fix spelling in this contributor guideline.
There’s a typo in “unnesessary”; it should be “unnecessary.”
Proposed patch
-- avoid using unnesessary emojis.
+- avoid using unnecessary emojis.Based on learnings: Avoid using unnecessary emojis.
📝 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.
Suggested change
| - avoid using unnesessary emojis. | |
| - avoid using unnecessary emojis. |
🧰 Tools
🪛 LanguageTool
[grammar] ~29-~29: Ensure spelling is correct
Context: ...behavior or flags change. - avoid using unnesessary emojis. ## Commit messages Short, imp...
(QB_NEW_EN_ORTHOGRAPHY_ERROR_IDS_1)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@AGENTS.md` at line 29, Correct the spelling of "unnesessary" to "unnecessary"
in the contributor guideline phrase "avoid using unnesessary emojis."; update
the sentence to read "avoid using unnecessary emojis." and ensure any other
occurrences of "unnesessary" in AGENTS.md are similarly fixed for consistency.
JerryAgbesi
force-pushed
the
feat/add-subcommand
branch
from
963f56c to
394555f
Compare
JerryAgbesi
changed the title
add sub command to replace previous --add flagadd sub command to replace previous --add flag
4 tasks
4 tasks
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
Replace the -a, --add root flag with a dedicated add subcommand so write operations live under their own verb (git/gh-style).
Behavioral changes:
the CLI now prints host "" already exists for , no change instead of claiming a write happened.
Docs / release:
Related issue
Closes #7
Summary by CodeRabbit
New Features
addsubcommand for adding SSH host aliases:add <alias> <user@host[:port]>.Breaking Changes
--addflag; use theaddsubcommand.Documentation
Tests
addsubcommand.Chores