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

Ability to read initial config from environment. #48

Merged
merged 2 commits into from
Dec 3, 2021
Merged

Ability to read initial config from environment. #48

merged 2 commits into from
Dec 3, 2021

Conversation

ibz
Copy link
Contributor

@ibz ibz commented Nov 29, 2021

No description provided.

@edouardparis
Copy link
Owner

Thank you for the contribution, please can you specify in the README https://github.com/edouardparis/lntop#config under the config section the three env var introduced in this PR ?

@ibz
Copy link
Contributor Author

ibz commented Nov 29, 2021

@edouardparis Yep, done!

@ibz
Copy link
Contributor Author

ibz commented Nov 29, 2021

However, if I think about it, it's kinda strange to accept env vars, yet only take them into account if the config file is missing. I think it would make more sense to have them override the values read from config also. What do you think?

@edouardparis
Copy link
Owner

I think the PR is good as it is for the current usage.
I am not sure about the overriding, It may introduce some confusion with values that the user specified in the config which in my opinion should have priority.

After thinking more about it, the .lntop/config.toml creation at start up was a mistake which should be fixed in next versions.
I think everything needs to be more explicit and reflects the lncli behaviour:
https://github.com/lightningnetwork/lnd/blob/master/cmd/lncli/main.go#L263

  • lntop: starts lntop with all default values
  • lntop --rpcserver=<...> --tlscertpath=<...> --macaroonpath=<...> starts lntop with specified values
  • lntop --conf <...> starts with the config values

The user then creates an alias the .bashrc according to his needs.

@ibz
Copy link
Contributor Author

ibz commented Nov 30, 2021

Yes, I agree. Creating a config file with default values seems to do more harm than good.

At least, having the env vars (whether to be used as default values in the config file, or to override the config file completely) fixes the problem that we have now, in that one cannot start lntop without 1) getting an error message 2) editing the config file 3) starting it again, which is very confusing.

@ibz
Copy link
Contributor Author

ibz commented Dec 3, 2021

@edouardparis Do you think we can merge this for now?

@edouardparis
Copy link
Owner

Yes sorry, send me a link for a tip

@edouardparis
Copy link
Owner

Thank you again

@edouardparis edouardparis merged commit a05908a into edouardparis:master Dec 3, 2021
@ibz
Copy link
Contributor Author

ibz commented Dec 3, 2021

Thank you for lntop!

@ibz
Copy link
Contributor Author

ibz commented Dec 3, 2021

Shall we make an issue for changing the parameters/configuration behavior as discussed above?

@edouardparis
Copy link
Owner

I am creating one, thank you for the reminder

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.

2 participants