refactor: simplify cli args handling - #744
Conversation
WalkthroughThe changes introduce a new generic function Changes
Sequence Diagram(s)sequenceDiagram
participant CMD as Cobra Command
participant LC as LoadConfig[ServeConfig]
participant V as Viper & Unmarshal
participant SC as ServeCommand Setup
CMD->>LC: Call LoadConfig[ServeConfig](cmd)
LC->>V: Bind flags and unmarshal config
V-->>LC: Return ServeConfig or error
LC-->>CMD: Return config/error
CMD->>SC: Process configuration and setup command
sequenceDiagram
participant CMD as Cobra Command
participant LC as LoadConfig[WorkerConfiguration]
participant V as Viper & Unmarshal
participant WC as WorkerCommand Setup
CMD->>LC: Call LoadConfig[WorkerConfiguration](cmd)
LC->>V: Bind flags and unmarshal config
V-->>LC: Return WorkerConfiguration or error
LC-->>CMD: Return config/error
CMD->>WC: Initialize worker with configuration values
Poem
✨ Finishing Touches
🪧 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 (
|
077e095 to
cfc9431
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/config.go (1)
9-21: Well-designed generic configuration loader functionThis is a great use of Go generics to create a reusable configuration loading pattern. The function properly handles error cases and provides context in error messages.
A small suggestion for improved spacing: consider adding a space between the closing parenthesis and opening brace in the function signature.
-func LoadConfig[V any](cmd *cobra.Command) (*V, error){ +func LoadConfig[V any](cmd *cobra.Command) (*V, error) {cmd/serve.go (1)
172-176: Clearly marked deprecated flagsIt's good practice to mark deprecated flags with a comment. Consider adding explicit deprecation notices using cobra's MarkFlagDeprecated to provide more information to users.
// Deprecated flags cmd.Flags().Bool(ExperimentalFeaturesFlag, false, "Enable features configurability") +cmd.Flags().MarkDeprecated(ExperimentalFeaturesFlag, "This flag will be removed in a future version") cmd.Flags().Bool(NumscriptInterpreterFlag, false, "Enable experimental numscript rewrite") +cmd.Flags().MarkDeprecated(NumscriptInterpreterFlag, "This flag will be removed in a future version") cmd.Flags().String(NumscriptInterpreterFlagsToPass, "", "Feature flags to pass to the experimental numscript interpreter") +cmd.Flags().MarkDeprecated(NumscriptInterpreterFlagsToPass, "This flag will be removed in a future version")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
go.modis excluded by!**/*.modgo.sumis excluded by!**/*.sum,!**/*.sumtools/generator/go.sumis excluded by!**/*.sum,!**/*.sum
📒 Files selected for processing (3)
cmd/config.go(1 hunks)cmd/serve.go(5 hunks)cmd/worker.go(3 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (8)
cmd/worker.go (3)
21-24: Good struct design for worker configurationThe struct fields use proper mapstructure tags that match the flag names defined as constants, making the mapping between CLI flags and configuration clear.
41-44: Improved error handling with LoadConfigThe refactored code uses the generic LoadConfig function to handle configuration, improving error handling with proper error wrapping.
52-56: Storage module configuration simplificationThe storage module initialization has been simplified, and worker configuration is now properly sourced from the loaded config struct. This makes the code more maintainable.
cmd/serve.go (5)
40-54: Well-structured ServeConfig with embedded WorkerConfigurationThe ServeConfig struct effectively consolidates all configuration parameters with proper mapstructure tags. Embedding WorkerConfiguration is a good approach to reuse the worker settings.
78-81: Consistent error handling patternGood use of the generic LoadConfig function with proper error wrapping that follows the same pattern used in worker.go.
97-98: Configuration access is now more consistentConfiguration values are now consistently accessed from the cfg struct, making the code more maintainable and easier to understand.
Also applies to: 100-104, 109-110, 112-112, 117-119, 121-123
147-147: Server binding uses configuration valueThe HTTP server binding address is now correctly accessed from the configuration struct.
154-156: Proper use of embedded WorkerConfigurationThe worker module correctly uses the HashLogsBlockCRONSpec and HashLogsBlockMaxSize from the embedded WorkerConfiguration structure.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #744 +/- ##
==========================================
- Coverage 82.01% 81.96% -0.06%
==========================================
Files 135 136 +1
Lines 7289 7263 -26
==========================================
- Hits 5978 5953 -25
Misses 1006 1006
+ Partials 305 304 -1 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
4d96ad5 to
c425e36
Compare
c425e36 to
ab4361d
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
cmd/serve.go (2)
52-54: Consider documenting experimental featuresThe experimental features flags such as
NumscriptInterpreter,NumscriptInterpreterFlags, andExperimentalFeaturesEnabledare included in the configuration struct, but it might be helpful to add comments explaining their purpose and implications.
171-173: Ensure deprecated flags are properly documentedThese flags have been moved to the configuration struct but are still added directly to the command. Consider adding a deprecation notice using
cmd.Flag(ExperimentalFeaturesFlag).Deprecated = "Use configuration file instead"to inform users about the preferred way to configure these features.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
cmd/serve.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (6)
cmd/serve.go (6)
40-55: Well-structured configuration consolidationThe new
ServeConfigstruct nicely consolidates all configuration parameters into a single type, making the configuration management more maintainable and type-safe. The use ofmapstructuretags ensures proper mapping between configuration keys and struct fields.
41-41: Good use of composition with WorkerConfigurationEmbedding the
WorkerConfigurationstruct with thesquashoption is a clean approach to reuse worker-related configuration without duplicating fields.
79-82: Improved configuration loading with type safetyThe generic
LoadConfig[ServeConfig]provides better type safety compared to manually retrieving flags. The error handling has also been improved with a more descriptive error message usingfmt.Errorf.
98-99: Consistent use of configuration struct fieldsThe code now consistently uses fields from the configuration struct instead of directly accessing flags, which improves maintainability and reduces the chance of errors from mismatched flag names.
Also applies to: 101-103, 104-105, 110-111, 113-114, 118-120, 122-124
152-157: Clean conditional module inclusionThe worker module is conditionally included based on the configuration, which is a clean approach. The use of the embedded
WorkerConfigurationfields is also consistent with the rest of the code.
127-136: Improved struct field formattingThe dependency injection parameter struct has better formatting with each field on its own line, which improves readability.
1ae89f5 to
7aa8c84
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
cmd/serve.go (1)
171-173: Appropriate handling of deprecated flags.Keeping these flags while marking them as deprecated provides a good migration path for users. Consider adding deprecation notices in logs or documentation to encourage migration to the new configuration approach.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
go.modis excluded by!**/*.mod
📒 Files selected for processing (1)
cmd/serve.go(5 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (1)
- GitHub Check: Tests
🔇 Additional comments (4)
cmd/serve.go (4)
40-55: Well-structured configuration consolidation.This new
ServeConfigstruct nicely consolidates all configuration parameters into a single, coherent structure. The embeddedWorkerConfigurationis a good use of composition, and the mapstructure tags ensure correct binding with Viper.
79-82: Good use of generics for type-safe configuration loading.The implementation of the generic
LoadConfigfunction improves type safety and reduces boilerplate code. The error handling is also improved with a more descriptive error message.
98-111: Consistent use of configuration object.The code now consistently references the
cfgobject properties rather than accessing flags directly, which improves maintainability and readability.
152-157:Details
❓ Verification inconclusive
Verify worker configuration mapping.
Ensure that the fields from embedded
WorkerConfigurationcorrectly map to the worker module config. Consider adding a test to verify this mapping works as expected.
🏁 Script executed:
#!/bin/bash # Check how WorkerConfiguration is defined and its relationship to worker.ModuleConfig grep -r "type WorkerConfiguration" .Length of output: 149
Review: Confirm Worker Configuration Mapping in cmd/serve.go
The current mapping from
cfg.WorkerConfigurationto theworker.ModuleConfiginsidecmd/serve.go(lines 152–157) appears to be correct—assigningHashLogsBlockCRONSpectoScheduleandHashLogsBlockMaxSizetoMaxBlockSize. Note that the definition ofWorkerConfigurationis present in both./pkg/testserver/worker.goand./cmd/worker.go; please ensure these definitions consistently include the expected fields.As a safeguard against future regressions, consider adding a unit test that verifies any changes in the configuration are accurately reflected in the worker module configuration.
3 participants
No description provided.