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

gnoland start --config flag is tied to the flag configuration and not the node config #1234

Closed
zivkovicmilos opened this issue Oct 11, 2023 · 5 comments · Fixed by #1240
Closed
Assignees
Labels
🐞 bug Something isn't working 📦 ⛰️ gno.land Issues or PRs gno.land package related

Comments

@zivkovicmilos
Copy link
Member

Description

The configuration flag value specified in the gnoland start command:

fs.StringVar(
&c.config,
"config",
"",
"config file (optional)",
)
}

is actually tied to the ffcli config (flag configuration for the gnoland start command), and not the node configuration:

ff.WithConfigFileFlag("config"),

@zivkovicmilos zivkovicmilos added 🐞 bug Something isn't working 📦 ⛰️ gno.land Issues or PRs gno.land package related labels Oct 11, 2023
@thehowl
Copy link
Member

thehowl commented Oct 11, 2023

Shall we change it and make it tied to the node config file?

This way we could also compromise on the "scriptability" Antonio was worrying about. ie. we can run gnoland like this:

gnoland -config /dev/stdin << EOF
config=file
values=1
EOF

@moul
Copy link
Member

moul commented Oct 11, 2023

To maintain consistency with other command-line interfaces (CLIs), I propose the following approach:

  • Keep the flag "-config" as the general configuration file.
  • Embed the TM2 config and prefix all its components.
type opts struct {
    Verbose    bool
    OtherFlag  string
    TM2Config  tm2config.Config
}

Then, register flags using the prefix --tm2-. This approach preserves the current behavior of manually configuring files while providing the option to patch them using flags. By adopting this method, we ensure consistency across your CLIs.

You can find more details about this approach in the description provided in issue #731.

@moul
Copy link
Member

moul commented Oct 11, 2023

Another approach is to introduce a gnoland init function that handles the customization (creation or patching) of the configuration file.

Subsequently, the gnoland start command can utilize the existing LazyLoadOrCreate method, which would render gnoland init optional, unless you intend to personalize your node.

@zivkovicmilos
Copy link
Member Author

@moul

I am on the fence about this, because I support both of the approaches you've mentioned.

I've always utilized specific flags for node configuration, along with support for a config file (you have the file -> env -> flags precedence here, or whatever the order is). The thing I dislike about this approach is that you insanely bloat the start command, which should be mega simple, while giving the user power to configure.

What do you think about this, as a compromise:

  • introduce a command suite for generating the node config and modifying it (we don't have this now)
  • make gnoland start take a --tm2-config flag, that is the path to the configuration. If no configuration is provided, the one from the current working directory is loaded (or a default one is generated and loaded). This last part details how we currently have the config file for the node working

@zivkovicmilos
Copy link
Member Author

I will open up a PR for implementing this fix, since it's blocking us on some efforts

@zivkovicmilos zivkovicmilos self-assigned this Oct 12, 2023
@zivkovicmilos zivkovicmilos moved this from Backlog to In Progress in 🚪🔁 The Portal Loop Oct 12, 2023
@moul moul moved this to 🌟 Wanted for Launch in 🚀 The Launch [DEPRECATED] Oct 13, 2023
@zivkovicmilos zivkovicmilos moved this from In Progress to In Review in 🚪🔁 The Portal Loop Oct 13, 2023
zivkovicmilos added a commit that referenced this issue Oct 20, 2023
## Description

This PR adds support for specifying a custom node configuration file
using the `--tm2-node-config` flag.

Resolves #1234

<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
- [x] Provided any useful hints for running manual tests
- [x] 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>
@github-project-automation github-project-automation bot moved this from In Review to Done in 🚪🔁 The Portal Loop Oct 20, 2023
thehowl pushed a commit to thehowl/gno that referenced this issue Oct 21, 2023
## Description

This PR adds support for specifying a custom node configuration file
using the `--tm2-node-config` flag.

Resolves gnolang#1234

<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
- [x] Provided any useful hints for running manual tests
- [x] 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>
gfanton pushed a commit to gfanton/gno that referenced this issue Nov 9, 2023
## Description

This PR adds support for specifying a custom node configuration file
using the `--tm2-node-config` flag.

Resolves gnolang#1234

<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
- [x] Provided any useful hints for running manual tests
- [x] 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>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
🐞 bug Something isn't working 📦 ⛰️ gno.land Issues or PRs gno.land package related
Projects
Status: 🌟 Wanted for Launch
Status: Done
Development

Successfully merging a pull request may close this issue.

3 participants