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 ENV variables support for few configuration option - Closes #2225 #2285

Merged
merged 4 commits into from Aug 8, 2018

Conversation

nazarhussain
Copy link
Contributor

What was the problem?

Currently we support some subset of configuration options to be set through the ENV variables. https://github.com/LiskHQ/lisk/tree/1.1.0#command-line-options, which are not enough to run a docker image

How did I fix it?

Added further support for ENV variables.

Review checklist

@nazarhussain nazarhussain self-assigned this Aug 3, 2018
@nazarhussain nazarhussain changed the base branch from 1.1.0 to 1.2.0 August 3, 2018 15:15
README.md Outdated
| --peers<br> -p | LISK_PEERS | peers.list | Comma separated list of peers to connect in the format `192.168.99.100:5000,172.169.99.77:5000` |
| | LISK_API_PUBLIC | api.access.public | Enable or disable public access of http API. Must be set to true/false |
| | LISK_API_WHITELIST | api.access.whiteList | Comma separated list of IPs to enable API access. Format `192.168.99.100,172.169.99.77` |
| | LISK_FORGING_DELEGATES | forging.delegates | Comma separated list of delegates to load in the format `publicKey|encryptedPassphrase,publicKey2|encryptedPassphrase2` |
Copy link
Contributor

Choose a reason for hiding this comment

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

I think you have to escape that, it does show properly in the rendered README file.

@nazarhussain nazarhussain force-pushed the 2225-env-variables-support branch 3 times, most recently from 2882311 to 9f3db6c Compare August 6, 2018 15:26
@nazarhussain
Copy link
Contributor Author

@fchavant Had updated the README with correct formatting.

@fchavant
Copy link
Contributor

fchavant commented Aug 7, 2018

@nazarhussain it still does not show up as code but it's not mangled at least

@nazarhussain
Copy link
Contributor Author

There was a problem with nowrap content in table as well using | sign inside table, that causes a lot of trouble. That's the best presentable content I can generate. @fchavant

fileLogLevel: program.log || process.env.LISK_FILE_LOG_LEVEL || null,
consoleLogLevel: process.env.LISK_CONSOLE_LOG_LEVEL || null,
cacheEnabled: process.env.LISK_CACHE_ENABLED === 'true',
wsPort: +program.port || getenv(process.env.LISK_WS_PORT),
Copy link

Choose a reason for hiding this comment

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

It should be wsPort instead of port.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No this is port came from this line https://github.com/LiskHQ/lisk/blob/71ab88cdfaf06b71038e3a88644ebab5d2b1441d/helpers/config.js#L50

Didn't change command parameters for backward compatibility.

@diego-G diego-G requested a review from ManuGowda August 7, 2018 16:03
Copy link
Contributor

@ManuGowda ManuGowda left a comment

Choose a reason for hiding this comment

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

PR looks good, one small comment

address: program.address,
consoleLogLevel: program.log || process.env.LISK_CONSOLE_LOG_LEVEL,
db: { database: program.database },
wsPort: +program.port || getenv(process.env.LISK_WS_PORT),
Copy link
Contributor

Choose a reason for hiding this comment

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

LISK_WS_PORT and LISK_HTTP_PORT should be number here?

@MaciejBaj MaciejBaj deleted the 2225-env-variables-support branch August 8, 2018 09:54
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.

None yet

5 participants