refactor: define config instance. config is no longer global #3959
Conversation
|
Important Review SkippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the WalkthroughThe overarching change involves integrating a Changes
Possibly related issues
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 (
|
|
We should be really thoughtful as we fix tests. Every fix is an incompatibility with the way it worked previously - not that it's bad just let's be thoughtful about the user experience |
frrist
force-pushed
the
frrist/config/remove-global
branch
from
a430cfe
to
c8cfb60
Compare
|
@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: 35
Out of diff range and nitpick comments (12)
cmd/util/hook/hooks.go (1)
71-73: The hook management functions are well-implemented, providing clear and effective handling of command hooks. Note the TODO comment for future improvements.Consider addressing the TODO comment to improve the management of update checks.
cmd/cli/version/version.go (1)
Line range hint
46-55: Refactor to handle potential errors fromrunVersion.RunE: func(cmd *cobra.Command, _ []string) error { - return runVersion(cmd, cfg, oV) + err := runVersion(cmd, cfg, oV) + if err != nil { + return fmt.Errorf("error running version command: %w", err) + } + return nil },pkg/storage/repo/storage_test.go (1)
Line range hint
66-114: Consider adding a network connectivity check in theTestCloneRepofunction to handle offline scenarios and avoid flaky tests.cmd/cli/cancel/cancel.go (1)
Line range hint
77-119: Refactor to improve readability and error handling incancelfunction.- if requestedJobID == "" { - var byteResult []byte - byteResult, err = util.ReadFromStdinIfAvailable(cmd) - if err != nil { - return fmt.Errorf("unknown error reading from file: %s", err) - } - requestedJobID = string(byteResult) - } + requestedJobID, err = util.GetRequestedJobID(cmd, requestedJobID) + if err != nil { + return err + }Extract the logic for reading the job ID from stdin into a separate function to improve readability and maintainability.
pkg/config/config.go (1)
19-30: Ensure that theContextinterface is thoroughly documented.Adding detailed comments to the interface methods can improve code readability and maintainability, especially for methods like
SetIfAbsentandForKeywhich might not be immediately clear to new developers.cmd/cli/root.go (1)
Line range hint
45-93: Ensure proper error handling inPersistentPreRunEandPersistentPostRunE.+ if err := configflags.BindFlags(cmd, cfg, rootFlags); err != nil { + return err + } + if err := cfg.System().BindPFlag("repo", RootCmd.PersistentFlags().Lookup("repo")); err != nil { + return err + } + if err := cfg.System().BindEnv("repo", "BACALHAU_DIR"); err != nil { + return err + }Adding error checks in these hooks ensures that any configuration or binding errors are caught early, preventing runtime issues.
cmd/cli/config/set.go (1)
Line range hint
74-162: RefactorsetConfigto improve readability and error handling.Consider breaking down the
setConfigfunction into smaller, more focused functions. This can improve readability and make the code easier to maintain. Additionally, ensure that all potential error paths are properly handled and that errors provide enough context for debugging.cmd/cli/config/auto.go (1)
Line range hint
94-162: RefactorautoConfigto improve clarity and maintainability.Consider refactoring the
autoConfigfunction to separate the logic for retrieving system capacity and setting resource limits. This can make the code clearer and easier to maintain, especially as the complexity of the resource calculation logic increases.pkg/config/helpers.go (1)
26-42: Consider adding detailed documentation for the genericGetfunction to explain its usage and the type constraints.pkg/test/requester/retries_test.go (2)
Line range hint
52-147: RefactorSetupSuiteto improve readability and maintainability. Consider breaking down this large setup function into smaller, more focused helper functions.
Line range hint
52-147: Consider using table-driven tests forTestRetryto enhance clarity and reduce redundancy. This approach can make it easier to add new test cases in the future.pkg/executor/docker/executor_test.go (1)
62-62: Ensure that theNewExecutorfunction is properly documented, especially since it now requires aconfig.Contextparameter. This is crucial for understanding how configuration is passed and managed within the executor.
frrist
force-pushed
the
frrist/config/remove-global
branch
from
d52f630
to
bcecd20
Compare
cmd/cli/root.go
Outdated
| repoDir, err := config.Get[string]("repo") | ||
| if err != nil { | ||
| util.Fatal(cmd, fmt.Errorf("failed to read --repo value: %w", err), 1) | ||
| } | ||
| if repoDir == "" { | ||
| // this error indicates `defaultRepo` was unable to find a default location and the user | ||
| // didn't provide on. | ||
| util.Fatal(cmd, fmt.Errorf("bacalhau repo not set, please use BACALHAU_DIR or --repo"), 1) | ||
| } | ||
| if _, err := setup.SetupBacalhauRepo(repoDir); err != nil { | ||
| util.Fatal(cmd, fmt.Errorf("failed to initialize bacalhau repo at '%s': %w", repoDir, err), 1) | ||
| } | ||
|
|
||
| if err := configflags.BindFlags(cmd, rootFlags); err != nil { | ||
| if err := configflags.BindFlags(cmd, cfg, rootFlags); err != nil { |
There was a problem hiding this comment.
Review: We no longer create a config when the bacalhau binary runs. We instead create or open a repo in the places it's needed.
cmd/cli/root.go
Outdated
| util.Fatal(RootCmd, err, 1) | ||
| } | ||
| if err := viper.BindEnv("repo", "BACALHAU_DIR"); err != nil { | ||
| if err := cfg.System().BindEnv("repo", "BACALHAU_DIR"); err != nil { |
There was a problem hiding this comment.
The repo path is part of the 'System' config. And currently is the only value in that instance. Users are still free to specify a path for their repo.
There was a problem hiding this comment.
as users can define the repo using --repo flag and BACALHAU_DIR env variable, and they should be able to configure the repo path, this doesn't sound like a system only config
cmd/cli/serve/serve_test.go
Outdated
| // this is required for the below line to succeed as environment is being deprecated. | ||
| config.Set(configenv.Testing) | ||
| peers, err = serve.GetPeers("env") | ||
| s.T().Skip("we are deprecating libp2p, skipping") |
There was a problem hiding this comment.
Review: we are deprecating Libp2p very shortly and I didn't want to bother trying to fix the compilation issues with this test because of that. Any objections?
There was a problem hiding this comment.
unless you are planning to delay merging this PR until libp2p is fully deprecated, which you shouldn't, then we shouldn't skip this test. Though to help me understand more, is the effort to fix it really significant? How is the issue related to libp2p and not to changes in the config setup?
There was a problem hiding this comment.
comment not addressed yet
cmd/util/api.go
Outdated
| // TODO(forrest) [refactor]: a repo is required here as it contains the clients | ||
| // private key and a pk is needed in order to perform the authentication flow | ||
| // iff the configured authentication mode is challenge. We could decide to only | ||
| // require a repo here if the client _wants_ to communicate with a requester | ||
| // that uses authorization. In the event a requester doesn't need authorization | ||
| // we could use a un-authenticated client and not open/init a repo here. |
There was a problem hiding this comment.
I would prefer to make the repo responsible for managing client keys. Sensitive items should not be stored in the config. This TODO lays out the intention for this change.
| // `authn.MethodTypeAsk`. Reads the JSON Schema returned by the `ask` endpoint | ||
| // and uses it to ask appropriate questions to the user on their terminal, and | ||
| // then returns their response as serialized JSON. | ||
| func askResponder(cmd *cobra.Command) responder { |
There was a problem hiding this comment.
this wasn't deleted, rather moved to a Responder interface.
| if dup, ok := seen[def.ConfigPath]; ok { | ||
| return fmt.Errorf("DEVELOPER ERROR: duplicate regsistration of config key %s for flag %s"+ | ||
| " previously registered on on flag %s", def.ConfigPath, def.FlagName, dup.FlagName) | ||
| } | ||
| seen[def.ConfigPath] = def |
There was a problem hiding this comment.
added to prevent bugs like the one in cmd/util/flags/configflags/timeouts.go from going unnoticed.
cmd/util/hook/hooks.go
Outdated
| Adapt(StartUpdateCheck), | ||
| // TODO(forrest) [fixme/are-you-fucking-kidding-me]: need to un-wang this from here | ||
| // gut check idea is to perform this in a single place at the root level | ||
| // Adapt(StartUpdateCheck), |
There was a problem hiding this comment.
This whole thing is really gross, especially now that we have moved away from a global config structure. In order to run the update checker we need an API client so we can ask the server what it's version is and then notify the user that the server they are talking to is out of date. I don't think we should notify users their server is out of date - I beleive the utility of this serve should be to tell users their binary is out of date. Further, doing all this in a pre-run hook is really complicated since it requires a client , TLS config, and api token to connect to the server and authenticate.
We still run an update checker that notifies users when they run serve but with this commented out we won't notify users when they run client commands.
I think a the follow on change here ought to be:
- Dial get.bacalhau.org to get the latest version
- if the version is newer than the current binary notify, else do nothing.
There was a problem hiding this comment.
Looking at StartUpdateCheck, checking the server version is best effort and there are other value in running the update checker even if we can't reach the server.
If you follow my advice about passing the client to commands through context or by using custom command types, you should be able to handle this gracefully in StartUpdateCheck by only calling a server if a client exists
cmd/util/tokens.go
Outdated
There was a problem hiding this comment.
Review: logic here is unchanged, difference is rather than lazily rely on the global config we instead provide a path to read and write token from as an argument.
pkg/config/config.go
Outdated
| func (c *config) Load(path string) error { | ||
| if _, err := os.Stat(path); os.IsNotExist(err) { | ||
| // if the config file doesn't exist then we obviously cannot load it | ||
| return fmt.Errorf("config file not found at at path: %q", path) |
There was a problem hiding this comment.
Load now requires a config to be present at the path, if no file is there is returns an error.
| ConfigFileMode = 0666 | ||
| ) | ||
|
|
||
| func Init(path string) (types.BacalhauConfig, error) { |
There was a problem hiding this comment.
Removed as calling config.New() initializes a config
| func getDefaultConfig(path string) types.BacalhauConfig { | ||
| // derive the default config for the specified environment. | ||
| defaultConfig := ForEnvironment() | ||
|
|
||
| // set default values for path dependent config. | ||
| defaultConfig.User.KeyPath = filepath.Join(path, UserPrivateKeyFileName) | ||
| defaultConfig.User.Libp2pKeyPath = filepath.Join(path, Libp2pPrivateKeyFileName) | ||
| defaultConfig.Node.ExecutorPluginPath = filepath.Join(path, PluginsPath) | ||
| defaultConfig.Node.ComputeStoragePath = filepath.Join(path, ComputeStoragesPath) | ||
| defaultConfig.Node.Compute.ExecutionStore.Path = filepath.Join(path, ComputeExecutionsStorePath) | ||
| defaultConfig.Node.Requester.JobStore.Path = filepath.Join(path, OrchestratorJobStorePath) | ||
| defaultConfig.Update.CheckStatePath = filepath.Join(path, UpdateCheckStatePath) | ||
| defaultConfig.Auth.TokensPath = filepath.Join(path, TokensPath) | ||
|
|
||
| // We default to the folder which contains the job store, and add | ||
| // a subfolder for the network store. | ||
| defaultConfig.Node.Network.StoreDir = filepath.Join( | ||
| filepath.Dir(defaultConfig.Node.Requester.JobStore.Path), | ||
| NetworkTransportStore, | ||
| ) | ||
|
|
||
| return defaultConfig |
There was a problem hiding this comment.
this logic is now the responsibility of the repo.
There was a problem hiding this comment.
@wdbaruni the behavior remains unmodified here but would be good to review closely
There was a problem hiding this comment.
this has been a very long and difficult PR to review, and I don't think I was able to cover all the details. There are major changes suggested, but still we need to break this PR before we can merge it even after addressing those proposals.
As a start, you can create a PR where global configs are not used beyond node package and that node package is responsible in preparing and injecting all configs required by other components, such as publishers, executors and downloaders
Second PR can be my suggestion to decouple configs from viper and pass BacalhauConfig around and to the node package
Another is passing the client to commands as suggested, and so many ways to break into more PRs
cmd/cli/config/auto.go
Outdated
| // TODO(forrest) [fixme] | ||
| func newAutoResourceCmd(cfg config.Context) *cobra.Command { |
There was a problem hiding this comment.
It's a TODO I left to remind myself to fix something and I forgot to remove the comment before pushing. Will remove.
cmd/cli/config/auto.go
Outdated
| // create or open a repo | ||
| repoPath, err := cfg.RepoPath() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| _, err = setup.SetupBacalhauRepo(repoPath, cfg) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
why do you need to initialize a repo? You are already passing a config to newAutoResourceCmd. How was that initialized? and how initializing the repo will change the config?
There was a problem hiding this comment.
why do you need to initialize a repo
Since we no longer create a repo in the root command, we must create on here if it's not present. This is because this CLI method modifies the config file in a repo, creating a config file when one isn't present.
You are already passing a config to newAutoResourceCmd. How was that initialized?
As was done prior to this change - the config instance is created with default values
see pkg/config/config.go
func New(opts ...Option) *config {
c := &config{
user: viper.New(),
system: viper.New(),
defaultCfg: configenv.Production,
}
for _, opt := range opts {
opt(c)
}
c.user.SetEnvPrefix(environmentVariablePrefix)
c.user.SetTypeByDefaultValue(inferConfigTypes)
c.user.AutomaticEnv()
c.user.SetEnvKeyReplacer(environmentVariableReplace)
c.setDefault(c.defaultCfg)
return c
}and how initializing the repo will change the config?
This is slightly different than before:
Prior to this change, the config instance set paths on itself that the repo would be configured based on.
Now when initializing the repo, it modifies the config by setting paths on the config instance via the method EnsureRepoPathsConfigured.
This change was implemented in this manner because we discussed removing the option for users to set all paths on the configurations themselves that fell within the repo. Instead, the repository now manages the paths it sets, with the top-level path being the repository path, which users are still allowed to set.
cmd/cli/config/default.go
Outdated
| // clear any existing configuration before generating the default. | ||
| config.Reset() | ||
| defaultConfig, err := config.Init(cmd.Flag("path").Value.String()) | ||
| c := config.New() |
There was a problem hiding this comment.
comment no longer applies
cmd/cli/config/default.go
Outdated
| showCmd.PersistentFlags().String("path", cfg.System().GetString("repo"), "sets path dependent config fields") | ||
| return showCmd | ||
| } |
There was a problem hiding this comment.
I am not a big fan of this. Maybe we don't need to show the dynamic and actual path for those configs. That is difficult to document, not consistent and can be confusing.
There was a problem hiding this comment.
We could just delete this command in favor of clearer documentation around what the default config structure is since lack of said docs are why it existed in the first place.
cmd/cli/config/list.go
Outdated
| // create or open a repo | ||
| repoPath, err := cfg.RepoPath() | ||
| if err != nil { | ||
| return err | ||
| } | ||
| _, err = setup.SetupBacalhauRepo(repoPath, cfg) | ||
| if err != nil { | ||
| return err | ||
| } |
There was a problem hiding this comment.
similar comment as above
pkg/config/config.go
Outdated
| type config struct { | ||
| // viper instance for holding user provided configuration | ||
| user *viper.Viper | ||
| // viper instance for holding system specific configuration | ||
| system *viper.Viper | ||
| // the default configuration values to initialize with |
There was a problem hiding this comment.
I have to say this is very confusing for me to follow and understand. What I think we need to do in general is just use and pass BacalhauConfig around instead of config.Context. You can have different implementations to construct BacalhauConfig with viper being our only option for now. This allows you to test things without having to setup viper or have a file based config. Just construct BacalhauConfig instance, set some values, and pass it to your test.
What I think will simplify things is making the repo path part of BacalhauConfig instead of something independent. The Rethinking Configuration doc is suggesting that as well
There was a problem hiding this comment.
What this might look like is moving types.BacalhauConfig to config.Config, and moving the current viper implementation to config/viper/... as an implementation to setup our config
There was a problem hiding this comment.
The challenge that you might face is config setters. We need to decouple that from BacalhauConfig and we can have a separate interface for writing configs, with viper being an implementation.
pkg/config/helpers.go
Outdated
| func Get[T any](c Context, key string) (T, error) { | ||
| raw := c.User().Get(key) |
There was a problem hiding this comment.
These global getters and methods should be part of the config.Context
There was a problem hiding this comment.
They cannot be because of generics
There was a problem hiding this comment.
I see. It would be nice if we just pass BacalhauConfig and avoid having these getters in general.
| @@ -22,13 +22,13 @@ var _ bidstrategy.SemanticBidStrategy = (*ImagePlatformBidStrategy)(nil) | |||
| var ManifestCache cache.Cache[docker.ImageManifest] | |||
| var mu sync.Mutex | |||
|
|
|||
| func NewImagePlatformBidStrategy(client *docker.Client) *ImagePlatformBidStrategy { | |||
| func NewImagePlatformBidStrategy(client *docker.Client, cfg config.Context) *ImagePlatformBidStrategy { | |||
There was a problem hiding this comment.
we shouldn't be passing bacalhau config all the way down to these internal components, including executor, tracer, s3 storage provider and so on. The boundary for the config and types.BacalhauConfig should be the node package.
There was a problem hiding this comment.
I agree. Implementing that change as a part of this PR would have made this already large change even larger so I opted not to do it. I would prefer to do that in a follow on.
pkg/node/compute.go
Outdated
| computeDataPath, _ := pkgconfig.Get[string](cfg, types.NodeComputeDataPath) | ||
| regFilename := fmt.Sprintf("%s.registration.lock", nodeID) | ||
| regFilename = filepath.Join(repo, pkgconfig.ComputeStorePath, regFilename) | ||
| regFilename = filepath.Join(computeDataPath, regFilename) |
There was a problem hiding this comment.
we want to minimize number of paths the users have to define in the config, and follow more convtions over configurations. The idea is to allow users to only define the repo path, and let each component create its sub-directories and files under the repo instead of allowing users to configure those as well
There was a problem hiding this comment.
I am in agreement so long as we make it the responsibility of the repo to create and manage these paths on disk, or whatever medium is backing the repo. Just not implementing that change in this PR.
There was a problem hiding this comment.
the repo shouldn't create these files and shouldn't be aware of all the files the different components needs. We really don't even need a repo type or initialization. We just need to make sure the repo path exists and with enough permissions. Then either node package should be responsible for creating the paths it passes to constructors, or the constructors should create the paths. My preference is for the node package to setup the dependencies, including path creations.
That is out of scope of this PR, but lets not add more configurations that we don't intend to keep
frrist
force-pushed
the
frrist/config/remove-global
branch
from
8ff1d6e
to
cfbc542
Compare
cmd/cli/docker/docker_run.go
Outdated
| var ipfsCfg types.IpfsConfig | ||
| if err := cfg.ForKey(types.NodeIPFS, &ipfsCfg); err != nil { | ||
| return err | ||
| } | ||
| return printer.PrintJobExecutionLegacy(ctx, executingJob, cmd, opts.DownloadSettings, opts.RunTimeSettings, apiLegacy, apiV2, ipfsCfg) |
There was a problem hiding this comment.
this looks weird that we are reading ipfs config just for logging purpose
cmd/cli/root.go
Outdated
| util.Fatal(RootCmd, err, 1) | ||
| } | ||
| if err := viper.BindEnv("repo", "BACALHAU_DIR"); err != nil { | ||
| if err := cfg.System().BindEnv("repo", "BACALHAU_DIR"); err != nil { |
cmd/cli/serve/serve_test.go
Outdated
| // this is required for the below line to succeed as environment is being deprecated. | ||
| config.Set(configenv.Testing) | ||
| peers, err = serve.GetPeers("env") | ||
| s.T().Skip("we are deprecating libp2p, skipping") |
There was a problem hiding this comment.
comment not addressed yet
cmd/cli/wasm/wasm_run.go
Outdated
| if err := legacy_job.VerifyJob(ctx, j); err != nil { | ||
| return fmt.Errorf("verifying job for submission: %w", err) | ||
| } | ||
|
|
||
| executingJob, err := apiLegacy.Submit(ctx, j) | ||
| if err != nil { | ||
| return fmt.Errorf("executing job: %w", err) | ||
| return fmt.Errorf("submitting job for execution: %w", err) | ||
| } | ||
|
|
There was a problem hiding this comment.
what is the purpose of moving away from the existing ExecuteJob util?
cmd/util/api.go
Outdated
| var tlsCfg types.ClientTLSConfig | ||
| if err := c.ForKey(types.NodeClientAPITLS, &tlsCfg); err != nil { | ||
| return nil, err | ||
| } | ||
| // TODO(forrest): this could be moved to a validate method on the config structure. | ||
| // we do this here because when we create a new client below it will panic if a file is provided that doesn't | ||
| // exist. The correct change is of course not to panic there. | ||
| if tlsCfg.CACert != "" { | ||
| if _, err := os.Stat(tlsCfg.CACert); os.IsNotExist(err) { | ||
| return nil, fmt.Errorf("CA certificate file %q does not exists", tlsCfg.CACert) | ||
| } else if err != nil { | ||
| return nil, fmt.Errorf("CA certificate file %q cannot be read: %w", tlsCfg.CACert, err) | ||
| } | ||
| } | ||
|
|
||
| var apiHost string | ||
| if err := c.ForKey(types.NodeClientAPIHost, &apiHost); err != nil { | ||
| return nil, err | ||
| } | ||
| var apiPort uint16 | ||
| if err := c.ForKey(types.NodeClientAPIPort, &apiPort); err != nil { | ||
| return nil, err | ||
| } | ||
| apiSheme := "http" | ||
| if tlsCfg.UseTLS { | ||
| apiSheme = "https" | ||
| } | ||
| base := fmt.Sprintf("%s://%s:%d", apiSheme, apiHost, apiPort) |
There was a problem hiding this comment.
GetAPIClient and GetAPIClientV2 became too long and difficult to read compared to their existing implementation. I am sure we can do better
pkg/repo/fs.go
Outdated
| // modifies the config to include keys for accessing repo paths | ||
| func (fsr *FsRepo) EnsureRepoPathsConfigured(c config.ReadWriter) { | ||
| c.SetIfAbsent(types.AuthTokensPath, fsr.join(config.TokensPath)) | ||
| c.SetIfAbsent(types.UserKeyPath, fsr.join(config.UserPrivateKeyFileName)) | ||
| c.SetIfAbsent(types.NodeExecutorPluginPath, fsr.join(config.PluginsPath)) | ||
|
|
||
| // NB(forrest): pay attention to the subtle name difference here | ||
| c.SetIfAbsent(types.NodeComputeDataPath, fsr.join(config.ComputeStoragesPath)) | ||
| c.SetIfAbsent(types.NodeComputeStoragePath, fsr.join(config.ComputeStorePath)) | ||
|
|
||
| c.SetIfAbsent(types.UpdateCheckStatePath, fsr.join(config.UpdateCheckStatePath)) | ||
| c.SetIfAbsent(types.NodeClientAPITLSAutoCertCachePath, fsr.join(config.AutoCertCachePath)) | ||
| c.SetIfAbsent(types.UserLibp2pKeyPath, fsr.join(config.Libp2pPrivateKeyFileName)) | ||
| c.SetIfAbsent(types.NodeNetworkStoreDir, fsr.join(config.OrchestratorStorePath, config.NetworkTransportStore)) | ||
|
|
||
| c.SetIfAbsent(types.NodeRequesterJobStorePath, fsr.join(config.OrchestratorStorePath, "jobs.db")) | ||
| c.SetIfAbsent(types.NodeComputeExecutionStorePath, fsr.join(config.ComputeStorePath, "executions.db")) | ||
| } |
There was a problem hiding this comment.
I have to disagree that this should be the responsibility of the config and not the repo. As we move forward, the repo shouldn't be aware of the configs or paths it holds. Initializing a repo should be as simple as making sure the repo path exist and with enough permission.
Config should use the repo as base for the different paths it holds, such as execution and job store paths. Creating the actual files if they don't exist should be the responsibility of node package as it should be the only one actually reading the config values
There was a problem hiding this comment.
Yeah, this is a bit of 🐔 🥚 problem.
Config should use the repo as base for the different paths it holds, such as execution and job store paths
This sounds interesting - would it imply the paths in the config are relative paths the repo uses rather than complete paths?
There was a problem hiding this comment.
default paths are relative to repo path, which is also the case today. Based on the proposal here https://www.notion.so/expanso/Rethinking-Configuration-435fbe87419148b4bbc5119d413786eb, we will only have a repo path and will no longer expose configurations for each individual path.
Please make sure you review the documented thoroughly, and avoid making changes that will shift us away from getting there
pkg/repo/migrations/helpers.go
Outdated
| v.SetConfigFile(configFile) | ||
|
|
||
| configFile := filepath.Join(repoPath, config.FileName) | ||
| c := config.New() |
There was a problem hiding this comment.
this will load config with default values and env variables. The intention here is to just get the configs from the config file
pkg/repo/migrations/v1_2.go
Outdated
| @@ -46,16 +46,16 @@ var V1Migration = repo.NewMigration( | |||
| // if no incorrect values are present they are left as is. | |||
| doWrite := false | |||
| if haveSameElements(oldSwarmPeers, cfg.Node.IPFS.SwarmAddresses) { | |||
| v.Set(types.NodeIPFSSwarmAddresses, []string{}) | |||
| currentCtx.Set(types.NodeIPFSSwarmAddresses, []string{}) | |||
There was a problem hiding this comment.
you should keep using viper directly for the migrations as we want to work directly with config files and not with fully loaded configs with defaults and binded env variables. Also I don't think it is making things any simpler by working with config.Context
pkg/repo/migrations/v2_3.go
Outdated
| r.EnsureRepoPathsConfigured(newCtx) | ||
| resolvedCfg, err := newCtx.Current() |
There was a problem hiding this comment.
I think all looks good, except ensuring repo paths are configured. Will this create the repo paths if they don't exist, or just populate the config with the paths?
pkg/repo/migrations/v2_3.go
Outdated
| @@ -21,7 +21,7 @@ var V2Migration = repo.NewMigration( | |||
| repo.RepoVersion2, | |||
| repo.RepoVersion3, | |||
| func(r repo.FsRepo) error { | |||
| v, fileCfg, err := readConfig(r) | |||
| currentCtx, currentCfg, err := readConfig(r) | |||
There was a problem hiding this comment.
we only want to read the configs in the file. Otherwise you will be writing all default configs to the file instead of just the new configs that we want to persist
frrist
force-pushed
the
frrist/config/remove-global
branch
from
3610fd2
to
910bea5
Compare
- this removes usage of all global config methods opting to passing explicit config values instead.
bacalhau is no longer used by station
- return the error message to original form so test on error message passes
frrist
force-pushed
the
frrist/config/remove-global
branch
from
6438e8d
to
c15e737
Compare
| fsr, err := setup.SetupBacalhauRepo(repoPath, cfg) | ||
| if err != nil { | ||
| return fmt.Errorf("failed to reconcile repo: %w", err) | ||
| } |
There was a problem hiding this comment.
nit: what can make things more readable is for SetupBacalhauRepo to return a modified config instead of modifying the pointer. Or at least rename it to reflect what it does, such as UpdateConfigWithRepo
| } | ||
|
|
||
| type Writer interface { | ||
| Load(path string) error |
Motivation
Previously we had a global config store. This is a bad pattern and complicates many aspects of creating a node. We should instead have a config instance that we pass to methods requiring config.
Background
Bacalhau uses cobra to accept CLI flags and viper to manage configuration state. Viper and Cobra may be used together by binding viper to CLI flag definitions defined via cobra. Binding in this way allows configuration to be specified using a mixture of defaults, config files, environment variables, and flags. The priority viper treats these different options is as follows with highest precedence first:
This mean that when no config file, env vars, or flags are provided, the default settings are used. When a config file is provided, the values present in the file override default values. When env vars are provided they will override the defaults and/or the config file. When flags are passed they will override any other medium providing configuration.
Implementation
The first point of interaction with cobra and viper is in
cmd/cli/root.gowhere we access the global viper instance and bind it to all persistent flags present on the root command. This configures the viper instance with values related to the repo path, API, and logging. From there, child commands (all other commands) may bind additional flags to viper. For example,cmd/cli/serve/serve.gobinds many different flags that configure how the server operates. When a child command is executed the following steps are taken to fully configure viper and produce a final configuration state.configis created with the global viper instancecmd/cli/root.gois retrieved from viperconfiginstance.executor_storages) pathjobs.db)executions.db)types.BacalhauConfig) contains values specified by defaults, config file, env vars, and/or flags merged in the order described above.For the rest of the commands implementation services accept
types.BacalhauConfigor a subset of it to configure themselves. They no longer depend on the previously globalconfigpackage.Questions?
Various fields mentioned above contain paths to locations on disk where values may be read/stored. At present these paths can be anywhere on the filesystem - they need not live under the repo path. Since it is not possible to define defaults for these fields ahead of time, we set them based on the repo path when it is initialized.
In future iterations of this work, as described in the Rethinking Configuration document, we intend to decouple the config from the repo. This can be achieved by removing the ability to configure the paths mentioned above from the config system and instead make them all relative to the repo path with hard-coded values. In this way, the repo path can become a field of the config that is provided to the repo when it initializes.