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

internal/cli: add support to overwrite config.toml values via cli flags #1008

Merged
merged 8 commits into from
Sep 21, 2023

Conversation

manav2401
Copy link
Contributor

@manav2401 manav2401 commented Sep 16, 2023

Description

With the new-cli introduced in v0.3.0, there were 2 ways to set the configuration required to run bor. One can either do it via normal cli flags (args) or via a toml based config. Due to some restrictions, if the toml based config is provided, bor overwrites all the given cli flags (explicitly by the user) with those provided in toml config.

An ideal behaviour should in any cli application should be that explicitly provided flags should take control no matter what value is provided in the toml based config. This PR implements that. Sufficient tests have been added and existing tests have been improved to cover all cases for all data types.

The order of flag will now be as follows:

  1. If set in cli args, use that value
  2. If not set in cli args and set in config, use that value
  3. If not set in any of them, use default value

Example:
If the toml config looks something like this:

chain = "mainnet"
identity = "Anon"
datadir = "config_datadir"
### More values

and the user runs the following command

bor server --config=config.toml --datadir=cli_datadir

Currently bor would set the datadir the value mentioned in config i.e. config_datadir.

With this PR, the value set for datadir would be cli_datadir.

This is a breaking change as it changes the way people run their bor nodes. It mostly affects those who run a combination of both. Until now, their cli flags weren't set if config was set but now onwards, their additional flags will also be set. This might require an explicit communication to node operators.

Changes

  • Bugfix (non-breaking change that solves an issue)
  • Hotfix (change that solves an urgent issue, and requires immediate attention)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (change that is not backwards-compatible and/or changes current functionality)
  • Changes only for a subset of nodes

Breaking changes

As mentioned above, it changes the way flags are parsed and handled. Cli args will overwrite config flags.

Checklist

  • I have added at least 2 reviewer or the whole pos-v1 team
  • I have added sufficient documentation in code
  • I will be resolving comments - if any - by pushing each fix in a separate commit and linking the commit hash in the comment reply
  • Created a task in Jira and informed the team for implementation in Erigon client (if applicable)
  • Includes RPC methods changes, and the Notion documentation has been updated

Testing

  • I have added unit tests
  • I have added tests to CI
  • I have tested this code manually on local environment
  • I have tested this code manually on remote devnet using express-cli
  • I have tested this code manually on mumbai
  • I have created new e2e tests into express-cli

Manual tests

Manual testing of flags with different datatypes have been carried to make sure it works for all types of values.

@manav2401 manav2401 requested a review from a team September 18, 2023 12:01
Copy link
Member

@pratikspatil024 pratikspatil024 left a comment

Choose a reason for hiding this comment

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

Noice!

@manav2401 manav2401 merged commit 8613ff1 into develop Sep 21, 2023
10 checks passed
@manav2401 manav2401 deleted the manav/overwrite-cli-flags branch September 21, 2023 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants