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

[DRAFT] Improvements on env variables awareness and update of the related documentation #1843

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

Neurone
Copy link
Contributor

@Neurone Neurone commented Oct 17, 2023

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:

  • adds a complete .env.defaults file listing all the env variables used by the application and their - explicit or computed - default values
  • review of the documentation, fixing and matching the default values with those found in the source code or in external libraries (i.e., the Hedera JavaScript SDK)

Related issue(s):

N/A

Notes for reviewer:

Still in draft, don't review.

Checklist

  • Documented (Code comments, README, etc.)
  • Tested (unit, integration, etc.)

@sonarcloud
Copy link

sonarcloud bot commented Oct 17, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
0.0% 0.0% Duplication

Copy link
Collaborator

@Nana-EC Nana-EC left a 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.
Copy link
Collaborator

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

Copy link
Collaborator

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"
Copy link
Collaborator

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 Neurone changed the title [DRAFT] Improvements on env variables awarness and update of the related documentation; [DRAFT] Improvements on env variables awareness and update of the related documentation; Jan 22, 2024
@Neurone Neurone changed the title [DRAFT] Improvements on env variables awareness and update of the related documentation; [DRAFT] Improvements on env variables awareness and update of the related documentation Jan 22, 2024
@Nana-EC
Copy link
Collaborator

Nana-EC commented Jan 23, 2024

@Neurone please see comments above
If there's improvements to take we'd be happy to but don't want to add extra work.
We could put the readme link as comment in the sample file but a file with all values might be too much

@Neurone
Copy link
Contributor Author

Neurone commented Jan 23, 2024

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:

  1. The application starts up and reads the default values from the .env.defaults file
  2. The application reads the .env file to override specific values
  3. The application fails if a parameter is not set; no workarounds setting default values fixed into the source code
  4. Documentation configuration tables are generated from the .env.defaults file, both for values and comments

Advantages of this flow

  • A single file guarantees developers a single point to check and update.
  • Reading from a .env.defaults file also saves the application from changes in default values in external dependencies (i.e., Hedera SDK) and can ease versioning, comparison, and backup of configurations.
  • Removing fallback values fixed in the source code makes the application more flexible, secure, and easier to write and debug.
  • Making an application fail in case of non-configured parameters is both a fixed behavior developers can implement across the project and a way to make the application more robust and prevent wrong overriding (i.e., using a file that sets a void variable)
  • Generating the documentation from the .env.defaults file can be automated (i.e., for every commit that changes that file) and give you a single point of the truth for values you can be sure the application is using.
  • Making all values explicit makes the application more robust, leaving out any doubts about what's going on or will happen.

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 .env.defaults file.

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.

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