[client] Persist service install parameters across reinstalls#5732
Merged
[client] Persist service install parameters across reinstalls#5732
Conversation
Contributor
📝 WalkthroughWalkthroughAdds persistent service install parameters stored as JSON in the state directory, applies saved values to CLI flags only when flags were not explicitly changed, integrates load/save into install/reconfigure flows, and adds a Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
client/cmd/service_installer.go (1)
122-124: Consider extracting repeated warning blocks into a small helper.The same load/save warning pattern appears in both commands. A helper would reduce duplication and keep messaging consistent.
♻️ Optional refactor sketch
+func warnLoadServiceParams(cmd *cobra.Command, err error) { + if err != nil { + cmd.PrintErrf("Warning: failed to load saved service params: %v\n", err) + } +} + +func warnSaveServiceParams(cmd *cobra.Command, err error) { + if err != nil { + cmd.PrintErrf("Warning: failed to save service params: %v\n", err) + } +} ... - if err := loadAndApplyServiceParams(cmd); err != nil { - cmd.PrintErrf("Warning: failed to load saved service params: %v\n", err) - } + warnLoadServiceParams(cmd, loadAndApplyServiceParams(cmd)) ... - if err := saveServiceParams(currentServiceParams()); err != nil { - cmd.PrintErrf("Warning: failed to save service params: %v\n", err) - } + warnSaveServiceParams(cmd, saveServiceParams(currentServiceParams()))Also applies to: 143-145, 198-200, 237-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/cmd/service_installer.go` around lines 122 - 124, Extract the repeated "load/save warning" pattern into a small helper to reduce duplication: create a function (e.g. handleServiceParamWarning(cmd *cobra.Command, action string, err error) or printServiceParamWarn(cmd *cobra.Command, action string, err error)) that checks err and calls cmd.PrintErrf with a consistent message like "Warning: failed to %s saved service params: %v\n", then replace the repeated blocks around loadAndApplyServiceParams and the corresponding save calls (references: loadAndApplyServiceParams, and the save-related calls at the other occurrences) to call this helper instead.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@client/cmd/service_params.go`:
- Around line 21-23: Update the struct comment for serviceParams to refer to the
correct directory name: change the path wording from "<configDir>/service.json"
to use the resolved state directory (e.g., "<stateDir>/service.json" or mention
configs.StateDir) so the comment matches how the file is actually resolved; edit
the comment immediately above the serviceParams type to make this replacement.
---
Nitpick comments:
In `@client/cmd/service_installer.go`:
- Around line 122-124: Extract the repeated "load/save warning" pattern into a
small helper to reduce duplication: create a function (e.g.
handleServiceParamWarning(cmd *cobra.Command, action string, err error) or
printServiceParamWarn(cmd *cobra.Command, action string, err error)) that checks
err and calls cmd.PrintErrf with a consistent message like "Warning: failed to
%s saved service params: %v\n", then replace the repeated blocks around
loadAndApplyServiceParams and the corresponding save calls (references:
loadAndApplyServiceParams, and the save-related calls at the other occurrences)
to call this helper instead.
🪄 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
Run ID: ffec0aab-2b57-4fbc-9b02-271385099d3c
📒 Files selected for processing (4)
client/cmd/service.goclient/cmd/service_installer.goclient/cmd/service_params.goclient/cmd/service_params_test.go
|
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
client/cmd/service_params.go (1)
195-200: Consider deterministic ordering forenvMapToSliceoutput.
envMapToSlicecurrently relies on map iteration order, which can produce different serialization across runs. While environment variables are converted back to a map when used, deterministic ordering improves reproducibility and simplifies debugging.♻️ Proposed refactor
import ( "context" "encoding/json" "fmt" "maps" "os" "path/filepath" + "sort" @@ func envMapToSlice(m map[string]string) []string { - s := make([]string, 0, len(m)) - for k, v := range m { + keys := make([]string, 0, len(m)) + for k := range m { + keys = append(keys, k) + } + sort.Strings(keys) + + s := make([]string, 0, len(keys)) + for _, k := range keys { + v := m[k] s = append(s, k+"="+v) } return s }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@client/cmd/service_params.go` around lines 195 - 200, envMapToSlice relies on Go map iteration order causing non-deterministic output; make it deterministic by extracting map keys, sorting them (e.g., using sort.Strings), then building the []string in that sorted key order; update the function envMapToSlice to gather keys from the input map[string]string, sort the keys, and append "key=value" pairs in sorted order so the returned slice is reproducible.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@client/cmd/service_params.go`:
- Around line 195-200: envMapToSlice relies on Go map iteration order causing
non-deterministic output; make it deterministic by extracting map keys, sorting
them (e.g., using sort.Strings), then building the []string in that sorted key
order; update the function envMapToSlice to gather keys from the input
map[string]string, sort the keys, and append "key=value" pairs in sorted order
so the returned slice is reproducible.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4e14c42f-92d1-4050-9d4a-29f683e30547
📒 Files selected for processing (1)
client/cmd/service_params.go
pappz
approved these changes
Mar 30, 2026
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
2 participants
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.
Describe your changes
When users run
netbird service uninstallfollowed bynetbird service install(or use--reconfigure), custom flags like--log-level,--management-url,--disable-profiles, and--service-envwere lost. Users had to remember and re-specify every flag.This adds a
service.jsonfile in the state directory that saves install-time parameters and restores them on the next install/reconfigure when the corresponding flag is not explicitly set.<stateDir>/service.jsonafter install and reconfigureChanged()to detect)--management-url ""or--disable-profiles=falsepersist the cleared statenetbird service reset-paramscommand to remove the saved file--service-env) merge: saved keys carry over, explicit keys win on conflictIssue ticket number and link
Stack
Checklist
Documentation
Select exactly one:
The feature is transparent to users: existing behavior is preserved, parameters are just remembered automatically.
Docs PR URL (required if "docs added" is checked)
Paste the PR link from https://github.com/netbirdio/docs here:
https://github.com/netbirdio/docs/pull/__
Summary by CodeRabbit
New Features
reset-paramscommand to clear saved service install parameters.Behavior
Tests