move the start sub-command to cobra.#864
Conversation
WalkthroughReplaces the urfave/cli start command with a Cobra-based command structure: adds Viper env binding, moves start logic into a new Cobra start command and wrapper, and renames configuration keys from UPPERCASE underscore style to lowercase hyphen style while adding Cobra flag wiring. Changes
Sequence Diagram(s)mermaid Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 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}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
cmd/arkd/commands.go (1)
20-28: Consider passing subcommand args into Cobra explicitly instead of relying onos.ArgsRight now the wrapper ignores the
*cli.Contextand callsstartCmd.Execute(), which in turn parsesos.Args. This works, but it means patterns likearkd start <args>will leave"start"as a positional arg to Cobra (even if you don’t useargstoday).You could make the behavior more explicit and future‑proof by wiring the urfave args into Cobra:
- startCmdWrapper = &cli.Command{ + startCmdWrapper = &cli.Command{ Name: "start", Usage: "Starts the arkd server", SkipFlagParsing: true, - Action: func(_ *cli.Context) error { - return startCmd.Execute() + Action: func(ctx *cli.Context) error { + startCmd.SetArgs(ctx.Args().Slice()) + return startCmd.Execute() }, }This keeps the wrapper behavior well‑defined even if you later start using Cobra positional args.
cmd/arkd/start.go (1)
29-31: DecouplestartActionfrom*cli.Contextonce urfave/cli is goneThe comment says
startActionis a wrapper for the Cobra command, but its signature still takes a*cli.Context(always passed asnilfrom Cobra). Once you finish the urfave→Cobra migration, consider changing it to:-func startAction(_ *cli.Context) error { +func startAction() error {and updating
RunEaccordingly. That will let you drop the urfave import from this file entirely.internal/config/config.go (1)
335-365: LoadConfig now depends on Viper/flags being preconfigured; consider defaults and error text alignment
LoadConfigassumes that Viper has already been wired with defaults and env/flag bindings (viaSetupFlags+ theroot.goinit). That’s fine for thearkd startcode path, but it’s worth double‑checking two things:
- Behavior when
LoadConfigis used outside the Cobra pathIf any tests, tools, or other code paths call
config.LoadConfig()without having first calledSetupFlags(or without setting Viper defaults),viper.GetString(Datadir)will be empty andinitDatadirwill attempt toMkdirAll(""), which will fail with a somewhat opaque “failed to create datadir” error.If you expect
LoadConfigto be reused outside the Cobra/CLI context, consider also setting Viper defaults inside this package (e.g. in aninitor at the start ofLoadConfig) so it is self‑contained:viper.SetDefault(Datadir, defaultDatadir) // ... other critical defaults if neededor clearly document that
SetupFlagsmust be called beforeLoadConfig.
- Align error messaging with new env/flag naming
In
liveStoreService:redisOpts, err := redis.ParseURL(c.RedisUrl) if err != nil { return fmt.Errorf("invalid REDIS_URL: %w", err) }the error references
REDIS_URL, but the new key isredis-urland the actual env var isARKD_REDIS_URL. Updating this message to reflect the new naming (or to mention both old and new env vars if you keep compatibility) will avoid confusion when debugging misconfigurations.Neither of these change runtime behavior in the happy path, but they make the config API more robust and operator‑friendly.
Also applies to: 788-801
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
cmd/arkd/commands.go(1 hunks)cmd/arkd/main.go(2 hunks)cmd/arkd/root.go(1 hunks)cmd/arkd/start.go(1 hunks)go.mod(3 hunks)internal/config/config.go(3 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
internal/config/config.go (3)
pkg/arkd-wallet/config/config.go (7)
Datadir(25-25)Port(24-24)LogLevel(26-26)OtelCollectorEndpoint(30-30)OtelPushInterval(31-31)PyroscopeServerURL(32-32)Config(84-97)pkg/arkd-wallet-btcwallet/config/config.go (5)
Datadir(18-18)Port(17-17)LogLevel(19-19)EsploraURL(21-21)Config(88-102)internal/interface/grpc/config.go (1)
Config(13-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: integration tests
- GitHub Check: unit tests
🔇 Additional comments (5)
go.mod (1)
47-47: Cobra/pflag dependency additions look consistent; just ensure the module graph is cleanAdding
github.com/spf13/cobra v1.10.2, itsmousetraphelper, and bumpingpflagtov1.0.9is the expected stack for Cobra and should coexist fine with the existing dependencies.I’d still recommend re‑running
go mod tidyand your test suite to confirm there are no unexpected indirect changes or version conflicts introduced by the new CLI wiring.Also applies to: 126-126, 261-261
cmd/arkd/start.go (2)
77-93: Nice touch on augmenting flag help with env variable hintsDecorating each flag’s
Usagewith its correspondingARKD_...environment variable viaenvReplacer+viper.BindPFlagsis a good way to surface configuration without separate docs. This directly addresses the “flag usage is now documented” goal forarkd start --help.
24-56: The gRPC service constructor already callsappConfig.Validate()internally (atinternal/interface/grpc/service.go), so an explicit validation call beforegrpcservice.NewServiceis redundant. Additionally,Config.Validate()only validates configuration fields (db type, scheduler type, etc.) and does not perform service initialization. Services like wallet, signer, and repo are initialized on-demand through getter methods likeWalletService()andAdminService(), not throughValidate(). The current code flow is correct as-is.Likely an incorrect or invalid review comment.
cmd/arkd/main.go (1)
26-44: Wrapper integration preserves defaultstartbehaviorWiring
startCmdWrapperintoapp.Commandsand usingapp.DefaultCommand = startCmd.Usekeepsarkd(without a subcommand) starting the daemon, while also allowingarkd start --helpto go through Cobra and expose the new, documented flags. This matches the compatibility note from the issue while improving per‑command help.cmd/arkd/root.go (1)
9-17: The review comment is incorrect; theARKD_prefix is established public API, not a breaking change.The
viper.SetEnvPrefix("ARKD")configuration at lines 9-17 is the original design, already documented in the README as the standard way to configurearkd. All environment variables (ARKD_DATADIR,ARKD_DB_TYPE,ARKD_PORT, etc.) are publicly documented, and operators are already using them. There is no legacy unprefixed behavior (DB_TYPE,DATADIR) to maintain backward compatibility with—the prefix has been part of the public API from the start.The comment above
envReplaceris also accurate: it correctly describes mapping flags like--db-typeto environment variables (which becomeARKD_DB_TYPEafter the prefix is applied). No changes needed.
This is the first (of multiple) commit that starts the migration from urfave/cli to spf13/cobra so that we have a unified commmand and flag handler. Now flag usage is documented. Closes arkade-os#782
qustavo
force-pushed
the
move_start_cmd_to_cobra
branch
from
96cc19b to
dab0942
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (1)
internal/config/config.go (1)
298-302: Empty help text for several flags.These flags have empty descriptions:
SettlementMinExpiryGap,VtxoNoCsvValidationCutoffDate,OnchainOutputFee,EnablePprof,AlertManagerURL(L314), andPyroscopeServerURL(L332). This reduces CLI usability when users run--help.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (6)
cmd/arkd/commands.go(1 hunks)cmd/arkd/main.go(2 hunks)cmd/arkd/root.go(1 hunks)cmd/arkd/start.go(1 hunks)go.mod(3 hunks)internal/config/config.go(3 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmd/arkd/start.go
- go.mod
🧰 Additional context used
🧬 Code graph analysis (1)
internal/config/config.go (3)
pkg/arkd-wallet/config/config.go (5)
Datadir(25-25)Port(24-24)LogLevel(26-26)LoadConfig(41-82)Config(84-97)pkg/arkd-wallet-btcwallet/config/config.go (6)
Datadir(18-18)Port(17-17)LogLevel(19-19)EsploraURL(21-21)LoadConfig(38-86)Config(88-102)internal/interface/grpc/config.go (1)
Config(13-23)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (3)
- GitHub Check: integration tests
- GitHub Check: Build and Scan
- GitHub Check: unit tests
🔇 Additional comments (4)
cmd/arkd/root.go (1)
9-17: LGTM! Environment variable binding setup is correct.The Viper configuration correctly:
- Sets
ARKDas the environment prefix- Uses
AutomaticEnv()to bind all keys- Replaces
-with_viaSetEnvKeyReplacerto map kebab-case flags (e.g.,--db-type) to environment variables (e.g.,ARKD_DB_TYPE)This aligns well with the new lowercase-hyphen keys in
internal/config/config.go.cmd/arkd/commands.go (1)
20-29: Wrapper pattern for incremental CLI migration looks good.The
SkipFlagParsing: truesetting is essential here—it prevents urfave/cli from consuming flags that should be parsed by Cobra'sstartCmd. The TODO comment clearly documents the transitional nature of this wrapper.internal/config/config.go (1)
156-213: LGTM! Consistent kebab-case naming for configuration keys.The key naming convention is now uniform (lowercase-hyphen), which integrates properly with the Viper environment replacer in
root.gothat maps--db-type→ARKD_DB_TYPE.cmd/arkd/main.go (1)
43-43: No changes needed.startCmd.Useis correctly set to"start"instart.go(line 21), matchingstartCmdWrapper.Name(commands.go line 23). The default command resolves correctly.
| cmd.Flags().Int(Port, DefaultPort, "Port (public) to listen on") | ||
| cmd.Flags().Int(AdminPort, DefaultAdminPort, | ||
| "Admin port (private) to listen on, fallback to service port if 0") |
There was a problem hiding this comment.
Type inconsistency: Port flags registered as Int, read as Uint32.
Port and AdminPort are registered with cmd.Flags().Int() but LoadConfig reads them with viper.GetUint32(). While ports are typically positive, the type mismatch may cause Viper binding issues.
Consider using cmd.Flags().Uint32() for consistency:
- cmd.Flags().Int(Port, DefaultPort, "Port (public) to listen on")
- cmd.Flags().Int(AdminPort, DefaultAdminPort,
+ cmd.Flags().Uint32(Port, uint32(DefaultPort), "Port (public) to listen on")
+ cmd.Flags().Uint32(AdminPort, uint32(DefaultAdminPort),
"Admin port (private) to listen on, fallback to service port if 0")📝 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.Flags().Int(Port, DefaultPort, "Port (public) to listen on") | |
| cmd.Flags().Int(AdminPort, DefaultAdminPort, | |
| "Admin port (private) to listen on, fallback to service port if 0") | |
| cmd.Flags().Uint32(Port, uint32(DefaultPort), "Port (public) to listen on") | |
| cmd.Flags().Uint32(AdminPort, uint32(DefaultAdminPort), | |
| "Admin port (private) to listen on, fallback to service port if 0") |
🤖 Prompt for AI Agents
internal/config/config.go around lines 254 to 256: the flags for Port and
AdminPort are registered with cmd.Flags().Int() but later read with
viper.GetUint32(), causing a type mismatch; change the flag registrations to
cmd.Flags().Uint32() for both Port and AdminPort, and ensure DefaultPort and
DefaultAdminPort are uint32 (or cast to uint32 when passing to the flag) so the
flag types match the viper.GetUint32() usage; leave the AdminPort 0 fallback
behavior unchanged.
| cmd.Flags().Int(UnilateralExitDelay, defaultUnilateralExitDelay, | ||
| "Unilateral exit delay in seconds") | ||
| cmd.Flags().Int(PublicUnilateralExitDelay, defaultUnilateralExitDelay, | ||
| "Public unilateral exit delay in seconds") | ||
| cmd.Flags().Int(CheckpointExitDelay, defaultCheckpointExitDelay, | ||
| "Checkpoint exit delay in seconds") | ||
| cmd.Flags().String(EsploraURL, defaultEsploraURL, "Esplora API URL") | ||
| cmd.Flags().Bool(NoMacaroons, defaultNoMacaroons, "Disable macaroon authentication") | ||
| cmd.Flags().Int(BoardingExitDelay, defaultBoardingExitDelay, "Boarding exit delay in seconds") | ||
| cmd.Flags().Int(RoundMaxParticipantsCount, defaultRoundMaxParticipantsCount, | ||
| "Maximum number of participants per round") | ||
| cmd.Flags().Int(RoundMinParticipantsCount, defaultRoundMinParticipantsCount, | ||
| "Minimum number of participants per round") |
There was a problem hiding this comment.
Type mismatch: Int flags read as Int64 in LoadConfig.
Several flags are registered with cmd.Flags().Int() but LoadConfig reads them with viper.GetInt64():
| Flag | SetupFlags type | LoadConfig getter |
|---|---|---|
UnilateralExitDelay |
Int (L269) |
GetInt64 (L406) |
PublicUnilateralExitDelay |
Int (L271) |
GetInt64 (L407) |
CheckpointExitDelay |
Int (L273) |
GetInt64 (L408) |
BoardingExitDelay |
Int (L277) |
GetInt64 (L409) |
RoundMaxParticipantsCount |
Int (L278) |
GetInt64 (L434) |
RoundMinParticipantsCount |
Int (L280) |
GetInt64 (L435) |
When Viper binds Cobra flags, a type mismatch can cause incorrect value retrieval (returning 0 or truncated values).
Apply this diff to fix the type mismatches:
- cmd.Flags().Int(UnilateralExitDelay, defaultUnilateralExitDelay,
+ cmd.Flags().Int64(UnilateralExitDelay, int64(defaultUnilateralExitDelay),
"Unilateral exit delay in seconds")
- cmd.Flags().Int(PublicUnilateralExitDelay, defaultUnilateralExitDelay,
+ cmd.Flags().Int64(PublicUnilateralExitDelay, int64(defaultUnilateralExitDelay),
"Public unilateral exit delay in seconds")
- cmd.Flags().Int(CheckpointExitDelay, defaultCheckpointExitDelay,
+ cmd.Flags().Int64(CheckpointExitDelay, int64(defaultCheckpointExitDelay),
"Checkpoint exit delay in seconds")
cmd.Flags().String(EsploraURL, defaultEsploraURL, "Esplora API URL")
cmd.Flags().Bool(NoMacaroons, defaultNoMacaroons, "Disable macaroon authentication")
- cmd.Flags().Int(BoardingExitDelay, defaultBoardingExitDelay, "Boarding exit delay in seconds")
- cmd.Flags().Int(RoundMaxParticipantsCount, defaultRoundMaxParticipantsCount,
+ cmd.Flags().Int64(BoardingExitDelay, int64(defaultBoardingExitDelay), "Boarding exit delay in seconds")
+ cmd.Flags().Int64(RoundMaxParticipantsCount, int64(defaultRoundMaxParticipantsCount),
"Maximum number of participants per round")
- cmd.Flags().Int(RoundMinParticipantsCount, defaultRoundMinParticipantsCount,
+ cmd.Flags().Int64(RoundMinParticipantsCount, int64(defaultRoundMinParticipantsCount),
"Minimum number of participants per round")📝 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.Flags().Int(UnilateralExitDelay, defaultUnilateralExitDelay, | |
| "Unilateral exit delay in seconds") | |
| cmd.Flags().Int(PublicUnilateralExitDelay, defaultUnilateralExitDelay, | |
| "Public unilateral exit delay in seconds") | |
| cmd.Flags().Int(CheckpointExitDelay, defaultCheckpointExitDelay, | |
| "Checkpoint exit delay in seconds") | |
| cmd.Flags().String(EsploraURL, defaultEsploraURL, "Esplora API URL") | |
| cmd.Flags().Bool(NoMacaroons, defaultNoMacaroons, "Disable macaroon authentication") | |
| cmd.Flags().Int(BoardingExitDelay, defaultBoardingExitDelay, "Boarding exit delay in seconds") | |
| cmd.Flags().Int(RoundMaxParticipantsCount, defaultRoundMaxParticipantsCount, | |
| "Maximum number of participants per round") | |
| cmd.Flags().Int(RoundMinParticipantsCount, defaultRoundMinParticipantsCount, | |
| "Minimum number of participants per round") | |
| cmd.Flags().Int64(UnilateralExitDelay, int64(defaultUnilateralExitDelay), | |
| "Unilateral exit delay in seconds") | |
| cmd.Flags().Int64(PublicUnilateralExitDelay, int64(defaultUnilateralExitDelay), | |
| "Public unilateral exit delay in seconds") | |
| cmd.Flags().Int64(CheckpointExitDelay, int64(defaultCheckpointExitDelay), | |
| "Checkpoint exit delay in seconds") | |
| cmd.Flags().String(EsploraURL, defaultEsploraURL, "Esplora API URL") | |
| cmd.Flags().Bool(NoMacaroons, defaultNoMacaroons, "Disable macaroon authentication") | |
| cmd.Flags().Int64(BoardingExitDelay, int64(defaultBoardingExitDelay), "Boarding exit delay in seconds") | |
| cmd.Flags().Int64(RoundMaxParticipantsCount, int64(defaultRoundMaxParticipantsCount), | |
| "Maximum number of participants per round") | |
| cmd.Flags().Int64(RoundMinParticipantsCount, int64(defaultRoundMinParticipantsCount), | |
| "Minimum number of participants per round") |
🤖 Prompt for AI Agents
internal/config/config.go around lines 269 to 281, several flags are registered
with cmd.Flags().Int() but later read with viper.GetInt64(), causing a type
mismatch; change the flag registrations for UnilateralExitDelay,
PublicUnilateralExitDelay, CheckpointExitDelay, BoardingExitDelay,
RoundMaxParticipantsCount, and RoundMinParticipantsCount to use
cmd.Flags().Int64(...) (matching the GetInt64 getters) so the flag types align
with Viper reads and preserve 64-bit values.
|
@qustavo sorry for the late reply 🙏. Can we go directly for the migration from urfave/cli to cobra? This way we don't need to have "wrappers" |
We could but that would turn the PR into a greater change which makes it harder to properly review it. To better understand your motivation, what's the downside of a wrapper (which will eventually be removed) |
I think it will still lead to a big pr once we remove the wrappers? In this case, the wrappers seem like unnecessary extra work? |
|
It might be more code changes, split in smaller PRs, IMO these are easier to reason about hence to review, also it should minimize branch conflicts. |
2 participants
This is the first (of multiple) commit that starts the migration from urfave/cli to spf13/cobra so that we have a unified commmand and flag handler.
Now flag usage is documented.
Closes #782
Summary by CodeRabbit
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.