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(cmd/gnoland): cli implement --home #2355

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

albttx
Copy link
Member

@albttx albttx commented Jun 13, 2024

This implement the point 2️⃣ and 4️⃣ in my wishlist (#2102)

This PR implement a new flag --home and remove multiple flags like: --output-path or --data-dir, --config-path which could be confusing while using gnoland binary.

All config is under the directory specified by --home

⚠️ genesis is by default under {{ --home }}/genesis.json 🎉

Examples:

  • gnoland genesis generate --output-path $GENESIS_FILE
$ gnoland genesis generate
Genesis successfully generated at ./gnoland-data/genesis.json

$ gnoland genesis generate --genesis /tmp/genesis.json                                                                                               ✔
Genesis successfully generated at /tmp/genesis.json
  • gnoland config set --config-path ./gnoland-data/config/config.toml rpc.laddr "tcp://0.0.0.0:26657"
$ gnoland config set rpc.laddr "tcp://0.0.0.0:26657"

$ gnoland config set  --home ~/gnoland-data rpc.laddr "tcp://0.0.0.0:26657"

@albttx albttx self-assigned this Jun 13, 2024
@github-actions github-actions bot added 📦 🌐 tendermint v2 Issues or PRs tm2 related 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Jun 13, 2024
Copy link

codecov bot commented Jun 13, 2024

Codecov Report

Attention: Patch coverage is 84.92063% with 19 lines in your changes missing coverage. Please review.

Project coverage is 54.96%. Comparing base (0e3c050) to head (0458677).
Report is 186 commits behind head on master.

Files with missing lines Patch % Lines
gno.land/cmd/gnoland/start.go 66.66% 2 Missing and 3 partials ⚠️
gno.land/cmd/gnoland/genesis_generate.go 40.00% 1 Missing and 2 partials ⚠️
gno.land/cmd/gnoland/config_init.go 60.00% 0 Missing and 2 partials ⚠️
gno.land/cmd/gnoland/config_set.go 66.66% 0 Missing and 1 partial ⚠️
gno.land/cmd/gnoland/genesis_balances_add.go 50.00% 0 Missing and 1 partial ⚠️
gno.land/cmd/gnoland/genesis_balances_remove.go 50.00% 0 Missing and 1 partial ⚠️
gno.land/cmd/gnoland/genesis_txs_add_packages.go 50.00% 0 Missing and 1 partial ⚠️
gno.land/cmd/gnoland/genesis_txs_add_sheet.go 50.00% 0 Missing and 1 partial ⚠️
gno.land/cmd/gnoland/genesis_txs_remove.go 50.00% 0 Missing and 1 partial ⚠️
gno.land/cmd/gnoland/genesis_validator_add.go 50.00% 0 Missing and 1 partial ⚠️
... and 2 more
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #2355      +/-   ##
==========================================
- Coverage   54.99%   54.96%   -0.04%     
==========================================
  Files         595      595              
  Lines       79775    79718      -57     
==========================================
- Hits        43872    43815      -57     
  Misses      32581    32581              
  Partials     3322     3322              
Flag Coverage Δ
contribs/gnodev 25.65% <ø> (-0.35%) ⬇️
contribs/gnofaucet 14.46% <ø> (ø)
contribs/gnokeykc 0.00% <ø> (ø)
contribs/gnomd 0.00% <ø> (ø)
gno.land 63.77% <84.92%> (-0.39%) ⬇️
gnovm 60.20% <ø> (ø)
misc/autocounterd 0.00% <ø> (ø)
misc/genproto 0.00% <ø> (ø)
misc/genstd 80.54% <ø> (ø)
misc/goscan 0.00% <ø> (ø)
misc/logos 17.38% <ø> (ø)
misc/loop 0.00% <ø> (ø)
tm2 54.46% <ø> (-0.04%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

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

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.

After doing a node deployment myself, I agree this is a good way to go, as for using genesis.json I've had to do quite a bit of a sing and dance to get it working.

cc @sw360cab for his first review :)

Comment on lines +19 to +23
type rootCfg struct {
homeDir homeDirectory
}
Copy link
Member

Choose a reason for hiding this comment

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

please, for flag structs, don't nest them. They can have a helper type like the one you created, but there shouldn't be 3 levels of nesting

Copy link
Member Author

Choose a reason for hiding this comment

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

I'm not sure to understand how you want it not nested ?

Copy link
Member

Choose a reason for hiding this comment

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

Yeah, I don't know, I had a bit of a think and discussion with Milos and I'm not sure how this should work. cc @zivkovicmilos

I don't like rootCfg 🤷

gno.land/cmd/gnoland/home.go Outdated Show resolved Hide resolved
@thehowl thehowl requested a review from sw360cab June 19, 2024 14:23
@albttx
Copy link
Member Author

albttx commented Jul 18, 2024

@thehowl @zivkovicmilos it's rebased and i fixed your reviews

gno.land/cmd/gnoland/root.go Outdated Show resolved Hide resolved
@@ -184,24 +126,24 @@ func TestSecrets_Verify_All_Missing(t *testing.T) {
initArgs := []string{
"secrets",
"init",
"--data-dir",
tempDir,
"--home",
Copy link
Member

Choose a reason for hiding this comment

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

If we use a single flag, I think we should use —data

Copy link
Member Author

Choose a reason for hiding this comment

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

I believe it could lead to confusions, because the folder contains datas, configurations, secrets...

@@ -28,7 +28,6 @@ func NewRootCmdWithBaseConfig(io commands.IO, base BaseOptions) *commands.Comman
ShortUsage: "<subcommand> [flags] [<arg>...]",
LongHelp: "Manages private keys for the node",
Options: []ff.Option{
ff.WithConfigFileFlag("config"),
Copy link
Member

Choose a reason for hiding this comment

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

Why?

Copy link
Member Author

Choose a reason for hiding this comment

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

because --config is replaced with --home, which is not a file

@moul
Copy link
Member

moul commented Jul 18, 2024

Adding genesis, config, and secret commands to the gnoland command was not the best move. They belong in a separate tool like contribs/gnolandtools, with fewer files involved. Or maybe they should just not exist.

The gnoland binary is meant to be a stable, finished product, not an SDK for running multiple blockchains. We're not going to be tweaking genesis files daily. Gno.land will have its mainnet, testnets, and a local setup that's either lazy or uses the latest snapshot. Other projects might create their own lands, but they won't be using the gnoland binary for that. Most of the configuration will be managed by the chain too.

In other words, 99.9% of people will use lazy or download a ready-to-start state.

Here's what we really need:

  • A solid --lazy option for easy setup.
  • A snapshot feature for gnodev. (100x better than gnoland genesis xxx commands)
  • A reliable editor to patch the genesis file.
  • Clear documentation and basic tool for managing secret keys.
  • Keeping gnoland light.

@albttx
Copy link
Member Author

albttx commented Jul 26, 2024

@moul I understand what you means, but i really think those commands are required from an ops perspective.

Creating a genesis file for a multi node cluster with gnoland start --lazy -> ctrl+c is just impossible.

You already told me this commands will come in a gno-sdk which i believe could be the right place, maybe for the moment let's keep it inside of the gnoland binary, but once ready, we could move it into this gno-sdk ?

about the rename from --home into --data, i believe it could lead to confusions, because the folder contains datas, configurations, secrets...

Copy link

This PR is stale because it has been open 3 months with no activity. Remove stale label or comment or this will be closed in 3 months.

@github-actions github-actions bot added Stale and removed Stale labels Nov 12, 2024
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: No status
Status: In Review
Development

Successfully merging this pull request may close these issues.

3 participants