-
Notifications
You must be signed in to change notification settings - Fork 12
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
Shift from file-based configuration to environment variables #165
Conversation
webpack.config.prod.babel.js
Outdated
NODE_ENV: JSON.stringify('production'), // clean up some react stuff | ||
BASENAME: JSON.stringify(basename), | ||
COUNTRY_PRODUCTION_CONFIG: JSON.stringify(process.env.COUNTRY_PRODUCTION_CONFIG || 'france') | ||
NODE_ENV: JSON.stringify(process.env.NODE_ENV), |
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.
✨ this is much better
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.
Looks good to me.
Looks like some missing env for circle-ci.
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 for the improvement and the great doc!
I'd like to understand more the rational behind the removal of the logger. I don't consider it blocking though.
@@ -23,7 +21,7 @@ function startServer(state) { | |||
|
|||
// Generic server errors (e.g. not caught by components) | |||
server.use((err, req, res, next) => { | |||
winston.error(req.method + ' ' + req.url, {error: err}) | |||
console.error(req.method + ' ' + req.url, {error: err}) |
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.
While dropping the logger altogether?
Seems like it add a purpose: #93
Do you think it's just to complex for what it brings?
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.
It is only used in this single occurrence.
#93 was about moving away from email-based error notifications that were never set up properly and that no one ever read. I was asked by the implementer if I had suggestions on how to log and recommended winston.
Adding a logger-dedicated module only makes sense if we actually use it. Currently, there is no monitoring in place, and the only usage of the error-logging mechanism was this line of code. It will be trivial to revert when / if someone puts the effort to set up logging properly. On the other hand, having it right now means adding a (useless) dependency and a lot of config complexity in order to pass a config object.
src/server/webpack-dev-server.js
Outdated
|
||
|
||
const WEBPACK_HOST = process.env.WEBPACK_HOST || 'localhost' | ||
const WEBPACK_PORT = parseInt(process.env.WEBPACK_PORT) | ||
const WEBPACK_HOST = process.env.WEBPACK_HOST || '0.0.0.0' |
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.
In this context, what is the difference between
localhost
and0.0.0.0
?
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.
In any context: localhost
is a hostname that is by default set (in a hosts
file) to resolve to the loopback address 127.0.0.1
.
0.0.0.0
, on the other hand, is a non-routable meta-address, which means it won't resolve on its own (i.e. you can't access 0.0.0.0 on a client), but is a standard way to express, in a server, “all IPv4 addresses on the local machine”.
Concretely, if server S has 192.168.0.1 as its local address on a local network and client C has 192.168.0.2:
- S can access S resources through 192.168.0.1 and 127.0.0.1.
- C can access S resources through 192.168.0.1.
However, for the server software to listen to 192.168.0.1, S has to explicitly state that it handles incoming packets from 192.168.0.1. In many cases, you'll have a frontal server on S, so you don't need to set this up explicitly: if Nginx listens to 192.168.0.1 and redirects to some local port P, then the S server only needs to bind to 127.0.0.1:P.
Here, we don't have any frontal server. So the only way S can listen both to 192.168.0.1 and 127.0.0.1 (and other addresses from other interfaces) is to use the meta-address 0.0.0.0.
In this specific context, this means we're allowing the webpack dev server to be available from all clients, not only the ones on the same computer as the server, so I can do remote debugging from another device on the LAN for example.
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 a lot for the thorough explanation!
@@ -20,48 +20,56 @@ npm install --production | |||
|
|||
## Configure your instance | |||
|
|||
You will need to tell the Legislation Explorer server where your OpenFisca API instance can be reached, as well as where your repository resides for contributors to be directed to. This is done with a configuration file. | |||
You will need to tell the Legislation Explorer server where your OpenFisca API instance can be reached, as well as where your repository resides for contributors to be directed to. This is all done through environment variables, allowing you to use the same code for any instance by [injecting these elements at runtime](https://12factor.net/config). |
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.
That's a really interesting resource, thanks for sharing!
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.
Thank you @MattiSG for this great PR ! I think it will make life easier for many people.
There are a few changes that I would require to accept the PR.
I tried to use the PATHNAME
in the .env
, and it did not work (the links in the legexp did not show the basename)
I tried to change the WEBPACK_HOST
on the .env
and it kept serving the asset on the default WEBPACK_HOST
.
README.md
Outdated
|
||
This web app supports [Matomo](https://matomo.org) (ex-Piwik) analytics out of the box. To set it up, use the following environment variables: | ||
|
||
- `MATOMO_URL`: he base URL at which the Matomo instance can be contacted. For example: `MATOMO_URL=https://stats.data.gouv.fr MATOMO_SITE_ID=4 npm start`. Only taken into account if used in conjunction with `MATOMO_SITE_ID` and if `MATOMO_CONFIG` is not defined. |
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.
he base URL
--> the base URL
|
||
### Server and public URL | ||
|
||
- `PATHNAME` defines the path at which the Legislation Explorer is served. Defaults to `/`. For example: `PATHNAME=/legislation` to serve on `https://fr.openfisca.org/legislation`. |
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.
Why PATHNAME
? Most libraries seem to use BASENAME
to desctibe the path the app is hosted on (e.g. piwik-react-router
, react-router
).
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.
As stated in the commit message for 408c898:
Use Node API name for URL pathname
The Node API uses “pathname”. In the URI spec, this component is called the “path component”.
This authoritative answer is reinforced by the fact that “basename” is ambiguous with the result of a basename
operation on a path (which drops its last part).
@@ -1,8 +1,9 @@ | |||
import config from '../config' | |||
|
|||
import acceptLanguage from 'accept-language' |
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.
Why the change ?
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.
I tend to document the rationale for changes in commit messages 🙂
For b97220b:
Unify import statements order
Group by distance: first group is for app imports, second group for dependencies, third group for standard library
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.
Just for information, Python recommends the opposite order, and that's also the default choice in the ESlint plugin that deals with import order.
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.
Yes, and that's also the one I usually use. It makes much more sense as it is safer for overrides, and reflects the priority of non-namespaced languages (i.e. CSS). I must have been quite confused when I changed the order to this 😕😞
I still feel that splitting in groups is an improvement, and this PR is lingering for too long.
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 a lot for the heads up though!!
config.apiBaseUrl = process.env.API_URL || 'http://0.0.0.0:5000' | ||
config.changelogUrl = process.env.CHANGELOG_URL | ||
|
||
config.pathname = process.env.PATHNAME || '/' |
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.
The default here is '/'
and is ''
in render.jsx
. is that voluntary ? If so, what is the benefit ?
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.
This is not on purpose, but not changing this value was the easiest. I believe the usage of that value in render.jsx
should be dropped altogether, but that would have made the PR even bigger.
package.json
Outdated
"dev:app-server": "NODE_ENV=development PORT=2030 node index.js", | ||
"dev:webpack-dev-server": "WEBPACK_PORT=2031 babel-node src/server/webpack-dev-server.js", | ||
"dev:app-server": "NODE_ENV=development node --require dotenv/config index.js", | ||
"dev:webpack-dev-server": "babel-node src/server/webpack-dev-server.js", |
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.
dotenv/config is not imported here.
However, process.env is called by /src/server/webpack-dev-server.js
and /webpack.config.dev.js
.
webpack.config.dev.js
Outdated
@@ -11,8 +11,8 @@ import writeAssets from './src/server/write-assets' | |||
|
|||
const assetsPath = path.resolve(__dirname, 'public') | |||
|
|||
const WEBPACK_HOST = process.env.HOST || 'localhost' | |||
const WEBPACK_PORT = parseInt(process.env.PORT) + 1 || 2031 | |||
const WEBPACK_HOST = process.env.WEBPACK_HOST || '0.0.0.0' |
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.
In my tests, the envar WEBPACK_HOST
in .env
was not taken into account.
It worked with the following code :
const my_env = require('dotenv').config().parsed
const WEBPACK_HOST = my_env.WEBPACK_HOST || '0.0.0.0'
const WEBPACK_PORT = parseInt(my_env.WEBPACK_PORT) || 2031
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 for this test!
I have not been able to understand the use case for changing WEBPACK_HOST
and have thus probably been too lax on testing.
Can I ask how you tried to set this value? I feel it would probably be a consequence of #165 (comment).
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.
I ended up removing the ability to set up WEBPACK_HOST
and WEBPACK_PORT
independently from HOST
and PORT
, it adds complexity and I can't see a proper use case for them. Please let me know if you had a use for it 😉
src/server/webpack-dev-server.js
Outdated
|
||
|
||
const WEBPACK_HOST = process.env.WEBPACK_HOST || 'localhost' | ||
const WEBPACK_PORT = parseInt(process.env.WEBPACK_PORT) | ||
const WEBPACK_HOST = process.env.WEBPACK_HOST || '0.0.0.0' |
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.
In my tests, the envar WEBPACK_HOST
in .env
was not taken into account.
It worked with the following code :
const my_env = require('dotenv').config().parsed
const WEBPACK_HOST = my_env.WEBPACK_HOST || '0.0.0.0'
const WEBPACK_PORT = parseInt(my_env.WEBPACK_PORT) || 2031
|
||
> If it gets lengthy or you want to persist these values, you can also use a `.env` file. | ||
> Such a file is a plaintext file with name `.env` stored in the root directory of your legislation explorer instance (i.e. next to the `package.json` file). List all of your environment variables in the `$NAME=$VALUE` syntax, one per line. If you have trouble writing this file, read the [parsing rules](https://github.com/motdotla/dotenv#rules). | ||
> An example file is provided as `.env.example`, setting default values. You can copy it, but changing it won't have any impact: only a file named `.env` will be taken into account. You should not commit this file: it depends on each instance. |
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.
Why not have a commited .env that has all the default environment values ? That way there is one place where they are all explicited.
such as :
API_URL=https://fr.openfisca.org/api/v19
CHANGELOG_URL=https://github.com/openfisca/country-template/blob/master/CHANGELOG.md
UI_STRINGS={"en":{"countryName":"the development environment"},"fr":{"countryName":"l’environnement de développement"}}
HOST='0.0.0.0'
PORT=2030
PATHNAME='/'
WEBPACK_HOST='0.0.0.0'
WEBPACK_PORT=2031
MATOMO_URL=https://stats.data.gouv.fr
MATOMO_SITE_ID=4
NODE_ENV=development
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.
I hoped this would have been clear enough with the reference to https://12factor.net/config and this:
You should not commit this file: it depends on each instance.
If we use the .env
file to store default values, then all clones will have changes that will conflict whenever there is a config (default value, renaming…) change.
How could I improve the documentation here?
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.
I'm sorry, I wasn't very precise in my comment.
I think the documentation is great and very clear. My request would be that the .env.example
(and not the .env
as I hastily wrote) file contain the available environment variable with their default value.
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.
From previous experience as a user in similar situations, I liked having a .env.example
containing all environment variables I could set, (possibly commented for the ones that were a little specific/uncommon).
It made my life easier while configuring. My 2 currency-cents
;)
@@ -4,13 +4,12 @@ | |||
"description": "OpenFisca - Legislation Explorer", | |||
"scripts": { | |||
"build": "webpack --config ./webpack.config.prod.babel.js", |
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.
dotenv/config is not imported here.
However, process.env is called by webpack.
Is that intentional ?
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.
No, you're right, it is important that PATHNAME
can be taken from the .env
file. Good catch, thanks!
Should close #167. |
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.
About the WEBPACK_HOST
and WEBPACK_PORT
envars :
(Github wound't let me comment on an outdated thread)
The only use I can see of the WEBPACK_PORT
I can figure is if your port PORT + 1
is already used.
Seem not that likely :)
|
||
> If it gets lengthy or you want to persist these values, you can also use a `.env` file. | ||
> Such a file is a plaintext file with name `.env` stored in the root directory of your legislation explorer instance (i.e. next to the `package.json` file). List all of your environment variables in the `$NAME=$VALUE` syntax, one per line. If you have trouble writing this file, read the [parsing rules](https://github.com/motdotla/dotenv#rules). | ||
> An example file is provided as `.env.example`, setting default values. You can copy it, but changing it won't have any impact: only a file named `.env` will be taken into account. You should not commit this file: it depends on each instance. |
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.
I'm sorry, I wasn't very precise in my comment.
I think the documentation is great and very clear. My request would be that the .env.example
(and not the .env
as I hastily wrote) file contain the available environment variable with their default value.
Group by distance: first group is for app imports, second group for dependencies, third group for standard library
Remove winstonConfig config element
Remove redundant “config” suffix Piwik renamed to Matomo in 2018
Define default values and load actual ones in a single place
Document the options
Document default API URL value
This property was not used in any other place and was only used for this inference. Decrease vendor lock-in.
Does not variate for each instance, depends on the legislation explorer itself
Does not variate for each instance, was already not used anymore
The current loading mechanism loads the config file in the frontend too, which fails to compile dotenv into a frontend bundle.
Remove unnecessary complexity. When in dev mode, it is expected that user has full port definition freedom and can accomodate for finding two subsequent available ports.
Define config in a single place
Would trigger a failed build on production on merge. Will have to be updated to support the latest config system as documented in the PR.
Alright, thanks everybody for the thoughtful reviews. We're finally ready to merge! I just rebased, checked the diff, and will merge as soon as CI passes. @openfisca/france-admin beware! As I did not get any reply in the last 18 days regarding deployment updates, I decided to take the safest direction and deactivated autodeploy (see 5da4e75). I will let you open another PR if you want to use CI for CD again. @openfisca/aotearoa you should be ready to use with Heroku! Let me know if you need more help than what was explained earlier in #165 (comment) 🙂 |
We're stuck because of #169. |
Fastest way out of this for @openfisca/aotearoa who would like to switch to Heroku is to merge this branch into the master of your fork. Best solution is to open a separate PR to switch integration tests to the country template. Necessary feature is to support the new slash syntax. |
Main objective and features
This changeset shifts the configuration mechanism from file-based to environment variables.
This allows to create instances for new tax and benefit systems (countries) without committing a full config file to the shared repository, leaving it to each instance to define their configuration at runtime.
This also allows to run the Legislation Explorer on application-level cloud providers (PaaS) such as Heroku, which do not allow modification of the filesystem between checkout and execution.
In order to facilitate usage, support is also provided for a
.env
file that can list all environment variables, for these cases where the operator has control over the filesystem.This changeset also:
Clients impact
All automatic deployments will fail when this PR is merged. Since the configuration mechanism now relies on variables being injected, you will need to update your setup to define these variables.
For convenience, here are the previously defined configuration files as
.env
files, as well as suggested migration paths:France
Define a
.env
file at the root of the current clone with the following data:Tunisia
Define a
.env
file at the root of the current clone with the following data:Aotearoa
Connected to Support loading arbitrary configuration files #147
master
.