Implement `-c/--config` flag #4242

frrist · 2024-07-17T22:08:35Z

Changes to Configuration Handling

The config package has been refactored to allow more flexible and powerful configuration options. The configuration constructor now accepts the following inputs:

Map of Config Keys to Values: Explicitly set configuration values.
Map of Config Keys to Flags: Command-line flags mapped to configuration keys.
Map of Config Keys to Environment Variable Names: Environment variables mapped to configuration keys.
Slice of Config File Paths: Paths to configuration files, applied in the order provided.
Default Configuration Structure: A fallback configuration structure.

Precedence of Configuration Sources

The precedence of these options, from highest to lowest, is as follows:

Map of config keys to values
Map of config keys to flags
Map of config keys to environment variable names
Slice of config file paths
Default configuration structure

This means that if a configuration key is defined in multiple sources, the value from the source higher on this list will be used.

Example Usage

Here’s an example of how to use the new configuration constructor:

cfg, err := config.New(
	config.WithValues(configValues),
	config.WithFlags(configFlags),
	config.WithEnvironmentVariables(configEnvVar),
	config.WithPaths(configFiles...),
	config.WithDefault(config.ForEnvironment()),
)

A more detailed example specific to Bacalhau can be found here.

Handling of Configuration Files in Bacalhau

When using Bacalhau, configuration files are treated differently depending on their location:

--config flag: When multiple configuration files are provided via the --config flag, Bacalhau merges the values from these files. The order in which the files are listed determines the precedence, with later files overriding earlier ones.
Files in ~/.bacalhau and ~/.config/bacalhau: These files are not merged, and only used a file(s) isn't specified by the --config flag. . Instead, Bacalhau prioritizes ~/.config/bacalhau. If a config file is found in ~/.config/bacalhau, it is used exclusively. If not, Bacalhau falls back to ~/.bacalhau. If neither file is present, Bacalhau does not load a config file.

Precedence with Flags and Configuration Files

Flags vs. Config Files: Command-line flags always take precedence over values specified in configuration files. For example, in the command:

bacalhau -c config.yaml -c override.yaml --api-host=1.1.1.1 serve

The client host will be set to 1.1.1.1, even if this key is present in config.yaml or override.yaml.

Multiple Flags: If multiple flags and config files are provided, the --config flag still takes precedence. For example:

bacalhau -c config.yaml -c override.yaml --api-host=1.1.1.1 -c node.clientapi.host=2.2.2.2 serve

Here, the client host will be set to 2.2.2.2 as specified by the -c node.clientapi.host=2.2.2.2 flag.

Changes to Agent API

In order to implement proper testing of the bacalhau configuration package the agent API was extended with a new endpoint /api/v1/agent/config which returns the current configuration used by an agent as json. This API allows bacalhau to be started with a specific configuration and then queried to make assertions on the configuration being used. test/config.sh implements coverage of this API and the config package.

Changes to `config list`

Previously this command returns a list of config keys and their default values. It now returns a list of config keys and their descriptions.

coderabbitai · 2024-07-17T22:08:41Z

Important

Review skipped

Auto reviews are disabled on this repository.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Tips

Chat

There are 3 ways to chat with CodeRabbit:

Review comments: Directly reply to a review comment made by CodeRabbit. Example:
- I pushed a fix in commit <commit_id>.
- Generate unit testing code for this file.
- Open a follow-up GitHub issue for this discussion.
Files and specific lines of code (under the "Files changed" tab): Tag @coderabbitai in a new review comment at the desired location with your query. Examples:
- @coderabbitai generate unit testing code for this file.
- @coderabbitai modularize this function.
PR comments: Tag @coderabbitai in a new PR comment to ask questions about the PR branch. For the best results, please provide a very specific query, as very limited context is provided in this mode. Examples:
- @coderabbitai generate interesting stats about this repository and render them as a table.
- @coderabbitai show all the console.log statements in this repository.
- @coderabbitai read src/utils.ts and generate unit testing code.
- @coderabbitai read the files in the src/scheduler package and generate a class diagram using mermaid and a README in the markdown format.
- @coderabbitai help me debug CodeRabbit configuration file.

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)

@coderabbitai pause to pause the reviews on a PR.
@coderabbitai resume to resume the paused reviews.
@coderabbitai review to trigger an incremental review. This is useful when automatic reviews are disabled for the repository.
@coderabbitai full review to do a full review from scratch and review all the files again.
@coderabbitai summary to regenerate the summary of the PR.
@coderabbitai resolve resolve all the CodeRabbit review comments.
@coderabbitai configuration to show the current CodeRabbit configuration for the repository.
@coderabbitai help to get help.

Additionally, you can add @coderabbitai ignore anywhere in the PR description to prevent this PR from being reviewed.

CodeRabbit Configuration File (`.coderabbit.yaml`)

You can programmatically configure CodeRabbit by adding a .coderabbit.yaml file to the root of your repository.
Please see the configuration documentation for more information.
If your editor has YAML language server enabled, you can add the path at the top of this file to enable auto-completion and validation: # yaml-language-server: $schema=https://coderabbit.ai/integrations/schema.v2.json

Documentation and Community

Visit our Documentation for detailed information on how to use CodeRabbit.
Join our Discord Community to get help, request features, and share feedback.
Follow us on X/Twitter for updates and announcements.

cmd/util/flags/cliflags/Config.md

cmd/util/flags/cliflags/config.go

cmd/util/flags/cliflags/job.go

cmd/util/flags/cliflags/repo.go

pkg/config/config.go

- closes #4217

- Ensures values set via -c/--config flag override file config in bacalhu repo

includes code generation to produce descriptions of each config type based on go code comments in config struct.

- closes #4243

wdbaruni · 2024-08-11T07:55:46Z

The documented behaviour looks as intended. Though I pulled the branch locally, and here are some issues I've found:

1. No config file found:

no config file provided, and no config file under ~/.bacalhau/config.yaml. It should start with default configurations, but it fails, and the failure is not clear to the user on how to fix things. Adding an empty ~/.bacalhau/config.yaml solved the issue

→ bacalhau serve --node-type=requester,compute
failed to setup config: failed to read config in "": Config File "config" Not Found in "[]"

2. Still auto-creating `~/.bacalhau/config.yaml`

repo is initialized, run bacalhau with config provided using bacalhau serve --node-type=requester,compute -c ~/config.yaml, we still auto-create config.yaml under the repo and we are still adding the NodeID to the config, which should be part of system_metadata.yaml

3. Config file values are not respected

It seems we are not reading the configurations from the config files, whether the user provided one using -c flag or the auto-found one under ~/.bacalhau/config.yaml. This what I tried:

# Set local publisher to 6611 in the default config location
→ bacalhau config set node.compute.localpublisher.port 6611

# verified the configuration was written
→ cat .bacalhau/config.yaml
node:
    compute:
        executionstore:
            Type: BoltDB
            Path: /Users/walid/.bacalhau/compute_store/executions.db
        localpublisher:
            port: 6611
    name: n-86d437a6-5214-420c-a941-ca950689d254
    requester:
        jobstore:
            Type: BoltDB

# Ran bacalhau
→ export LOG_LEVEL=DEBUG
→ bacalhau serve --node-type=requester,compute

# Compute config still shows port 6001
09:51:44.497 | DBG workspace/bacalhau/pkg/node/config_compute.go:226 > Compute config: {...LocalPublisher:{Address:public Port:6001

# The same behaviour with running with user provided config. Though `-c Node.Compute.LocalPublisher.Port=6633` is working fine

4.Long error messages

This is a general problem with Bacalhau, but will be great if we can slowly improve things with new changes. Our error messages are a big too long and not straight to the point

# no config file found
→ bacalhau serve --node-type=requester,compute -c walid.yaml
failed to setup config: failed to read config in "walid.yaml": open walid.yaml: no such file or directory

# no matching config key. This one is not that bad
→ bacalhau serve --node-type=requester,compute -c walid.baruni
invalid argument "walid.baruni" for "-c, --config" flag: no config key matching "walid.baruni"

wdbaruni · 2024-08-11T07:58:51Z

My recommendation is to improve the testing coverage of these changes, and to document the testing plan and manual testing you've done to verify the changes are working as expected. This is a very tricky area you are working on, and easy for issues to slip through :)

wdbaruni · 2024-08-11T08:16:50Z

cmd/util/flags/cliflags/repo.go

+	if userDir, err := os.UserConfigDir(); err == nil {
+		return filepath.Join(userDir, defaultBacalhauDir)
+	}


We discussed having the UserConfigDir as a default for the config path, and not for the repo path. Both locations should be decoupled from other. The only reason we are still considering <repo>/config.yaml as the default location for configs is for backward compatibility only. After implementing migration logic where we migrate configs from repo to UserConfigDir, then we should no longer look for configs under the repo

Our tenant in this area should be less-is-more. We should avoid doing more magic on-behalf of the user and avoid having too many paths and forks in the logic of config setup. My original preference was to even not have a default config path, but I understand that will complicate migration logic

wdbaruni · 2024-08-11T08:21:54Z

cmd/util/repo.go

+				return nil, err
+			}
+		}
+		configFiles = append(configFiles, repoConfig, xdgConfig)


you should look for xddConfig and only if not preset (indicating no migration yet), then look at repoConfig

frrist · 2024-08-13T18:37:03Z

no config file provided, and no config file under ~/.bacalhau/config.yaml. It should start with default configurations, but it fails, and the failure is not clear to the user on how to fix things. Adding an empty ~/.bacalhau/config.yaml solved the issue.

Great catch, this has been addressed and tests for it have been added in test/config.sh

repo is initialized, run bacalhau with config provided using bacalhau serve --node-type=requester,compute -c ~/config.yaml, we still auto-create config.yaml under the repo and we are still adding the NodeID to the config, which should be part of system_metadata.yaml

This is still the intended behavior until the repo is decoupled from the config file. At present the generated config file contains paths to various stores on disk. We will migrate away from this as part of #4219

It seems we are not reading the configurations from the config files, whether the user provided one using -c flag or the auto-found one under ~/.bacalhau/config.yaml. This what I tried:

Another good catch, this is fixed and tests implemented to verify.

This is a general problem with Bacalhau, but will be great if we can slowly improve things with new changes. Our error messages are a big too long and not straight to the point

I have made these clearer. e.g.

no file exists.

bacalhau serve -c walid.yaml
invalid argument "walid.yaml" for "-c, --config" flag: the specified configuration file "walid.yaml" doesn't exist

invalid config filed

bacalhau serve --node-type=requester,compute -c walid.baruni
invalid argument "walid.baruni" for "-c, --config" flag: no config key matching "walid.baruni" run 'bacalhau config list' for a list of valid keys

invalid config field that happens to also be a file

touch walid.baruni
bacalhau serve -c walid.baruni
Error: invalid argument "walid.baruni" for "-c, --config" flag: config files must end in suffix '.yaml' or '.yml'

file is present, but empty so warn

touch walid.yaml
bacalhau serve -c walid.yaml 
11:44:19.87 | WRN cmd/util/flags/cliflags/config.go:85 > the specified configuration file is empty and ineffectual
...(process continues)

file is a directoy

mkdir walid.yaml
bacalhau serve -c walid.yaml
Error: invalid argument "walid.yaml" for "-c, --config" flag: the specified configuration file "walid.yaml" is a directory, must be a file

wdbaruni

Looks good

wdbaruni · 2024-08-14T09:17:20Z

pkg/config/config.go

+	/*
+		if len(c.values) > 0 {
+			if err := c.base.MergeConfigMap(c.values); err != nil {
+				return nil, fmt.Errorf("merging config values: %w", err)
+			}
+		}
+	*/


leftover commented code

frrist force-pushed the frrist/cli/config-flag branch from d55ac29 to a2bb09b Compare July 18, 2024 00:03

frrist mentioned this pull request Jul 18, 2024

Server Suite timeout_test.go is an invalid test (was never implemented correctly to begin with) #4243

Closed

frrist linked an issue Jul 18, 2024 that may be closed by this pull request

Server Suite timeout_test.go is an invalid test (was never implemented correctly to begin with) #4243

Closed

frrist force-pushed the frrist/cli/config-flag branch from 456de34 to 0529b2b Compare July 18, 2024 16:47

frrist requested review from wdbaruni and udsamani July 18, 2024 16:47

wdbaruni requested changes Aug 1, 2024

View reviewed changes

frrist added 9 commits August 7, 2024 15:43

feat: implement config flag

4a85ee1

- closes #4217

refactor: Update config Load function to respect flag precedence

1a0bf0f

- Ensures values set via -c/--config flag override file config in bacalhu repo

test: implement test for config flag and readme

e8f8af6

feat: implement auto-complete for --config flag

e93db5e

includes code generation to produce descriptions of each config type based on go code comments in config struct.

fix: correctly set the global viper instance on config load

a471456

testing: delete pkg/cmd/timeout_test.go

979dc58

- closes #4243

refactor: validate config key names from flag

3794d4f

refactor: improve configuration logic and add testing

f4c43a5

fix: cli config test after requiring validation

7a03503

frrist force-pushed the frrist/cli/config-flag branch from a741d3f to 7a03503 Compare August 7, 2024 22:45

refactor: config list prints keys and descriptions

9c85a1d

frrist requested a review from wdbaruni August 8, 2024 00:15

refactor: config now accepts a map of values and slice of paths

65718b2

frrist self-assigned this Aug 8, 2024

Merge branch 'main' into frrist/cli/config-flag

f2bd75f

wdbaruni reviewed Aug 11, 2024

View reviewed changes

frrist added 4 commits August 13, 2024 09:40

Merge branch 'main' into frrist/cli/config-flag

aa2fe5e

refactor: late loading of config flags and environment vars

d9c19b6

feat: implement agent config endpoint and testing

6a5085b

test: add test case for config flag ordering

06fcbd6

refactor: better error message for config flags

a26ae37

wdbaruni approved these changes Aug 14, 2024

View reviewed changes

refactor: remove extranious code comments

52f207a

frrist merged commit 50831fa into main Aug 14, 2024
4 checks passed

frrist deleted the frrist/cli/config-flag branch August 14, 2024 17:57

frrist mentioned this pull request Sep 26, 2024

issue with -c api.port #4539

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implement `-c/--config` flag #4242

Implement `-c/--config` flag #4242

frrist commented Jul 17, 2024 •

edited

Loading

coderabbitai bot commented Jul 17, 2024

Review skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (`.coderabbit.yaml`)

Documentation and Community

wdbaruni commented Aug 11, 2024 •

edited

Loading

wdbaruni commented Aug 11, 2024

wdbaruni Aug 11, 2024

wdbaruni Aug 11, 2024

frrist commented Aug 13, 2024 •

edited

Loading

wdbaruni left a comment

wdbaruni Aug 14, 2024

Implement -c/--config flag #4242

Implement -c/--config flag #4242

Conversation

frrist commented Jul 17, 2024 • edited Loading

Changes to Configuration Handling

Precedence of Configuration Sources

Example Usage

Handling of Configuration Files in Bacalhau

Precedence with Flags and Configuration Files

Changes to Agent API

Changes to config list

coderabbitai bot commented Jul 17, 2024

Review skipped

Chat

CodeRabbit Commands (invoked as PR comments)

CodeRabbit Configuration File (.coderabbit.yaml)

Documentation and Community

wdbaruni commented Aug 11, 2024 • edited Loading

1. No config file found:

2. Still auto-creating ~/.bacalhau/config.yaml

3. Config file values are not respected

4.Long error messages

wdbaruni commented Aug 11, 2024

wdbaruni Aug 11, 2024

Choose a reason for hiding this comment

wdbaruni Aug 11, 2024

Choose a reason for hiding this comment

frrist commented Aug 13, 2024 • edited Loading

wdbaruni left a comment

Choose a reason for hiding this comment

wdbaruni Aug 14, 2024

Choose a reason for hiding this comment

Implement `-c/--config` flag #4242

Implement `-c/--config` flag #4242

frrist commented Jul 17, 2024 •

edited

Loading

Changes to `config list`

CodeRabbit Configuration File (`.coderabbit.yaml`)

wdbaruni commented Aug 11, 2024 •

edited

Loading

2. Still auto-creating `~/.bacalhau/config.yaml`

frrist commented Aug 13, 2024 •

edited

Loading