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

Add support for keyfiles with -ethKeystorePath, update flag descriptions, flagset output to stdout #2713

Merged
merged 25 commits into from
Feb 13, 2023

Conversation

eliteprox
Copy link
Contributor

@eliteprox eliteprox commented Jan 6, 2023

What does this pull request do? Explain your changes. (required)

  • Updates various flag descriptions that a file can be used in place of a string.
  • Changes the default flagset output to os.StdOut. This has the effect of enabling users to grep --help output,
  • Add support for keystore file with -ethKeystorePath. This can be used alternatively to ethAcctAddr
    • ETH address is parsed from keyfile. -ethAcctAddr is optional, but if provided the address must match with the one found in the keyfile.

Specific updates (required)

  • Updated flag descriptions for OrchSecret, EthAcctAddr, EthPassword, EthKeystorePath and PricePerBroadcaster
  • Changed command line output for flagset to os.StdOut
  • Parse ETH address from keyfile when a file is provided to -ethKeystorePath
  • Validates ethAcctAddr and keystore address match or that ethAcctAddr is not provided, before using address from keystore
  • Added test coverage

How did you test each of these updates (required)
I tested the ethKeystorePath and ethAcctAddr flags in the following ways:

  • With a directory and with a valid file. Eth account is properly found from keystore
  • When set to a directory: keystoreDir was set to same path and ethAcctAddr flag still works for selecting eth account.
  • When set to a valid key file: keystoreDir was set to parent directory of the file and ETH address is parsed. If -ethAcctAddr flag is also provided, the address parsed from keyfile must match the ethAcctAddr, otherwise error: -ethKeystorePath and -ethAcctAddr were both provided, but ethAcctAddr does not match the address found in keystore
  • With a missing file or directory: provided -ethKeystorePath was not found
  • With invalid json in file: error opening keystore

Does this pull request close any open issues?
fix #2689 cli help is printed to stderr instead of stdout
fix #2690 cli help didn't mention -ethPassword and -orchSecret can be file path
fix #2691 Usage for -ethKeystorePath option is confusing

Checklist:

@codecov
Copy link

codecov bot commented Jan 6, 2023

Codecov Report

Merging #2713 (e71c828) into master (f07d69d) will decrease coverage by 0.02593%.
The diff coverage is 42.30769%.

❗ Current head e71c828 differs from pull request most recent head 0710709. Consider uploading reports for the commit 0710709 to get more accurate results

Impacted file tree graph

@@                 Coverage Diff                 @@
##              master       #2713         +/-   ##
===================================================
- Coverage   56.34825%   56.32232%   -0.02593%     
===================================================
  Files             88          88                 
  Lines          19147       19186         +39     
===================================================
+ Hits           10789       10806         +17     
- Misses          7767        7788         +21     
- Partials         591         592          +1     
Impacted Files Coverage Δ
common/util.go 60.29851% <0.00000%> (-1.28686%) ⬇️
cmd/livepeer/starter/starter.go 4.29338% <43.58974%> (+1.44149%) ⬆️
cmd/livepeer/livepeer.go 47.34043% <83.33333%> (-0.25315%) ⬇️

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 f07d69d...0710709. Read the comment docs.

Impacted Files Coverage Δ
common/util.go 60.29851% <0.00000%> (-1.28686%) ⬇️
cmd/livepeer/starter/starter.go 4.29338% <43.58974%> (+1.44149%) ⬆️
cmd/livepeer/livepeer.go 47.34043% <83.33333%> (-0.25315%) ⬇️

@joegraviton
Copy link

Thanks for fixing my 3 issues in batch 👍

@leszko
Copy link
Contributor

leszko commented Jan 10, 2023

@eliteprox Thanks for the PR. Mark as ready when you think you finalized it. Then, we'll review and comment on this.

@eliteprox eliteprox marked this pull request as ready for review January 14, 2023 05:13
@eliteprox
Copy link
Contributor Author

@leszko This PR is ready for review.

@leszko leszko self-requested a review January 17, 2023 15:56
@leszko
Copy link
Contributor

leszko commented Jan 17, 2023

@leszko This PR is ready for review.

Perfect. Thank you. I'll review it.

Copy link
Contributor

@leszko leszko left a comment

Choose a reason for hiding this comment

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

I added a few comments. Most of them are minor.

From the more important ones, I think we should fail-fast in the following scenarios:

  • EthKeystorePath points to a non-existing file
  • EthAcctAddr and EthKeystorePath are both defined but they point different ETH addresses

cmd/livepeer/livepeer.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter.go Outdated Show resolved Hide resolved
common/util.go Show resolved Hide resolved
Copy link
Contributor

@leszko leszko left a comment

Choose a reason for hiding this comment

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

Added some super minor comments. Please take a look and address and think it's ready for the merge. Good work 👍

cmd/livepeer/starter/starter.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter.go Show resolved Hide resolved
cmd/livepeer/starter/starter.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter_test.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter_test.go Outdated Show resolved Hide resolved
cmd/livepeer/starter/starter_test.go Outdated Show resolved Hide resolved
common/util.go Show resolved Hide resolved
common/util.go Show resolved Hide resolved
@leszko
Copy link
Contributor

leszko commented Feb 9, 2023

@eliteprox when you address all comments, please click "Re-request review"

@leszko leszko mentioned this pull request Feb 13, 2023
Copy link
Contributor

@leszko leszko left a comment

Choose a reason for hiding this comment

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

Added one more comment, could you check?

Copy link
Contributor

@leszko leszko left a comment

Choose a reason for hiding this comment

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

LGTM, thanks for all the changes @eliteprox

I'll merge as soon as the CI build is completed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants