fix(server): improve flag parsing #48
Conversation
WalkthroughThis update modifies the server startup process by removing the Changes
Possibly related PRs
Suggested reviewers
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
🪧 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 (
|
server/start.go
Outdated
| @@ -272,7 +273,14 @@ func startNode( | |||
|
|
|||
| nodeKey := &key.NodeKey{PrivKey: signingKey, PubKey: signingKey.GetPublic()} | |||
|
|
|||
| rollkitcfg, err := config.Load(rootCmd) | |||
| cmd := &cobra.Command{} | |||
There was a problem hiding this comment.
I went this way, Load would work if it was called on start directly.
I think for other protocols loading directly the flags from the command is what makes sense. In the SDK we have another pre-processing flow and create dynamically the command.
I was thinking, maybe adding a LoadFromViper in rollkit as well, but I think it doesn't really makes sense either.
![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
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
go.mod(1 hunks)server/start.go(7 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
server/start.go (1)
rpc/rpc.go (1)
RPCServer(57-65)
🪛 golangci-lint (1.64.8)
server/start.go
279-279: Error return value of flags.Set is not checked
(errcheck)
🔇 Additional comments (7)
go.mod (1)
253-253: Markingpflagas a direct dependency is appropriate.This change aligns with the refactoring in
server/start.go, wherepflagis now explicitly used for constructing the flag set. No further concerns here.server/start.go (6)
32-32: Newpflagimport is consistent with usage.The import is required for dynamically constructing the flag set. This looks good.
63-63: RefactoredStartHandlersignature.Removing the
rootCmdparameter and returning aStartCommandHandlerpromotes clearer separation of concerns and centralizes flag construction in one place. Good approach.
83-83: Delegating tostartInProcessis consistent with the new design.Replacing direct references to an external command with an in-process function call matches the refactoring goal of avoiding
rootCmdusage.
123-123: Updated call tostartNodewithoutrootCmd.The simplified invocation demonstrates the intended decoupling from any externally defined command. No issues found.
261-261: Parameter refactoring forstartNode.Accepting
srvCtxdirectly instead of separate parameters is a clean approach, consolidating the server context in one place.
265-265: Using a dedicated logger for Rollkit.Defining
logger := srvCtx.Logger.With("module", "rollkit")helps isolate logs for debugging the Rollkit node. No issues here.
server/start.go
Outdated
| cmd := &cobra.Command{} | ||
| flags := &pflag.FlagSet{} | ||
| for key, value := range srvCtx.Viper.AllSettings() { | ||
| flags.Set(key, fmt.Sprintf("%v", value)) | ||
| } | ||
| cmd.Flags().AddFlagSet(flags) | ||
|
|
||
| rollkitcfg, err := config.Load(cmd) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Check the error returned by flags.Set.
Ignoring this error may lead to subtle misconfigurations if the flag setting fails. Handle or log the error to improve reliability.
Here is a possible fix:
for key, value := range srvCtx.Viper.AllSettings() {
- flags.Set(key, fmt.Sprintf("%v", value))
+ if err := flags.Set(key, fmt.Sprintf("%v", value)); err != nil {
+ logger.Error("failed to set flag", "key", key, "value", value, "err", err)
+ }
}📝 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.
| cmd := &cobra.Command{} | |
| flags := &pflag.FlagSet{} | |
| for key, value := range srvCtx.Viper.AllSettings() { | |
| flags.Set(key, fmt.Sprintf("%v", value)) | |
| } | |
| cmd.Flags().AddFlagSet(flags) | |
| rollkitcfg, err := config.Load(cmd) | |
| cmd := &cobra.Command{} | |
| flags := &pflag.FlagSet{} | |
| for key, value := range srvCtx.Viper.AllSettings() { | |
| if err := flags.Set(key, fmt.Sprintf("%v", value)); err != nil { | |
| logger.Error("failed to set flag", "key", key, "value", value, "err", err) | |
| } | |
| } | |
| cmd.Flags().AddFlagSet(flags) | |
| rollkitcfg, err := config.Load(cmd) |
🧰 Tools
🪛 golangci-lint (1.64.8)
279-279: Error return value of flags.Set is not checked
(errcheck)
There was a problem hiding this comment.
Copilot reviewed 1 out of 2 changed files in this pull request and generated no comments.
Files not reviewed (1)
- go.mod: Language not supported
Comments suppressed due to low confidence (1)
server/start.go:277
- [nitpick] Consider using pflag.NewFlagSet() instead of directly instantiating a FlagSet, and handle potential errors returned by flags.Set to ensure robust flag parsing.
flags := &pflag.FlagSet{}
Ref: rollkit/go-execution-abci#48 (comment) I eventually went with `LoadFromViper`, as the flags are added in the start command and i would have had to duplicate most of the logic otherwise. `LoadFromViper` should be used for the SDK, as it does some prepossessing of flags. `Load` should be used for any other use case (non SDK based chains).
Overview
Summary by CodeRabbit