-
Notifications
You must be signed in to change notification settings - Fork 72
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
[DRAFT] Improvements on env variables awareness and update of the related documentation #1843
base: main
Are you sure you want to change the base?
Conversation
Kudos, SonarCloud Quality Gate passed! 0 Bugs No Coverage information |
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.
Thanks.
Some point of clarifications
@@ -0,0 +1,113 @@ | |||
# Default configuration if not overridden. |
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.
Does the existing Configuration.md not satisfy this or does it need further emphasis.
This file would be a dupe of that and would require extra effort to maintain on top of that README
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.
Is another approach to note the Config README in the env.sample
file?
@@ -1,5 +1,5 @@ | |||
HEDERA_NETWORK=mainnet | |||
HEDERA_NETWORK="mainnet" |
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.
Q: why was this needed? Isn't it inherently string already?
@Neurone please see comments above |
The base idea of this PR is to relieve the burden of updating Configuration.md - now out of synchronization with reality (the code) and other parts of the documentation (i.e., the README) - and to provide security, consistency of behavior and predictability to the application and its dependencies. The application flow should be changed like this:
Advantages of this flow
I spoke with Alfredo today about this PR. If you like the overall idea, we can keep it, eventually with another implementation if you don't like the idea of the If you like the idea and the solution, I can continue with this PR (sooner or later ^^) and finish it. It's not trivial or complex, I would say "doable", but it requires extensive checks nonetheless. If you don't like the idea, we can close this. |
Description:
Some developers have problems understanding the default configuration values used by the JSON-RPC Relay and all the possible configurations.
This PR wants to address that issue, and in particular:
.env.defaults
file listing all the env variables used by the application and their - explicit or computed - default valuesRelated issue(s):
N/A
Notes for reviewer:
Still in draft, don't review.
Checklist