Frrist/fx config instance #3954
Conversation
- TODO
- Doesn't compile because much code still depends on global config.
- address gloabl dep on config with dep injection
- address root gloabl flag binding on config instance see todo in
root.go command.
WalkthroughThe recent overhaul in the codebase focuses on enhancing functionality across various modules including node management, data storage, and public APIs. Key developments include the introduction of new provider functions for compute operations, restructuring of node configuration and management, and the refinement of storage and publisher configurations. The changes aim to streamline operations, improve scalability, and boost performance in handling complex network and compute tasks. Changes
Poem
Recent Review DetailsConfiguration used: CodeRabbit UI Files ignored due to path filters (1)
Files selected for processing (146)
Files not reviewed due to errors (15)
Files skipped from review due to trivial changes (3)
Additional comments not posted (247)
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 as PR comments)
Additionally, you can add CodeRabbit Configration File (
|
|
@coderabbitai review |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 33
Out of diff range and nitpick comments (6)
pkg/node/labels.go (1)
23-27: RenamingstaticLabelstoStaticLabelsincreases accessibility and flexibility in using the labels.Consider adding a comment or documentation to clarify the intended use of the
StaticLabelsfield, especially to caution against unintended modifications.pkg/repo/fs.go (1)
138-138: Ensure proper logging during repository initialization.Consider adding more detailed logging during the initialization process to help with debugging and tracking the setup steps.
pkg/nodefx/requester/node.go (1)
42-42: Modularization TODO comment.The TODO comment about further modularizing the
Modulevariable is a good reminder to keep the code clean and maintainable. Consider prioritizing this task to improve the structure of the requester node setup.pkg/nodefx/compute/node.go (1)
354-354: TODO comment suggests renaming theStartmethod toRunsince it blocks.Consider renaming the
Startmethod toRunto better indicate that it blocks execution.pkg/config/types/generated_viper_defaults.go (2)
58-75: Consider adding comments to describe each configuration block.Adding comments for each configuration block can improve the readability and maintainability of the code, especially for new developers or when revisiting this code after some time.
122-127: Review the naming consistency of configuration keys.The naming of some configuration keys, such as
NodeComputeJobSelectionPolicyProbeHTTP, might benefit from a review to ensure they are consistent and intuitive across the configuration file.
| p.Viper.Set(NodeServerAPITLSServerCertificate, cfg.Node.ServerAPI.TLS.ServerCertificate) | ||
| p.Viper.Set(NodeServerAPITLSServerKey, cfg.Node.ServerAPI.TLS.ServerKey) | ||
| p.Viper.Set(NodeServerAPITLSSelfSigned, cfg.Node.ServerAPI.TLS.SelfSigned) | ||
| p.Viper.Set(NodeServerMiddlewareConfig, cfg.Node.ServerMiddlewareConfig) | ||
| p.Viper.Set(NodeServerMiddlewareConfigHeaders, cfg.Node.ServerMiddlewareConfig.Headers) | ||
| p.Viper.Set(NodeServerMiddlewareConfigReadTimeout, cfg.Node.ServerMiddlewareConfig.ReadTimeout.AsTimeDuration()) | ||
| p.Viper.Set(NodeServerMiddlewareConfigRequestHandlerTimeout, cfg.Node.ServerMiddlewareConfig.RequestHandlerTimeout.AsTimeDuration()) | ||
| p.Viper.Set(NodeServerMiddlewareConfigMaxBytesToReadInBody, cfg.Node.ServerMiddlewareConfig.MaxBytesToReadInBody) | ||
| p.Viper.Set(NodeServerMiddlewareConfigThrottleLimit, cfg.Node.ServerMiddlewareConfig.ThrottleLimit) | ||
| p.Viper.Set(NodeServerMiddlewareConfigLogLevel, cfg.Node.ServerMiddlewareConfig.LogLevel) | ||
| p.Viper.Set(NodeServerMiddlewareConfigMigrations, cfg.Node.ServerMiddlewareConfig.Migrations) | ||
| p.Viper.Set(NodeServerMiddlewareConfigDebug, cfg.Node.ServerMiddlewareConfig.Debug) | ||
| p.Viper.Set(NodeServer, cfg.Node.Server) | ||
| p.Viper.Set(NodeServerAddress, cfg.Node.Server.Address) | ||
| p.Viper.Set(NodeServerPort, cfg.Node.Server.Port) | ||
| p.Viper.Set(NodeServerAutoCertDomain, cfg.Node.Server.AutoCertDomain) | ||
| p.Viper.Set(NodeServerAutoCertCache, cfg.Node.Server.AutoCertCache) | ||
| p.Viper.Set(NodeServerTLSCertificateFile, cfg.Node.Server.TLSCertificateFile) | ||
| p.Viper.Set(NodeServerTLSKeyFile, cfg.Node.Server.TLSKeyFile) | ||
| p.Viper.Set(NodeServerReadHeaderTimeout, cfg.Node.Server.ReadHeaderTimeout.AsTimeDuration()) | ||
| p.Viper.Set(NodeServerWriteTimeout, cfg.Node.Server.WriteTimeout.AsTimeDuration()) | ||
| p.Viper.Set(NodeLibp2p, cfg.Node.Libp2p) | ||
| p.Viper.Set(NodeLibp2pSwarmPort, cfg.Node.Libp2p.SwarmPort) | ||
| p.Viper.Set(NodeLibp2pPeerConnect, cfg.Node.Libp2p.PeerConnect) |
There was a problem hiding this comment.
📝 NOTE
This review was outside the diff hunks and was mapped to the diff hunk with the greatest overlap. Original lines [278-325]
Duplicate settings in SetDefaults and Set functions.
The configuration settings are duplicated in both SetDefaults and Set functions. This could lead to maintenance issues and inconsistencies. Consider refactoring to avoid duplication.
| // TODO(forrest) [refactor]: why do we do this, should be removed as this is just a random env var | ||
| // will remove when review is okayed. | ||
| /* | ||
| if !h.keepStack { | ||
| h.logger.Info().Msg("removing container") | ||
| if err := h.client.RemoveContainer(ctx, h.containerID); err != nil { | ||
| return err | ||
| } | ||
| return h.client.RemoveObjectsWithLabel(ctx, labelExecutionID, labelExecutionValue(h.ID, h.executionID)) | ||
| } | ||
| return h.client.RemoveObjectsWithLabel(ctx, labelExecutionID, labelExecutionValue(h.ID, h.executionID)) | ||
| } | ||
|
|
||
| */ |
There was a problem hiding this comment.
Remove commented-out code block.
The commented-out code block related to keepStack should be removed to clean up the codebase, as per the changes mentioned in the summary.
| if err := config.ForKey(types.NodeCompute, &cfg); err != nil { | ||
| return node.ComputeConfig{}, err | ||
| } | ||
| panic("deprecate me") |
There was a problem hiding this comment.
Replace panic with a more graceful deprecation method.
Using panic to deprecate functions is not user-friendly and can cause abrupt application termination. Consider using a logging mechanism or a deprecation warning instead.
| if err := config.ForKey(types.NodeRequester, &cfg); err != nil { | ||
| return node.RequesterConfig{}, err | ||
| } | ||
| panic("deprecate me") |
There was a problem hiding this comment.
Replace panic with a more graceful deprecation method.
Similar to the GetComputeConfig function, replace the panic in GetRequesterConfig with a more graceful deprecation method to enhance user experience and avoid abrupt application termination.
| func getNodeType(c *config.Config) (requester, compute bool, err error) { | ||
| requester = false | ||
| compute = false | ||
| err = nil | ||
|
|
||
| nodeType := viper.GetStringSlice(types.NodeType) | ||
| var nodeType []string | ||
| err = c.ForKey(types.NodeType, &nodeType) | ||
| if err != nil { | ||
| return false, false, err | ||
| } |
There was a problem hiding this comment.
Validate node types more robustly.
The current validation for node types only supports "compute" and "requester" and throws an error for any other type. Consider allowing a broader range of valid node types or providing a clearer message about supported types.
| func LoadConfig(c *config.Config) (types.AuthConfig, error) { | ||
| var cfg types.AuthConfig | ||
| if err := c.ForKey(types.Auth, &cfg); err != nil { | ||
| return types.AuthConfig{}, nil | ||
| } | ||
|
|
||
| return cfg, nil | ||
| } |
There was a problem hiding this comment.
Consider improving error handling in LoadConfig.
The function returns an empty AuthConfig and swallows the error if c.ForKey fails. This could lead to silent failures. Consider logging the error or propagating it upwards.
- return types.AuthConfig{}, nil
+ return types.AuthConfig{}, errCommittable 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.
| func LoadConfig(c *config.Config) (types.AuthConfig, error) { | |
| var cfg types.AuthConfig | |
| if err := c.ForKey(types.Auth, &cfg); err != nil { | |
| return types.AuthConfig{}, nil | |
| } | |
| return cfg, nil | |
| } | |
| func LoadConfig(c *config.Config) (types.AuthConfig, error) { | |
| var cfg types.AuthConfig | |
| if err := c.ForKey(types.Auth, &cfg); err != nil { | |
| return types.AuthConfig{}, err | |
| } | |
| return cfg, nil | |
| } |
| // TODO(forrest) [fixme] | ||
| return | ||
| /* |
There was a problem hiding this comment.
Restore or clarify the future of StartUpdateCheck.
The function has been modified to effectively disable its original update checking functionality. Consider restoring the original functionality or providing a clear plan and timeline for its replacement.
| return fmt.Errorf("DEVELOPER ERROR: please file an issue with this error message: %w", err) | ||
| } | ||
| if !strictVersioning { | ||
| panic("TODO") |
There was a problem hiding this comment.
Incomplete implementation of CheckVersion function. Please complete the implementation or provide a timeline for its completion.
| // TODO(forrest) [refactor]: the ideal case here would be to provide a labelsProvider and have a separate method | ||
| // to provide the NodeStateProvider that accepts the labels provider as input. The labelsProvider could be provided | ||
| // outside of this file, possibly in nodefx/node.go |
There was a problem hiding this comment.
Consider implementing the suggested refactor regarding labelsProvider to enhance modularity and testability.
| } | ||
|
|
||
| func RegisterNodeInfoProviderDecorators(transport *nats_transport.NATSTransport, provider *routing.NodeStateProvider) { | ||
| // TODO(forrest) [refactor]: this this PR needs to support libp2p then the transport layer will need to be an interface. |
There was a problem hiding this comment.
Consider implementing the suggested refactor to support different transport layers as indicated by the TODO comment.
|
This was a helpful spike. Closing in favor of #3959 which extracted a subset of the work from this relating to configuration. A second PR will follow which will include the dependency injection via fx as was sketched out here. |
Summary by CodeRabbit
New Features
Refactor
Bug Fixes
Documentation