-
Notifications
You must be signed in to change notification settings - Fork 38
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
cleanup, local development fixes and start best practices #453
Changes from 3 commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. env.example is for local development (without docker and a faf-stack) |
||
#NODE_ENV=production | ||
|
||
# you should not need to change the port (used by express) inside the container, just here for completion | ||
#PORT=3000 | ||
|
||
# configs to change the ports in docker compose for the container | ||
# only needed if your host already has 8020 binded to another service | ||
# beware, changing the host port (8020) also needs an update in the hydra client for valid callback-urls | ||
#WEBSITE_EXPOSED_PORT=8020 | ||
#WEBSITE_CONTAINER_PORT=3000 | ||
|
||
HOST=http://localhost:8020 | ||
API_URL=http://faf-java-api:8010 | ||
OAUTH_URL=http://faf-ory-hydra:4444 | ||
|
||
# on a local environment with docker, the internal docker-service-domain (faf-ory-hydra:4444) is not reachable for a browser | ||
# you can omit this env and it will fallback to OAUTH_URL if you know what you are doing. | ||
OAUTH_PUBLIC_URL=http://localhost:4444 | ||
|
||
# unsing the "production" wordpress because the faf-local-stack is just an empty instance without any news etc. | ||
WP_URL=https://direct.faforever.com | ||
|
||
LOBBY_API_URL=http://faf-python-server:4000 | ||
|
||
OAUTH_CLIENT_ID=faf-website | ||
OAUTH_CLIENT_SECRET=banana | ||
SESSION_SECRET_KEY=banana | ||
RECAPTCHA_SITE_KEY=6LeIxAcTAAAAAJcZVRqyHh71UMIEGNQ_MXjiZKhI | ||
TOKEN_LIFESPAN=43200 | ||
CLAN_INVITES_LIFESPAN_DAYS=30 | ||
EXTRACTOR_INTERVAL=5 | ||
PLAYER_COUNT_INTERVAL=15 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we should also update the faf-stack to include some of the fixes from here. |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,17 +1,2 @@ | ||
# Auto detect text files and perform LF normalization | ||
* text=auto | ||
|
||
# Custom for Visual Studio | ||
*.cs diff=csharp | ||
|
||
# Standard to msysgit | ||
*.doc diff=astextplain | ||
*.DOC diff=astextplain | ||
*.docx diff=astextplain | ||
*.DOCX diff=astextplain | ||
*.dot diff=astextplain | ||
*.DOT diff=astextplain | ||
*.pdf diff=astextplain | ||
*.PDF diff=astextplain | ||
*.rtf diff=astextplain | ||
*.RTF diff=astextplain |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,20 @@ | ||
name: Website static checks | ||
|
||
on: [pull_request] | ||
|
||
concurrency: | ||
group: ${{ github.workflow }}-${{ github.ref }} | ||
cancel-in-progress: true | ||
|
||
jobs: | ||
checks: | ||
runs-on: ubuntu-latest | ||
steps: | ||
- uses: actions/checkout@v4 | ||
- uses: actions/setup-node@v3 | ||
with: | ||
node-version: '20' | ||
cache: 'yarn' | ||
- run: yarn install | ||
# --force should be removed if all the issues are fixed | ||
- run: ./node_modules/.bin/grunt jshint --force |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -46,4 +46,6 @@ public/styles/css/* | |
.env | ||
|
||
#Ignore link.json | ||
link.json | ||
link.json | ||
|
||
public/js/*.js |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,16 @@ | ||
depcheck: | ||
ignoreMatches: [ | ||
"npm-check", # used manually to check for updates | ||
"pug", # used in express as the template engine | ||
"awesomplete", # used statically in grunt/concat.js | ||
|
||
# grunt plugins used in "./grunt" | ||
"grunt-sass", | ||
"grunt-postcss", | ||
"grunt-nodemon", | ||
"grunt-contrib-watch", | ||
"grunt-contrib-uglify-es", | ||
"grunt-contrib-jshint", | ||
"grunt-contrib-concat", | ||
"grunt-concurrent" | ||
] |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,27 +1,22 @@ | ||
# Use an ubuntu-image for building assets for use in a runtime image... | ||
FROM node:lts as builder | ||
|
||
RUN mkdir code | ||
|
||
# Add files to /code folder | ||
ADD . /code | ||
FROM node:20.9-bookworm as builder | ||
RUN apt-get update && apt-get install -y --no-install-recommends dumb-init | ||
ENV NODE_ENV development | ||
|
||
COPY . /code | ||
WORKDIR /code | ||
|
||
RUN yarn install | ||
|
||
RUN yarn install --production=false --frozen-lockfile | ||
RUN ./node_modules/.bin/grunt prod | ||
RUN yarn install --production=true --ignore-optional --frozen-lockfile | ||
|
||
# Slimmer runtime image without python/make/gcc etc. | ||
FROM node:lts-alpine as runtime | ||
FROM node:20.9.0-bookworm-slim as runtime | ||
ENV NODE_ENV production | ||
|
||
COPY --from=builder /code /code | ||
COPY --from=builder /usr/bin/dumb-init /usr/bin/dumb-init | ||
COPY --from=builder --chown=node:node /code /code | ||
|
||
WORKDIR /code | ||
USER node | ||
|
||
# Only install runtime dependencies for the runtime image | ||
RUN yarn --prod | ||
|
||
CMD PORT=3000 npm start | ||
CMD ["dumb-init", "node", "express.js"] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. applied some of the best practices here. |
||
|
||
EXPOSE 3000 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,6 @@ | ||
FROM node:20.9-bookworm | ||
RUN apt-get update && apt-get install -y --no-install-recommends dumb-init | ||
ENV NODE_ENV development | ||
WORKDIR /code | ||
|
||
CMD ["dumb-init", "./node_modules/.bin/grunt", "concurrent"] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. multistage was actually not beneficial, so i decided to create a new dockerfile for local-dev |
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. we are not using heroku somehow, right? |
This file was deleted.
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,19 @@ | ||
version: "3.5" | ||
|
||
services: | ||
website: | ||
env_file: .env | ||
ports: | ||
- ${WEBSITE_EXPOSED_PORT-8020}:${WEBSITE_CONTAINER_PORT-3000} | ||
volumes: | ||
- .:/code | ||
build: | ||
dockerfile: Dockerfile-dev | ||
context: . | ||
networks: | ||
- faf-stack | ||
|
||
networks: | ||
faf-stack: | ||
name: faf-stack_faf | ||
fcaps marked this conversation as resolved.
Show resolved
Hide resolved
|
||
external: true |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. in every .env file i could found we setting the |
||
process.env.CALLBACK = process.env.CALLBACK || 'callback'; | ||
process.env.OAUTH_URL = process.env.OAUTH_URL || 'https://hydra.faforever.com'; | ||
process.env.OAUTH_PUBLIC_URL = process.env.OAUTH_PUBLIC_URL || process.env.OAUTH_URL; | ||
process.env.API_URL = process.env.API_URL || 'https://api.faforever.com'; | ||
process.env.OAUTH_CLIENT_ID = process.env.OAUTH_CLIENT_ID || '12345'; | ||
process.env.OAUTH_CLIENT_SECRET = process.env.OAUTH_CLIENT_SECRET || '12345'; | ||
|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
userInfoURL: process.env.OAUTH_URL + '/userinfo?schema=openid', | ||
clientID: process.env.OAUTH_CLIENT_ID, | ||
clientSecret: process.env.OAUTH_CLIENT_SECRET, | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. none of the removed files where used, sometimes they where just empty after #410 |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,37 +1,17 @@ | ||
module.exports = { | ||
js: { | ||
nonull: true, | ||
files: { | ||
'public/js/bottom.min.js': [ | ||
'public/js/app/headroom.min.js', | ||
'public/js/app/navigation.js' | ||
], | ||
'public/js/leaderboards.min.js': [ | ||
'node_modules/awesomplete/awesomplete.js', | ||
'node_modules/moment/min/moment-with-locales.min.js', | ||
'public/js/app/leaderboards.js' | ||
], | ||
'public/js/leagues.min.js': [ | ||
'node_modules/awesomplete/awesomplete.js', | ||
'node_modules/moment/min/moment-with-locales.min.js', | ||
'public/js/app/leagues.js' | ||
], | ||
'public/js/account.min.js': [ | ||
'node_modules/bootstrap-validator/dist/validator.js' | ||
], | ||
'public/js/blog.min.js': [ | ||
'public/js/app/blog.js' | ||
], | ||
'public/js/newshub.min.js': [ | ||
'public/js/app/newshub.js' | ||
], | ||
'public/js/report.min.js': [ | ||
'node_modules/awesomplete/awesomplete.js', | ||
'public/js/app/report.js' | ||
], | ||
'public/js/browse_clans.min.js': [ | ||
'node_modules/awesomplete/awesomplete.js', | ||
'public/js/app/browse_clans.js' | ||
] | ||
} | ||
} | ||
}; |
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -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. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. uglify is not needed on dev, and calling a taks without |
||
options: { | ||
logConcurrentOutput: true | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -1,13 +1,26 @@ | ||
module.exports = { | ||
options: { | ||
reporter: require('jshint-stylish'), | ||
force: true | ||
}, | ||
all: [ | ||
'routes/**/*.js', | ||
'models/**/*.js' | ||
], | ||
server: [ | ||
'./express.js' | ||
] | ||
}; | ||
browser_files: { | ||
options: { | ||
esversion: 8, | ||
asi: true | ||
}, | ||
src: [ | ||
'public/js/app/**/*js', | ||
] | ||
}, | ||
node_files: { | ||
options: { | ||
node: true, | ||
esversion: 11, | ||
asi: true | ||
}, | ||
src: [ | ||
'./express.js', | ||
'scripts/**/*js', | ||
'routes/**/*js', | ||
] | ||
} | ||
}; |
This file was deleted.
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.
ignoring node_modules did nothing for the build nor the local env