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

yaml configuration for booting the each nodes #1684

Merged
merged 25 commits into from
Jul 27, 2023

Conversation

kjeom
Copy link
Contributor

@kjeom kjeom commented Nov 11, 2022

Proposed changes

  • Migration from urfave/cli.v1 to urfave/cli/v2

    • See https://cli.urfave.org/migrate-v1-to-v2/ for full list of changes.
    • Changes to our codebase:
      • Specify flag aliases as Flag.Aliases []string
      • Specify flag envs as Flag.EnvVars []string
      • Flags are now pointers &cli.BoolFlag{ ... }
      • Commands are now pointers &cli.Command{ ... }
      • No ctx.GlobalString(). There is only ctx.String(). Same goes for each flag types.
  • Flag positioning

    • Command - Flag - Args order is now important.
      kcn init --datadir mydata genesjs.json   # cmd-flag-args OK
      kcn --datadir mydata init genesis.json   # flag-cmd-args OK
      kcn init genesis.json --datadir mydata   # cmd-args-flag BAD
  • Load flags from a YAML file

    • ken --conf config.yaml
    • The YAML file contains the (key, value) pairs where the key is a flag name or alias.
  • Refactor

    • Move file cmd/utils/nodecmd/nodeflags.go to cmd/utils/nodeflags.go
    • Move function checkExclusive to utils.CheckExclusive

Types of changes

Please put an x in the boxes related to your change.

  • Bugfix
  • New feature or enhancement
  • Others

Checklist

Put an x in the boxes that apply. You can also fill these out after creating the PR. If you're unsure about any of them, don't hesitate to ask. We're here to help! This is simply a reminder of what we are going to look for before merging your code.

  • I have read the CONTRIBUTING GUIDELINES doc
  • I have signed the CLA
  • Lint and unit tests pass locally with my changes ($ make test)
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)
  • Any dependent changes have been merged and published in downstream modules

Related issues

  • Please leave the issue numbers or links related to this PR here.

Further comments

If this is a relatively large or complex change, kick off the discussion by explaining why you chose the solution you did and what alternatives you considered, etc...

nohkwak
nohkwak previously approved these changes Nov 16, 2022
cmd/homi/setup/flags.go Outdated Show resolved Hide resolved
cmd/utils/flags.go Outdated Show resolved Hide resolved
cmd/utils/flags.go Outdated Show resolved Hide resolved
@kjeom kjeom requested a review from ehnuje as a code owner November 17, 2022 09:38
blukat29
blukat29 previously approved these changes Nov 18, 2022
Copy link
Contributor

@blukat29 blukat29 left a comment

Choose a reason for hiding this comment

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

LGTM except merge conflict

@blukat29 blukat29 modified the milestones: v1.10 (Kore), v1.11 (Mantle) Nov 22, 2022
@yoomee1313 yoomee1313 requested review from ian0371 and removed request for ehnuje, 2dvorak, KimKyungup, jeongkyun-oh and JayChoi1736 December 27, 2022 08:43
cmd/utils/config.go Outdated Show resolved Hide resolved
cmd/utils/config.go Outdated Show resolved Hide resolved
cmd/utils/config.go Outdated Show resolved Hide resolved
cmd/utils/config.go Outdated Show resolved Hide resolved
@blukat29
Copy link
Contributor

@kjeom
There are some cases where GlobalIsSet is replaced with Bool.
Can we keep it IsSet in this PR, and refactor like below in another PR?

- if ctx.IsSet(..) {
-     cfg.Item = ctx.Bool(..)
- }
+ cfg.Item = ctx.Bool(..)
- cfg.Item = ctx.IsSet(..)
+ cfg.Item = ctx.Bool(..)

@kjeom
Copy link
Contributor Author

kjeom commented Jul 27, 2023

@kjeom There are some cases where GlobalIsSet is replaced with Bool. Can we keep it IsSet in this PR, and refactor like below in another PR?

- if ctx.IsSet(..) {
-     cfg.Item = ctx.Bool(..)
- }
+ cfg.Item = ctx.Bool(..)
- cfg.Item = ctx.IsSet(..)
+ cfg.Item = ctx.Bool(..)

Sure and any issues?

@blukat29
Copy link
Contributor

@kjeom Because it's difficult to review. If it's not changed I can just compare side-by-side.

@kjeom
Copy link
Contributor Author

kjeom commented Jul 27, 2023

@kjeom Because it's difficult to review. If it's not changed I can just compare side-by-side.

Ok, but I have left a comment for a case.

altsrc.NewIntFlag(RPCWriteTimeoutFlag),
altsrc.NewIntFlag(RPCIdleTimeoutFlag),
altsrc.NewIntFlag(RPCExecutionTimeoutFlag),
}
Copy link
Member

Choose a reason for hiding this comment

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

UnsafeDebugDisableFlag, RPCGlobalEVMTimeoutFlag are missing from CommonRPCFlags

Copy link
Member

@aidan-kwon aidan-kwon left a comment

Choose a reason for hiding this comment

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

LGTM except for @blukat29's command and my minor comment. Let's handle this in another PR

@kjeom kjeom merged commit 56c0fbd into klaytn:dev Jul 27, 2023
10 checks passed
@blukat29 blukat29 removed the need to merge Need to merge for the next time label Nov 14, 2023
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.

None yet

7 participants