cleanup, local development fixes and start best practices #453
Conversation
|
we can split this PR into multiple smaller ones, but keeping all the things in my head (what js file can be deleted etc,) was too much for me. |
- removed some unused yarn deps (via npm_check) - added missing deps (grunt did not complain on concat, but files where missing) - removed Procfile (was prob. a leftover of a Heroku trail) - removed .dockerignore, since it is doing nothing for the build - fixed all of the grunt commands (that are used) - removed grunt node-inspector -> unused - added npm_check to find unused yarn deps and updates - Dockerfile was updated to include some of the best practices for node-containers - Docker compose was added for easy development with the faf-stack - Readme updated to include local docker env setup - Express port is now defaulting to 3000 - OAUTH_PUBLIC_URL was intruduced for docker environments (container -> container vs brower -> container) urls - unused js files removed (no reference in the code, or empty files)
fcaps
force-pushed
the
feature/update_local_development
branch
from
8e6c0c1 to
7a4ed4b
Compare
.dockerignore
Outdated
There was a problem hiding this comment.
ignoring node_modules did nothing for the build nor the local env
| TOKEN_LIFESPAN=43200 | ||
| CLAN_INVITES_LIFESPAN_DAYS=30 | ||
| EXTRACTOR_INTERVAL=5 | ||
| PLAYER_COUNT_INTERVAL=15 |
There was a problem hiding this comment.
we should also update the faf-stack to include some of the fixes from here.
| RUN yarn --prod | ||
|
|
||
| CMD PORT=3000 npm start | ||
| CMD ["dumb-init", "node", "express.js"] |
There was a problem hiding this comment.
applied some of the best practices here.
more info https://snyk.io/blog/10-best-practices-to-containerize-nodejs-web-applications-with-docker/
| ENV NODE_ENV development | ||
| WORKDIR /code | ||
|
|
||
| CMD ["dumb-init", "./node_modules/.bin/grunt", "concurrent"] |
There was a problem hiding this comment.
multistage was actually not beneficial, so i decided to create a new dockerfile for local-dev
Procfile
Outdated
There was a problem hiding this comment.
we are not using heroku somehow, right?
| @@ -17,9 +17,10 @@ app.locals.clanInvitations = {}; | |||
| require('dotenv').config(); | |||
| //Define environment variables with default values | |||
| process.env.NODE_ENV = process.env.NODE_ENV || 'production'; | |||
| process.env.PORT = process.env.PORT || '4000'; | |||
| process.env.PORT = process.env.PORT || '3000'; | |||
There was a problem hiding this comment.
in every .env file i could found we setting the PORT env, just because we have this fallback here.
| @@ -227,7 +228,7 @@ app.get('/login', passport.authenticate('faforever')); | |||
| passport.use('faforever', new OidcStrategy({ | |||
| issuer: process.env.OAUTH_URL + '/', | |||
| tokenURL: process.env.OAUTH_URL + '/oauth2/token', | |||
| authorizationURL: process.env.OAUTH_URL + '/oauth2/auth', | |||
| authorizationURL: process.env.OAUTH_PUBLIC_URL + '/oauth2/auth', | |||
There was a problem hiding this comment.
a browser can not resolve the docker name aka "http://faf-ory-hydra:4444", so this is needed for docker environments, this needs also be fixed in the faf-stack
| @@ -0,0 +1,33 @@ | |||
| # node env is set statically to "development" in the dockerfile, you could override it here. | |||
There was a problem hiding this comment.
What was the thinking with having both a .env.template and .env.fafstack? It seems a little redundant
There was a problem hiding this comment.
env.example is for local development (without docker and a faf-stack)
grunt/concat.js
Outdated
There was a problem hiding this comment.
none of the removed files where used, sometimes they where just empty after #410
| @@ -1,6 +1,6 @@ | |||
| module.exports = { | |||
| dev: { | |||
| tasks: ['nodemon:debug', 'concat:js', 'uglify:dev', 'watch'], | |||
| tasks: ['nodemon', 'concat', 'watch'], | |||
There was a problem hiding this comment.
uglify is not needed on dev, and calling a taks without :, just executes all tasks
Cleanup, local development improvements and applying some best practices