Skip to content
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.

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

feat: add gnoland config command suite #1605

Merged
merged 32 commits into from
Mar 12, 2024

Conversation

zivkovicmilos
Copy link
Member

@zivkovicmilos zivkovicmilos commented Jan 30, 2024

Description

Based on discussions in #1593, this PR introduces the CLI suite for manipulating the config.toml. Using this suite, we can build better workflows for power users.

This PR is a series of lego blocks that are required for us to get a normal user chain connection going:

All of these PRs get us to a point where the user can run:

  • gnoland init
  • gnoland start

to get up and running with any Gno network. The added middle step is fine-tuning the configuration and genesis, but it's worth noting this step is optional.

New commands:

gnoland config --help             
USAGE
  config <subcommand> [flags]

Gno config manipulation suite, for editing base and module configurations

SUBCOMMANDS
  init  Initializes the Gno node configuration
  set   Edits the Gno node configuration
  get   Shows the Gno node configuration

In short, the gnoland config init command initializes a default config.toml, while other subcommands allow editing
viewing any field in the specific configuration.

Contributors' checklist...
  • Added new tests, or not needed, or not feasible
  • Provided an example (e.g. screenshot) to aid review or the PR is self-explanatory
  • Updated the official documentation or not needed
  • No breaking changes were made, or a BREAKING CHANGE: xxx message was included in the description
  • Added references to related issues and PRs
  • Provided any useful hints for running manual tests
  • Added new benchmarks to generated graphs, if any. More info here.

@zivkovicmilos zivkovicmilos added the 📦 ⛰️ gno.land Issues or PRs gno.land package related label Jan 30, 2024
@zivkovicmilos zivkovicmilos self-assigned this Jan 30, 2024
@github-actions github-actions bot added the 📦 🌐 tendermint v2 Issues or PRs tm2 related label Jan 30, 2024
Copy link

codecov bot commented Jan 30, 2024

Codecov Report

Attention: Patch coverage is 77.29730% with 42 lines in your changes are missing coverage. Please review.

Project coverage is 47.50%. Comparing base (45c8f90) to head (62d3951).

Files Patch % Lines
gno.land/cmd/gnoland/config.go 63.26% 17 Missing and 1 partial ⚠️
gno.land/cmd/gnoland/config_set.go 79.22% 9 Missing and 7 partials ⚠️
gno.land/cmd/gnoland/config_get.go 81.25% 3 Missing and 3 partials ⚠️
gno.land/cmd/gnoland/config_init.go 91.30% 1 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #1605      +/-   ##
==========================================
+ Coverage   47.41%   47.50%   +0.08%     
==========================================
  Files         384      388       +4     
  Lines       61187    61367     +180     
==========================================
+ Hits        29014    29152     +138     
- Misses      29744    29774      +30     
- Partials     2429     2441      +12     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@thehowl thehowl mentioned this pull request Jan 31, 2024
7 tasks
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

Some comments from the review meeting:

  • Preferably let's avoid having all of the flags, and instead having a single source of truth. Approaches can be reflect, codegen, or keeping two registries with a test that checks that the config in tm2 and here doesn't fall out of sync.
  • And a preferable API seems to be gnoland config <key> [<value>] (or some variation).

@zivkovicmilos
Copy link
Member Author

Some comments from the review meeting:

  • Preferably let's avoid having all of the flags, and instead having a single source of truth. Approaches can be reflect, codegen, or keeping two registries with a test that checks that the config in tm2 and here doesn't fall out of sync.
  • And a preferable API seems to be gnoland config <key> [<value>] (or some variation).

Based on these discussions, if we wanted to adapt gnoland config to use the git config syntax (similar), we'd need to use reflect.

I've drawn up a small snippet that demonstrates how it would look like (note it always uses the fresh struct, so we can't risk falling out of sync in the update command):
https://goplay.tools/snippet/LuahKUZcoig

Let me know your thoughts, if this looks alright, so I can apply it to this PR 🙏

cc @gfanton @ajnavarro @moul

@zivkovicmilos
Copy link
Member Author

zivkovicmilos commented Feb 16, 2024

@thehowl @gfanton @ajnavarro

I've swapped the approach in this PR to utilize reflect, as we've discussed:
714db31 (and with an updated comment here)

Now, you can edit individual fields (however nested) using a similar syntax to git config.
Ex:
gnoland config edit RPC.ListenAddress tcp://0.0.0.0:8080

I've also noticed that my previous version of the PR lacked support for the Mempool configuration, which nobody seemed to point out 🙂
This has been resolved in the commit I've linked, and covered with unit tests

tm2/pkg/bft/config/config.go Show resolved Hide resolved
tm2/pkg/bft/config/config.go Show resolved Hide resolved
gno.land/cmd/gnoland/config.go Outdated Show resolved Hide resolved
gno.land/cmd/gnoland/config_init.go Outdated Show resolved Hide resolved
gno.land/cmd/gnoland/config_init.go Outdated Show resolved Hide resolved
gno.land/cmd/gnoland/config_edit.go Outdated Show resolved Hide resolved
gno.land/cmd/gnoland/config_edit.go Outdated Show resolved Hide resolved
gno.land/cmd/gnoland/config_edit.go Outdated Show resolved Hide resolved
gno.land/cmd/gnoland/config_edit.go Outdated Show resolved Hide resolved
gno.land/cmd/gnoland/config_edit.go Outdated Show resolved Hide resolved
@zivkovicmilos
Copy link
Member Author

@gfanton @ajnavarro
Friendly ping to check the latest changes 🙏

8hd6lm

gno.land/cmd/gnoland/config_set.go Outdated Show resolved Hide resolved
gno.land/cmd/gnoland/config_set.go Outdated Show resolved Hide resolved
Copy link
Member

@thehowl thehowl left a comment

Choose a reason for hiding this comment

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

I'll make a separate comment for the unmarshaler; mostly nitpicky comments as usual :)

gno.land/cmd/gnoland/config.go Outdated Show resolved Hide resolved
gno.land/cmd/gnoland/config_init.go Show resolved Hide resolved
gno.land/cmd/gnoland/config_get.go Outdated Show resolved Hide resolved
gno.land/cmd/gnoland/config_get.go Show resolved Hide resolved
gno.land/cmd/gnoland/config_set_test.go Show resolved Hide resolved
gno.land/cmd/gnoland/config_get_test.go Outdated Show resolved Hide resolved
Copy link
Member

@gfanton gfanton left a comment

Choose a reason for hiding this comment

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

Just a small change needed. I experimented with the command a bit. looks good ! :)

gno.land/cmd/gnoland/config_set.go Outdated Show resolved Hide resolved
@thehowl thehowl merged commit bc77c2b into gnolang:master Mar 12, 2024
184 of 185 checks passed
@leohhhn leohhhn deleted the feat/config branch March 12, 2024 12:00
albttx pushed a commit to albttx/gno that referenced this pull request Mar 15, 2024
## Description

Based on discussions in gnolang#1593, this PR introduces the CLI suite for
manipulating the `config.toml`. Using this suite, we can build better
workflows for power users.

This PR is a series of lego blocks that are required for us to get a
normal user chain connection going:
- gnolang#1252 - solved `genesis.json` management and manipulation
- gnolang#1593 - solved node secrets management and manipulation
- this PR - solves `config.toml` management and manipulation

All of these PRs get us to a point where the user can run:
- `gnoland init`
- `gnoland start`

to get up and running with any Gno network. The added middle step is
fine-tuning the configuration and genesis, but it's worth noting this
step is optional.

New commands:
```shell
gnoland config --help             
USAGE
  config <subcommand> [flags]

Gno config manipulation suite, for editing base and module configurations

SUBCOMMANDS
  init  Initializes the Gno node configuration
  set   Edits the Gno node configuration
  get   Shows the Gno node configuration
```

In short, the `gnoland config init` command initializes a default
`config.toml`, while other subcommands allow editing
 viewing any field in the specific configuration.

<details><summary>Contributors' checklist...</summary>

- [x] Added new tests, or not needed, or not feasible
- [x] Provided an example (e.g. screenshot) to aid review or the PR is
self-explanatory
- [x] Updated the official documentation or not needed
- [x] No breaking changes were made, or a `BREAKING CHANGE: xxx` message
was included in the description
- [x] Added references to related issues and PRs
- [ ] Provided any useful hints for running manual tests
- [ ] Added new benchmarks to [generated
graphs](https://gnoland.github.io/benchmarks), if any. More info
[here](https://github.com/gnolang/gno/blob/master/.benchmarks/README.md).
</details>
thehowl added a commit that referenced this pull request Mar 26, 2024
…1769)

After briefly discussing #1605, me and @zivkovicmilos convened that it
would be better if the `config` commands took, for the keys in the
configuration, their TOML name rather than the internal Go struct field
name. This PR implements that.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related
Projects
Status: Done
Status: No status
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants