feat(e2e): `config` flag for arbitrary CometBFT configurations by cason · Pull Request #3967 · cometbft/cometbft

cason · 2024-09-02T13:45:35Z

Addresses #3832.

Introduces a flag config to the manifest. It is a list of strings that should have the format key = value. Keys are any of the configuration parameters included in a CometBFT configuration file.

The flag can be global, meaning that the configurations will be applied for every node in the network, or specific for a given node, when inserted in the section referring to that node. Local/specific configurations are applied after the global ones, therefore overriding them.

Example:

config = [
   "p2p.send_rate = 51200",
   "filter_peers = true",
]

The configurations are applying using viper, as I could not find another way to load configuration parameters. They are always loaded as a string, and this appears to work correctly.

Note: this PR is based atop #3964 for simplicity, but it does not need to.

PR checklist

Tests written/updated
Changelog entry added in .changelog (we use unclog to manage our changelog)
Updated relevant documentation (docs/ or spec/) and code comments

melekes

👍

test/e2e/pkg/manifest.go

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

hvanz · 2024-09-03T10:39:19Z

Wouldn't it be better to use in the manifest the Config struct of the config package instead of strings of the form"key = value"? Then in the manifest one could write

[node.validator01]
config.P2P.send_rate = 512000

That way we don't need to care when the configuration changes, and we can reuse the existing validation functions.

cason · 2024-09-03T11:09:22Z

Wouldn't it be better to use in the manifest the Config struct of the config package instead of strings of the form"key = value"? Then in the manifest one could write
[node.validator01]
config.P2P.send_rate = 512000
That way we don't need to care when the configuration changes, and we can reuse the existing validation functions.

That was my original idea, but it does not work.

The Config object use the Viper setup, with mapstructure tags. Which are not recognized by the toml package we use to decode the manifest.

cason · 2024-09-03T11:10:32Z

That way we don't need to care when the configuration changes, and we can reuse the existing validation functions.

We still check the config parameters, as we call the validate method.

hvanz · 2024-09-03T11:19:24Z

The Config object use the Viper setup, with mapstructure tags. Which are not recognized by the toml package we use to decode the manifest.

yeah, I thought about that. We would need to use the mapstructure tags in the manifest too instead of the toml tags. But I guess the implementation would be quite different from this PR.

cason · 2024-09-03T11:22:28Z

yeah, I thought about that. We would need to use the mapstructure tags in the manifest too instead of the toml tags.

Initially I was thinking on the opposite direction, namely, getting riding of this dependency on Viper. But after playing a little with it, we probably should use viper on e2e as well.

Notice that we don't need necessarily to use the mapstructure tag, see the companion PR for the genesis file. We can change the tag Viper uses, but we need to be consistent.

Closes #3961. Similarly to #3967, introduces a new `genesis` field for manifest files. The format is `key = value` and multiple entries can be provided. For example, ``` genesis = [ "consensus_params.evidence.max_bytes = 1000", "chain_id = pr3969" ] ``` After the genesis document is produced by the `setup` implementation of the runner, the provided genesis customized configurations are applied to the generated genesis file. The solution also uses `viper`, as it allows setting only some specific keys, leaving the pre-produced content untouched. Note: this PR is based atop #3964 for simplicity, but it does not need to. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Andy Nogueira <me@andynogueira.dev>

…eys (#4016) Addresses comments #3969 (review) and #3967 (review) on previous PRs. The solution is in fact simple, as it is possible to tell viper to return an error if one of the configured entries (key/value pairs) were not applied to the destination structure, because they don't exist or are invalid. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Andy Nogueira <me@andynogueira.dev>

Addresses #3832. Introduces a flag `config` to the manifest. It is a list of strings that should have the format `key = value`. Keys are any of the configuration parameters included in a CometBFT configuration file. The flag can be global, meaning that the configurations will be applied for every node in the network, or specific for a given node, when inserted in the section referring to that node. Local/specific configurations are applied after the global ones, therefore overriding them. Example: ``` config = [ "p2p.send_rate = 51200", "filter_peers = true", ] ``` The configurations are applying using `viper`, as I could not find another way to load configuration parameters. They are always loaded as a string, and this appears to work correctly. Note: this PR is based atop #3964 for simplicity, but it does not need to. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com> Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Andy Nogueira <me@andynogueira.dev> (cherry picked from commit cea82b1)

Closes #3961. Similarly to #3967, introduces a new `genesis` field for manifest files. The format is `key = value` and multiple entries can be provided. For example, ``` genesis = [ "consensus_params.evidence.max_bytes = 1000", "chain_id = pr3969" ] ``` After the genesis document is produced by the `setup` implementation of the runner, the provided genesis customized configurations are applied to the generated genesis file. The solution also uses `viper`, as it allows setting only some specific keys, leaving the pre-produced content untouched. Note: this PR is based atop #3964 for simplicity, but it does not need to. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Andy Nogueira <me@andynogueira.dev> (cherry picked from commit 6c5cd32) # Conflicts: # test/e2e/pkg/manifest.go # test/e2e/pkg/testnet.go # test/e2e/runner/setup.go

…eys (#4016) Addresses comments #3969 (review) and #3967 (review) on previous PRs. The solution is in fact simple, as it is possible to tell viper to return an error if one of the configured entries (key/value pairs) were not applied to the destination structure, because they don't exist or are invalid. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Andy Nogueira <me@andynogueira.dev> (cherry picked from commit bff1667) # Conflicts: # test/e2e/pkg/testnet.go # test/e2e/runner/setup.go

Closes #3961. Similarly to #3967, introduces a new `genesis` field for manifest files. The format is `key = value` and multiple entries can be provided. For example, ``` genesis = [ "consensus_params.evidence.max_bytes = 1000", "chain_id = pr3969" ] ``` After the genesis document is produced by the `setup` implementation of the runner, the provided genesis customized configurations are applied to the generated genesis file. The solution also uses `viper`, as it allows setting only some specific keys, leaving the pre-produced content untouched. Note: this PR is based atop #3964 for simplicity, but it does not need to. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Andy Nogueira <me@andynogueira.dev>

…ort #3967) (#4309) Addresses #3832. Introduces a flag `config` to the manifest. It is a list of strings that should have the format `key = value`. Keys are any of the configuration parameters included in a CometBFT configuration file. The flag can be global, meaning that the configurations will be applied for every node in the network, or specific for a given node, when inserted in the section referring to that node. Local/specific configurations are applied after the global ones, therefore overriding them. Example: ``` config = [ "p2p.send_rate = 51200", "filter_peers = true", ] ``` The configurations are applying using `viper`, as I could not find another way to load configuration parameters. They are always loaded as a string, and this appears to work correctly. Note: this PR is based atop #3964 for simplicity, but it does not need to. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments <hr>This is an automatic backport of pull request #3967 done by [Mergify](https://mergify.com). --------- Co-authored-by: Daniel <daniel.cason@informal.systems>

Closes #3961. Similarly to #3967, introduces a new `genesis` field for manifest files. The format is `key = value` and multiple entries can be provided. For example, ``` genesis = [ "consensus_params.evidence.max_bytes = 1000", "chain_id = pr3969" ] ``` After the genesis document is produced by the `setup` implementation of the runner, the provided genesis customized configurations are applied to the generated genesis file. The solution also uses `viper`, as it allows setting only some specific keys, leaving the pre-produced content untouched. Note: this PR is based atop #3964 for simplicity, but it does not need to. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Andy Nogueira <me@andynogueira.dev>

…eys (#4016) Addresses comments #3969 (review) and #3967 (review) on previous PRs. The solution is in fact simple, as it is possible to tell viper to return an error if one of the configured entries (key/value pairs) were not applied to the destination structure, because they don't exist or are invalid. --- #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Andy Nogueira <me@andynogueira.dev>

…port #3969) (#4312) Closes #3961. Similarly to #3967, introduces a new `genesis` field for manifest files. The format is `key = value` and multiple entries can be provided. For example, ``` genesis = [ "consensus_params.evidence.max_bytes = 1000", "chain_id = pr3969" ] ``` After the genesis document is produced by the `setup` implementation of the runner, the provided genesis customized configurations are applied to the generated genesis file. The solution also uses `viper`, as it allows setting only some specific keys, leaving the pre-produced content untouched. Note: this PR is based atop #3964 for simplicity, but it does not need to. --- This is an manual backport of pull request #3969. #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments

…eys (backport #4016) (#4313) Addresses comments #3969 (review) and #3967 (review) on previous PRs. The solution is in fact simple, as it is possible to tell viper to return an error if one of the configured entries (key/value pairs) were not applied to the destination structure, because they don't exist or are invalid. --- This is an manual backport of pull request #4016. #### PR checklist - [ ] Tests written/updated - [ ] Changelog entry added in `.changelog` (we use [unclog](https://github.com/informalsystems/unclog) to manage our changelog) - [ ] Updated relevant documentation (`docs/` or `spec/`) and code comments --------- Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com> Co-authored-by: Andy Nogueira <me@andynogueira.dev>

Daniel Cason added 8 commits September 2, 2024 13:09

e2e: rename Manifest fields colliding with Testnet

4991c80

e2e: Tesnet includes and extends Manifest

92f9ec1

e2e: Node includes and extends ManifestNode

0a596f2

e2e: fix Database Node setting

778fbc6

e2e: manifest config flag, global and per node

de9cc68

e2e: Testnet.Config included in Node.Config

af7cb2a

e2e: runner setup apply Config entries

67ce59d

e2e: validate 'config' entries, must be "key = value"

912d029

cason requested a review from a team as a code owner September 2, 2024 13:45

cason requested a review from a team September 2, 2024 13:45

cason added e2e Related to our end-to-end tests enhancement New feature or request labels Sep 2, 2024

cason self-assigned this Sep 2, 2024

e2e: pleasing the linter

d1ae2d3

cason linked an issue Sep 2, 2024 that may be closed by this pull request

e2e: allow customize Comet's config parameters for all or specific nodes #3832

Closed

melekes approved these changes Sep 2, 2024

View reviewed changes

test/e2e/pkg/manifest.go Outdated Show resolved Hide resolved

test/e2e/pkg/manifest.go Outdated Show resolved Hide resolved

Daniel and others added 2 commits September 2, 2024 17:02

Apply suggestions from code review

7cbc116

Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>

Merge branch 'main' into cason/e2e-refactor

2072bad

cason mentioned this pull request Sep 2, 2024

feat(e2e): allow customizing genesis parameters on the manifest #3969

Merged

3 tasks

mergify bot and others added 6 commits September 2, 2024 15:57

Merge branch 'cason/e2e-refactor' into cason/3832-e2e-comet-config

cec8bd1

e2e: only runs viper if there are keys to set

e0bf483

Merge branch 'main' into cason/e2e-refactor

6e8de84

Merge branch 'cason/e2e-refactor' into cason/3832-e2e-comet-config

bcce981

Merge branch 'main' into cason/e2e-refactor

71b18ae

Merge branch 'cason/e2e-refactor' into cason/3832-e2e-comet-config

42ce5f1

mergify bot added 4 commits September 4, 2024 04:38

Merge branch 'main' into cason/e2e-refactor

eb40ed4

Merge branch 'cason/e2e-refactor' into cason/3832-e2e-comet-config

54169a0

Merge branch 'main' into cason/e2e-refactor

49dbc0d

Merge branch 'cason/e2e-refactor' into cason/3832-e2e-comet-config

6b4b2de

Base automatically changed from cason/e2e-refactor to main September 4, 2024 13:17

Merge branch 'main' into cason/3832-e2e-comet-config

ab239c7

cason enabled auto-merge September 4, 2024 13:20

cason added this pull request to the merge queue Sep 4, 2024

Merged via the queue into main with commit cea82b1 Sep 4, 2024

cason deleted the cason/3832-e2e-comet-config branch September 4, 2024 13:30

cason mentioned this pull request Sep 6, 2024

fix(e2e): viper errors with invalid config and genesis manifest keys #4016

Merged

3 tasks

cason added the backport-to-v1.x label Oct 20, 2024

mergify bot mentioned this pull request Oct 20, 2024

feat(e2e): config flag for arbitrary CometBFT configurations (backport #3967) #4309

Merged

3 tasks

This was referenced Oct 20, 2024

feat(e2e): allow customizing genesis parameters on the manifest (backport #3969) #4310

Closed

fix(e2e): viper errors with invalid config and genesis manifest keys (backport #4016) #4311

Closed

cason mentioned this pull request Oct 20, 2024

feat(e2e): allow customizing genesis parameters on the manifest (backport #3969) #4312

Merged

6 tasks

cason mentioned this pull request Oct 20, 2024

fix(e2e): viper errors with invalid config and genesis manifest keys (backport #4016) #4313

Merged

3 tasks

cason mentioned this pull request Nov 12, 2024

feat(log): added log_colors configuration flag #4475

Merged

3 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(e2e): `config` flag for arbitrary CometBFT configurations#3967

feat(e2e): `config` flag for arbitrary CometBFT configurations#3967
cason merged 26 commits intomainfrom
cason/3832-e2e-comet-config

cason commented Sep 2, 2024 •

edited

Loading

Uh oh!

melekes left a comment

Uh oh!

Uh oh!

Uh oh!

hvanz commented Sep 3, 2024

Uh oh!

cason commented Sep 3, 2024 •

edited

Loading

Uh oh!

cason commented Sep 3, 2024

Uh oh!

hvanz commented Sep 3, 2024

Uh oh!

cason commented Sep 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Conversation

cason commented Sep 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR checklist

Uh oh!

melekes left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hvanz commented Sep 3, 2024

Uh oh!

cason commented Sep 3, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

cason commented Sep 3, 2024

Uh oh!

hvanz commented Sep 3, 2024

Uh oh!

cason commented Sep 3, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

cason commented Sep 2, 2024 •

edited

Loading

cason commented Sep 3, 2024 •

edited

Loading