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

cmd: Add config file support #2137

Merged
merged 5 commits into from
Dec 17, 2021
Merged

Conversation

leszko
Copy link
Contributor

@leszko leszko commented Dec 16, 2021

What does this pull request do? Explain your changes. (required)
Add a possibility to configure Livepeer using a config file instead of flags.

Sample usage:

$ livepeer -config livepeer.conf

Sample livepeer.conf

broadcaster
cliAddr :7937
httpAddr :8938
orchAddr 127.0.0.1:8935,127.0.0.1:8936
v 6

Specific updates (required)
Used the library https://github.com/peterbourgon/ff, which allows using config file for the flags.

How did you test each of these updates (required)

  1. Created a sample config file and checked if it works
  2. Checked if both config files and flags are defined if flags take precedence over config file

Does this pull request close any open issues?
fix #101

Checklist:

@hthillman
Copy link
Contributor

Checked if both config files and flags are defined if flags take precedence over config file

Just to be super clear here - if flags are defined, they'll override the config?

Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

Looks good! Just one minor question related to a comment.

I also think it would be great to add a short guide for configuring livepeer using a config file in https://github.com/livepeer/livepeer-org/tree/master (the repo for https://livepeer.org/docs). Maybe a section called "Configuring Livepeer" under the "Installation" header...

Also, just want to confirm my understanding of the ff package:

Since the package will parse flags, env vars and the config file the following should be true right?

  • If -maxGasPrice is specified in all three, the value used in the flag takes precedence
  • If -maxGasPrice is not specified in flags, but other flags are specified and the config file specifies -maxGasPrice then the value from the config file will be used

EDIT: Regarding the above question - just read your testing steps! 😓

cmd/livepeer/livepeer.go Outdated Show resolved Hide resolved
@leszko
Copy link
Contributor Author

leszko commented Dec 16, 2021

Checked if both config files and flags are defined if flags take precedence over config file

Just to be super clear here - if flags are defined, they'll override the config?

Yes, they'll override the config.

@leszko
Copy link
Contributor Author

leszko commented Dec 17, 2021

I also think it would be great to add a short guide for configuring livepeer using a config file in https://github.com/livepeer/livepeer-org/tree/master (the repo for https://livepeer.org/docs). Maybe a section called "Configuring Livepeer" under the "Installation" header...

Yes, working on this together with @hthillman in the branch docs-config-updates of the https://github.com/livepeer/livepeer-org repo. I think we can merge this PR and later merge the doc-related PR.

Also, just want to confirm my understanding of the ff package:

Since the package will parse flags, env vars and the config file the following should be true right?

  • If -maxGasPrice is specified in all three, the value used in the flag takes precedence
  • If -maxGasPrice is not specified in flags, but other flags are specified and the config file specifies -maxGasPrice then the value from the config file will be used

EDIT: Regarding the above question - just read your testing steps! 😓

Yeah, it works exactly as you described. One more thing is that after the comment from @iameli , we added the env variables. So the priority here is (from the highest):

  • flags
  • env variables
  • config file

Copy link
Member

@yondonfu yondonfu left a comment

Choose a reason for hiding this comment

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

LGTM!

The ability to use env vars for config is a bonus - nice!

Copy link
Member

@iameli iameli left a comment

Choose a reason for hiding this comment

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

LGTM!

@leszko leszko merged commit 832166f into livepeer:master Dec 17, 2021
@leszko leszko deleted the 101-support-config-file branch December 17, 2021 16:07
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.

Support Config File
4 participants