Skip to content
This repository has been archived by the owner on Oct 7, 2020. It is now read-only.

Parse configs from toml file - #223 #224

Closed
wants to merge 24 commits into from

Conversation

nodech
Copy link
Contributor

@nodech nodech commented Aug 3, 2017

Implements: #223

Integrated codebase from go-ethereum for parsing toml files.

Now we can remove configs flag and expect it in the home and remove flags.
Where should I put example toml file ?

nodech and others added 23 commits July 11, 2017 16:59
When Tendermint starts it is not harnessed to an ABCI app, the tutorial should reassure them that this is normal until Ethermint is started.
Reassure users that error output is expected
Signed-off-by: Adrian Brink <[email protected]>
Signed-off-by: Adrian Brink <[email protected]>
… #stable is covered by semver, everything else can change.

Signed-off-by: Adrian Brink <[email protected]>
@nodech nodech requested a review from adrianbrink August 3, 2017 22:50
@codecov
Copy link

codecov bot commented Aug 3, 2017

Codecov Report

Merging #224 into unstable will decrease coverage by 6.23%.
The diff coverage is 10.95%.

Impacted file tree graph

@@             Coverage Diff              @@
##           unstable     #224      +/-   ##
============================================
- Coverage     40.42%   34.18%   -6.24%     
============================================
  Files             5        5              
  Lines           282      351      +69     
============================================
+ Hits            114      120       +6     
- Misses          149      208      +59     
- Partials         19       23       +4
Impacted Files Coverage Δ
cmd/utils/config.go 35.93% <10.95%> (-31.86%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9c6c201...9d8e69f. Read the comment docs.

Node node.Config
Ethstats ethstatsConfig
type additionalConfigs struct {
UnlockAccounts []string `toml:",omitempty"`
Copy link
Contributor

Choose a reason for hiding this comment

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

Am not familiar with toml, but toml:",omitempty" for all the struct fields seems a little off to me.

@@ -91,6 +136,23 @@ func DefaultNodeConfig() node.Config {
return cfg
}

// LoadConfig takes configuration file path and full configuration and applies file configs to it
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we make this:
LoadConfig reads a configuration from disk and saves it into cfg

Minor not: also perhaps for easier parameter reading, make cfg something like
saveCfg or so and if that change applies, apply it to the comment above, please.

}
defer f.Close() // nolint: errcheck

err = tomlSettings.NewDecoder(bufio.NewReader(f)).Decode(cfg)
Copy link
Contributor

Choose a reason for hiding this comment

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

The signature http://godoc.org/github.com/naoina/toml#Config.NewDecoder is:

func (*toml.Config).NewDecoder(io.Reader)

so I don't think we need the bufio.NewReader(f) since *os.File is already an io.Reader, unless there is something special about that decoder that it needs buffered io.

defer f.Close() // nolint: errcheck

err = tomlSettings.NewDecoder(bufio.NewReader(f)).Decode(cfg)
// Add file name to errors that have a line number.
Copy link
Contributor

Choose a reason for hiding this comment

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

Nice!

}
lines := strings.Split(string(text), "\n")
// Sanitise DOS line endings.
for i := range lines {
Copy link
Contributor

Choose a reason for hiding this comment

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

Might be a lot easier to read if for a non channel we didn't use i := range lines
so

for i := 0; i < len(lines); i++

comment += "# Note: this config doesn't contain the genesis block.\n\n"
}

out, err := tomlSettings.Marshal(&cfg)
Copy link
Contributor

Choose a reason for hiding this comment

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

Double indirection here, cfg is already a pointer i.e *GethConfig


// DumpConfig dumps toml file from the configurations to stdout
// #unstable
func DumpConfig(cfg *GethConfig) error {
Copy link
Contributor

Choose a reason for hiding this comment

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

This is a nice helper but we could perhaps make it more configurable like this

// FdumpTomlConfig serializes cfg to the output writer
func FdumpTomlConfig(f io.Writer, cfg *GethConfig) error {
   comment := ""
   if cfg.Eth.Genesis != nil {
      cfg.Eth.Genesis = nil
      comment += "# Note: this config doesn't contain the genesis block.\n\n"
   }

   out, err := tomlSettings.Marshal(cfg)
   if err != nil {
      return err
   }
   io.WriteString(w, comment)
   _, err = w.Write(out)
   return err
}

and then

// DumpTomlConfig is a helper that serializes cfg to stdout
func DumpTomlConfig(cfg *GethConfig) error {
   return FdumpTomlConfig(os.Stdout, cfg)
}

and then we can also make unit tests to test that FdumpTomlConfig works well and matches our expectations.

Copy link
Contributor

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

Hello @nodar-chkuaselidze, thank you for the PR! I am getting familiar with Ethermint, so please pardon me if I suggested the wrong thing, but I've left some comments that perhaps could be applied to the code.

@nodech
Copy link
Contributor Author

nodech commented Aug 5, 2017

Well, DumpConfig and config parsing stuff is directly taken from geth. I was more concentrated on moving flags to config file and making them work than improving go-ethereum's implementation.

@odeke-em
Copy link
Contributor

odeke-em commented Aug 5, 2017

@nodar-chkuaselidze oh I see, my bad. In that case, please feel free to get your changes in for parity with geth, and then I'll submit a follow up PR adding the changes and tag you as a review if you are free. Am new around here so just learning how things work :)

@adrianbrink adrianbrink reopened this Aug 14, 2017
@adrianbrink adrianbrink changed the title Parse configs from file as in geth WIP: Parse configs from toml file - #223 Aug 14, 2017
@adrianbrink adrianbrink changed the base branch from unstable to develop August 17, 2017 15:32
Copy link
Contributor

@odeke-em odeke-em left a comment

Choose a reason for hiding this comment

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

To avoid blocking you as mentioned in my last comment, I'll remove my requested changes :)

@adrianbrink adrianbrink changed the title WIP: Parse configs from toml file - #223 Parse configs from toml file - #223 Aug 24, 2017
@adrianbrink adrianbrink self-assigned this Aug 24, 2017
@adrianbrink
Copy link

This PR will not be merged in this form. I would like to thank everyone that contributed, but the new proposed structure in #295 will tackle this separately.

i-norden pushed a commit to vulcanize/old_ethermint that referenced this pull request Aug 31, 2020
Bumps [github.com/spf13/cobra](https://github.com/spf13/cobra) from 0.0.5 to 0.0.6.
- [Release notes](https://github.com/spf13/cobra/releases)
- [Commits](spf13/cobra@0.0.5...v0.0.6)

Signed-off-by: dependabot-preview[bot] <[email protected]>

Co-authored-by: dependabot-preview[bot] <27856297+dependabot-preview[bot]@users.noreply.github.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants