-
Notifications
You must be signed in to change notification settings - Fork 20.4k
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
cmd/puppeth: new version as presented at devcon3 #15390
Conversation
c20260e
to
c750fad
Compare
c750fad
to
b065648
Compare
b065648
to
79a46be
Compare
79a46be
to
b169a30
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
From my limited perspective, it looks good to me. I would rely more on external templates and try to reduce code duplication. Cool tool though, glad I looked at the devcon pressentation!
) | ||
|
||
// explorerDockerfile is the Dockerfile required to run a block explorer. | ||
var explorerDockerfile = ` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I see that you have a lot of these files embedded into go files. How about using something like Ego ? https://github.com/benbjohnson/ego
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did consider using external file templates and perhaps embedding them with go generate
and go-bindata
. But truth be told these are fairly small files, so it's less of a hassle to just keep them embedded directly and have a single place to edit and debug them.
cmd/puppeth/module_node.go
Outdated
// String implements the stringer interface. | ||
func (info *nodeInfos) String() string { | ||
discv5 := "" | ||
// Report converts the typed struct into a plain string->string map, cotnaining |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
typo: "cotnaining"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good catch, fixed all occurrences.
cmd/puppeth/wizard_faucet.go
Outdated
@@ -46,9 +47,10 @@ func (w *wizard) deployFaucet() { | |||
minutes: 1440, | |||
tiers: 3, | |||
} | |||
existed = false |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so basically
existed := err == nil
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yup, much cleaner, fixed all occurrences.
@gballet Thanks for the review! PTAL :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
Ideal to have before merging:
faucet
source without needing an external repo (puppeth/go-ethereum).