8000 feat(e2e): `config` flag for arbitrary CometBFT configurations by cason · Pull Request #3967 · cometbft/cometbft · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

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

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 26 commits into from
Sep 4, 2024
Merged

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

merged 26 commits into from
Sep 4, 2024

Conversation

cason
Copy link
Contributor
@cason cason commented Sep 2, 2024 •
edited
Loading

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

@cason cason requested review from a team as code owners September 2, 2024 13:45
@cason cason added e2e Related to our end-to-end tests enhancement New feature or request labels Sep 2, 2024
@cason cason self-assigned this Sep 2, 2024
@cason 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
melekes approved these changes Sep 2, 2024
View reviewed changes
Copy link
Contributor
@melekes melekes left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

cason and others added 2 commits September 2, 2024 17:02
@cason cason mentioned this pull request Sep 2, 2024
Merged
3 tasks
@hvanz
Copy link
Member
hvanz commented Sep 3, 2024

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
Copy link
Contributor Author
cason commented Sep 3, 2024 •
edited
Loading

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
Copy link
Contributor Author
cason commented Sep 3, 2024

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
Copy link
Member
hvanz commented Sep 3, 2024

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
Copy link
Contributor Author
cason commented Sep 3, 2024

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.

Base automatically changed from cason/e2e-refactor to main September 4, 2024 13:17
@cason cason enabled auto-merge September 4, 2024 13:20
@cason cason added this pull request to the merge queue Sep 4, 2024
Merged via the queue into main with commit cea82b1 Sep 4, 2024
34 checks passed
@cason cason deleted the cason/3832-e2e-comet-config branch September 4, 2024 13:30
github-merge-queue bot pushed a commit that referenced this pull request Sep 4, 2024
@cason @mergify @andynog
feat(e2e): allow customizing genesis parameters on the manifest (#3969)
6c5cd32 
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>
@cason cason mentioned this pull request Sep 6, 2024
Merged
3 tasks
github-merge-queue bot pushed a commit that referenced this pull request Sep 10, 2024
@cason @mergify @andynog
fix(e2e): viper errors with invalid config and genesis manifest k…
bff1667 
…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>
@cason cason added the backport-to-v1.x Tell Mergify to backport the PR to v1.x label Oct 20, 2024
mergify bot added a commit that referenced this pull request Oct 20, 2024
@cason @melekes
@mergify @andynog
feat(e2e): config flag for arbitrary CometBFT configurations (#3967)
04efd1c 
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)
@mergify mergify bot mentioned this pull request Oct 20, 2024
Merged
3 tasks
mergify bot pushed a commit that referenced this pull request Oct 20, 2024
@cason @mergify
feat(e2e): allow customizing genesis parameters on the manifest (#3969)
939cd2d 
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
mergify bot pushed a commit that referenced this pull request Oct 20, 2024
@cason @mergify
fix(e2e): viper errors with invalid config and genesis manifest k…
669b4ed 
…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
This was referenced Oct 20, 2024
Closed
Closed
cason added a commit that referenced this pull request Oct 20, 2024
@cason @mergify @andynog
feat(e2e): allow customizing genesis parameters on the manifest (#3969)
0f1663d 
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>
mergify bot added a commit that referenced this pull request Oct 20, 2024
@mergify @cason
feat(e2e): config flag for arbitrary CometBFT configurations (backp…
968f51c 
…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>
cason added a commit that referenced this pull request Oct 20, 2024
@cason @mergify @andynog
feat(e2e): allow customizing genesis parameters on the manifest (#3969)
c24f067 
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>
@cason cason mentioned this pull request Oct 20, 2024
Merged
6 tasks
cason added a commit that referenced this pull request Oct 20, 2024
@cason @mergify @andynog
fix(e2e): viper errors with invalid config and genesis manifest k…
b92931f 
…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>
@cason cason mentioned this pull request Oct 20, 2024
Merged
3 tasks
cason added a commit that referenced this pull request Oct 20, 2024
@cason
feat(e2e): allow customizing genesis parameters on the manifest (back…
3d7bfb0 
…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
cason added a commit that referenced this pull request Oct 20, 2024
@cason @mergify @andynog
fix(e2e): viper errors with invalid config and genesis manifest k…
df25950 
…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>
@cason cason mentioned this pull request Nov 12, 2024
Merged
3 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@melekes melekes melekes approved these changes

@andynog andynog andynog approved these changes

Assignees

@cason cason

Labels
backport-to-v1.x Tell Mergify to backport the PR to v1.x e2e Related to our end-to-end tests enhancement New feature or request
Projects
No open projects
CometBFT
Status: Done
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

e2e: allow customize Comet's config parameters for all or specific nodes
4 participants
@cason @hvanz @melekes @andynog
0