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

Refactor docker image and CI pipeline #1624

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
27 commits
Select commit Hold shift + click to select a range
d267051
chore: node 12.16.3 -> 16.15.0
keeler May 29, 2022
da19113
chore: npm audit fix
keeler May 29, 2022
9e00685
chore: Add .nvmrc
keeler May 29, 2022
d636bfb
feat: Add Makefile
keeler May 29, 2022
bf5ca74
chore: Github workflow use npm ci
keeler May 29, 2022
5fbc461
wip: Attempt at better docker build caching
keeler May 29, 2022
b62b9d3
docs: Makefile help target
keeler May 29, 2022
92327e9
feat: Don't need git in docker image
keeler May 29, 2022
cfeb1bf
refactor: CI pipeline use docker image
keeler May 30, 2022
d37a9f3
fix: Hide news if no message of the day
keeler May 30, 2022
528a1c0
fix: Broke editorconfig
keeler May 30, 2022
2bddf77
fix: Docker env from arg invalidates cache
keeler May 30, 2022
d7f1a04
fix: Shallow clone doesn't include tags
keeler May 31, 2022
2700269
fix: Use fetch-depth 0 to get tag history
keeler May 31, 2022
5e4c63c
chore: Undo npm audit fix for now
keeler Jun 1, 2022
efff286
feedback: Use lts-alpine base and 16.15.1
keeler Jun 2, 2022
c39a7ab
chore: Undo changes moved to PR 1623
keeler Jun 2, 2022
1ecd506
chore: Docker ignore Vim swap files
keeler Jun 3, 2022
36c7690
refactor: Move npm ci above copying source
keeler Jun 3, 2022
bba12ac
feat: Makefile can specify --no-cache in docker build
keeler Jun 3, 2022
d1a1f7e
perf: Update cards/sets before copying sources
keeler Jun 4, 2022
de384b6
Merge branch 'master' into refactor-docker-and-ci-pipeline
keeler Jun 5, 2022
14092a1
fix: Save space by combine chown data/ command
keeler Jun 19, 2022
90788bf
chore: dr4ftuser/dr4ftgroup -> dr4ft
keeler Jun 19, 2022
145f284
fix: No need for dr4ft user
keeler Jun 19, 2022
548a9e9
Merge branch 'master' into refactor-docker-and-ci-pipeline
keeler Jun 19, 2022
57f5775
chore: Spacing in Dockerfile & Makefile
keeler Jun 19, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
13 changes: 10 additions & 3 deletions .dockerignore
Original file line number Diff line number Diff line change
@@ -1,4 +1,11 @@
data/*
!data/scores.json
**/.git
Copy link
Contributor Author

@keeler keeler Jun 8, 2022

Choose a reason for hiding this comment

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

Previously, the .git folder was added to the docker image for the sake of building in a version number, namely a commit hash to display in the footer. Now that version number is built in as an environment variable. The other ignores here leave out files that are irrelevant to the docker image build. Any time these files changed required the entire image to be rebuilt.

**/.github
README.md
data
built
doc
node_modules
frontend/lib
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed frontend/lib and data/scores.json because they don't appear to exist anymore.

Added ignores for .git and .github because they really shouldn't be built into the docker image anyway. In particular, having .git built into the image means that making a commit required rebuilding the image from the COPY . . step that existed previously, even if that commit changed no code (e.g. a README change) which is unnecessary.

Makefile

# Vim swap files
**/.*.sw[po]
4 changes: 4 additions & 0 deletions .editorconfig
Original file line number Diff line number Diff line change
Expand Up @@ -9,3 +9,7 @@ trim_trailing_whitespace = true
[*.{js,jsx,css,js.default}]
indent_size = 2
indent_style = space

[Makefile]
# Makefiles require tabs for indentation
indent_style = tab
70 changes: 12 additions & 58 deletions .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,55 +13,28 @@ on:
- '**.md'

jobs:
test:
name: Run tests

strategy:
fail-fast: false
matrix:
node_version:
- 14
- 16
verify:
name: Verify

runs-on: ubuntu-latest

steps:
- name: Checkout
uses: actions/checkout@v3

- name: Setup
uses: actions/setup-node@v2.5.1
with:
node-version: ${{matrix.node_version}}
fetch-depth: 0
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Setting fetch-depth: 0 makes it so git describe --tags works in the make docker step.


# See https://github.com/npm/cli/issues/2610
- name: Workaround for Node 14
if: matrix.node_version == '14'
run: sed -i 's/git+ssh/git+https/g' package-lock.json
- name: Build
run: make docker DOCKER_CACHE_OFF=1

- name: Install
run: npm ci
- name: Lint
run: make docker-lint

- name: Test
run: npm test


docker:
name: Test docker image

runs-on: ubuntu-latest

continue-on-error: true

steps:
- name: Checkout
uses: actions/checkout@v3

- name: Build
run: docker build --tag dr4ft-app .
run: make docker-test

- name: Run
run: docker run -dp 1337:1337 dr4ft-app
- name: Run docker image
run: make docker-run

- name: Show info
run: |
Expand All @@ -70,24 +43,5 @@ jobs:
docker ps -a
echo
docker images


lint:
name: Run ESLint

runs-on: ubuntu-latest

steps:
- name: Checkout
uses: actions/checkout@v3

- name: Setup
uses: actions/setup-node@v2.5.1
with:
node-version: 16

- name: Install
run: npm ci --ignore-scripts

- name: Run ESLint
run: npm run lint
echo
make docker-logs FOLLOW=
44 changes: 31 additions & 13 deletions Dockerfile
Original file line number Diff line number Diff line change
@@ -1,24 +1,42 @@
FROM node:lts-alpine
ENV NPM_CONFIG_LOGLEVEL warn

# Install "git"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Installing git was apparently done only to allow running git describe --tags in config/version.js. This is slow and bloats the size of the image, and is IMHO unnecessary just to get a commit hash in the footer.

RUN apk update \
&& apk add alpine-sdk
ARG VERSION_INFO=noVersion

ENV NPM_CONFIG_LOGLEVEL warn
ENV PORT=1337

# Set working dir as /app
WORKDIR /app

# Add sources to /app
COPY . .
Copy link
Contributor Author

@keeler keeler Jun 19, 2022

Choose a reason for hiding this comment

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

Running COPY . . is not a good idea IMHO. When building a docker image instructions like RUN, COPY, etc. will be cached as a 'layer' in the image. When running COPY . . any file changing invalidates the cache for this layer, meaning this and everything below it needs to re-run.

RUN adduser -S dr4ftuser
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Node images already include a non-root user called node, so adding another one is redundant. https://github.com/nodejs/docker-node/blob/main/docs/BestPractices.md#non-root-user

RUN chown dr4ftuser -R .
Copy link
Contributor Author

@keeler keeler Jun 19, 2022

Choose a reason for hiding this comment

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

Running chown in a separate RUN instruction like this has a sneaky behavior where it actually copies files in layers above it, increasing the size of the image. The COPY instruction includes a --chown flag that does not trigger this behavior, hence why it's used all over the place here.

USER dr4ftuser

# Install the dependencies
RUN npm ci
Copy link
Contributor Author

@keeler keeler Jun 19, 2022

Choose a reason for hiding this comment

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

Running npm ci at the end in combination with the COPY . . instruction above it meant that any time any source file changed, the dependencies would need to get reinstalled and the card info would be re-downloaded again regardless of whether it was necessary. This happens because a change in any source file (or really any file) invalidates the layer cache of the COPY . . layer. This slowed down repeated iterations of docker builds significantly.

Now, the dependencies and card info are added to the image before most of this code. You can test the difference this makes by running make docker to build the image, changing a file in frontend/ in some innocuous way (like adding a comment), then running make docker again. You'll see that the build picks up from the COPY frontend/ layer, skipping all the layers above it that don't need to change, including the dependency installation and card info download.

COPY --chown=node package.json .
COPY --chown=node package-lock.json .
RUN npm ci --ignore-scripts

# Update card database
COPY --chown=node backend/core backend/core
COPY --chown=node config/ config/
COPY --chown=node scripts/ scripts/
RUN npm run download_allsets \
&& npm run download_booster_rules \
&& chown node -R data/
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Including this chown in the same RUN instruction does not increase the size of the image like it would if it were in its own separate RUN instruction.


# Add sources to /app
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I tried to order these in such a way that more frequently-changing files would be copied in later layers so the layer cache invalidation would only apply to lower layers. Open to suggestions.

Also, I think the app.json file could be excluded here.

COPY --chown=node LICENSE .
COPY --chown=node .gitignore .
COPY --chown=node .babelrc .
COPY --chown=node .eslintrc.js .
COPY --chown=node .mocharc.yaml .
COPY --chown=node webpack.prod.js .
COPY --chown=node webpack.common.js .
COPY --chown=node webpack.dev.js .
COPY --chown=node app.json .
COPY --chown=node app.js .
COPY --chown=node backend/ backend/
COPY --chown=node frontend/ frontend/

# Publish the port 1337
EXPOSE 1337
RUN npm run webpack

ENV VERSION_INFO=$VERSION_INFO
# Run the server
ENTRYPOINT [ "npm", "start" ]
72 changes: 72 additions & 0 deletions Makefile
Original file line number Diff line number Diff line change
@@ -0,0 +1,72 @@
VERSION_INFO ?= $$(git describe --tags)
IMAGE ?= dr4ft-app
CONTAINER ?= $(IMAGE)-container
PORT ?= 1337


# Show makefile help by default
.DEFAULT_GOAL = help


CACHE_OFF=0
ifeq ($(CACHE_OFF), 1)
NO_CACHE_FLAG = --no-cache
else
NO_CACHE_FLAG =
endif
.PHONY: docker
docker: ## Build docker image
@echo "Building with version info $(VERSION_INFO)"
docker build $(NO_CACHE_FLAG) \
--build-arg VERSION_INFO=$(VERSION_INFO) \
-t $(IMAGE) .


.PHONY: docker-run
docker-run: docker-stop docker ## Run app in docker container
docker run -d \
--name $(CONTAINER) \
--env "PORT=$(PORT)" \
-p $(PORT):$(PORT) \
$(IMAGE)
@echo "##########################################"
@echo "Dr4ft now running at http://localhost:$(PORT)"
@echo "##########################################"


.PHONY: docker-stop
docker-stop: ## Stop running docker container
docker stop $(CONTAINER) > /dev/null 2>&1 || true
docker container rm $(CONTAINER) > /dev/null 2>&1 || true


.PHONY: docker-logs
FOLLOW := -f
docker-logs: ## Show logs from running docker container
docker logs $(FOLLOW) $(CONTAINER)


.PHONY: docker-test
docker-test: docker ## Run tests in docker container
docker run --rm \
--entrypoint npm \
$(IMAGE) run test:js


.PHONY: docker-lint
docker-lint: docker ## Lint code in docker container
docker run --rm \
--entrypoint npm \
$(IMAGE) run lint


.PHONY: docker-clean
docker-clean: docker-stop ## Remove any built docker images
docker rmi -f $$(docker images -q $(IMAGE)) > /dev/null 2>&1 || true


# Output help string for each target.
# With thanks to: https://marmelab.com/blog/2016/02/29/auto-documented-makefile.html
.PHONY: help
help: ## Show this help
@awk 'BEGIN {FS = ":.*?## "} /^[a-zA-Z_-]+:.*?## / {printf "\033[36m%-30s\033[0m %s\n", $$1, $$2}' $(MAKEFILE_LIST)
2 changes: 1 addition & 1 deletion app.js
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const bodyParser = require("body-parser");
const {spawn} = require("child_process");

const {app: config, version} = require("./config");
const logger = require("./backend/logger");
const logger = require("./backend/core/logger");
const router = require("./backend/router");
const apiRouter = require("./backend/api/");
require("./backend/data-watch");
Expand Down
9 changes: 4 additions & 5 deletions backend/api/sets.js
Original file line number Diff line number Diff line change
@@ -1,12 +1,11 @@
const fs = require("fs");
const express = require("express");
const setsRouter = express.Router();
const { getSets, saveSetAndCards } = require("../data");
const doSet = require("../import/doSet");
const logger = require("../logger");
const parser = require("../import/xml/parser");
const { getDataDir, getSets, saveSetAndCards } = require("../core/data");
const doSet = require("../core/import/doSet");
const logger = require("../core/logger");
const parser = require("../core/import/xml/parser");
const path = require("path");
const {getDataDir} = require("../data");

const customDataDir = path.join(getDataDir(), "custom");
if (!fs.existsSync(customDataDir)) {
Expand Down
4 changes: 2 additions & 2 deletions backend/boosterGenerator.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
const {getCardByUuid, getSet, getBoosterRules} = require("./data");
const logger = require("./logger");
const {getCardByUuid, getSet, getBoosterRules} = require("./core/data");
const logger = require("./core/logger");
const weighted = require("weighted");
const {sample, sampleSize, random, concat} = require("lodash");

Expand Down
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
File renamed without changes.
2 changes: 1 addition & 1 deletion backend/logger.js → backend/core/logger.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const { createLogger, format, transports } = require("winston");
const { combine, timestamp, printf } = format;
const { logger: config } = require("../config");
const { logger: config } = require("../../config");

const logger = createLogger({
level: config.level,
Expand Down
2 changes: 1 addition & 1 deletion backend/mtgjson.js → backend/core/mtgjson.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const fs = require("fs");
const path = require("path");
const {getDataDir} = require("../backend/data");
const {getDataDir} = require("./data");
const VERSION_FILE = path.join(getDataDir(), "version.json");
const logger = require("./logger");
const semver = require("semver");
Expand Down
4 changes: 2 additions & 2 deletions backend/data-watch.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const fs = require("fs");
const {reloadData} = require("./data");
const logger = require("./logger");
const {reloadData} = require("./core/data");
const logger = require("./core/logger");

/**
* Add a watch on fs to get updated even from external process
Expand Down
4 changes: 2 additions & 2 deletions backend/game.js
Original file line number Diff line number Diff line change
Expand Up @@ -8,9 +8,9 @@ const Human = require("./player/human");
const Pool = require("./pool");
const Room = require("./room");
const Rooms = require("./rooms");
const logger = require("./logger");
const logger = require("./core/logger");
const Sock = require("./sock");
const {saveDraftStats, getDataDir} = require("./data");
const {saveDraftStats, getDataDir} = require("./core/data");

module.exports = class Game extends Room {
constructor({ hostId, title, seats, type, sets, cube, isPrivate, modernOnly, totalChaos, chaosPacksNumber, picksPerPack }) {
Expand Down
2 changes: 1 addition & 1 deletion backend/player/bot.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
const {sample, pull, times} = require("lodash");

const Player = require("./index");
const logger = require("../logger");
const logger = require("../core/logger");

module.exports = class Bot extends Player {
constructor(picksPerPack, burnsPerPack, gameId) {
Expand Down
2 changes: 1 addition & 1 deletion backend/player/human.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const {pull, find, pullAllWith, remove, times, sample, chain} = require("lodash"
const Player = require("./index");
const util = require("../util");
const hash = require("../hash");
const logger = require("../logger");
const logger = require("../core/logger");

module.exports = class Human extends Player {
constructor(sock, picksPerPack, burnsPerPack, gameId) {
Expand Down
2 changes: 1 addition & 1 deletion backend/pool.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const {sample, shuffle, random, range, times, constant, pull} = require("lodash");
const boosterGenerator = require("./boosterGenerator");
const { getCardByUuid, getCardByName, getRandomSet, getExpansionOrCoreModernSets: getModernList, getExansionOrCoreSets: getSetsList } = require("./data");
const { getCardByUuid, getCardByName, getRandomSet, getExpansionOrCoreModernSets: getModernList, getExansionOrCoreSets: getSetsList } = require("./core/data");
const draftId = require("uuid").v1;

/**
Expand Down
2 changes: 1 addition & 1 deletion backend/pool.spec.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@
const assert = require("assert");
const Pool = require("./pool");
const {range, times, constant} = require("lodash");
const {getPlayableSets} = require("./data");
const {getPlayableSets} = require("./core/data");

const assertPackIsCorrect = (got) => {
const cardIds = new Set();
Expand Down
2 changes: 1 addition & 1 deletion backend/router.js
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ const Game = require("./game");
const Rooms = require("./rooms");
const Sock = require("./sock");
const util = require("./util");
const logger = require("./logger");
const logger = require("./core/logger");

function create(opts) {
try {
Expand Down
4 changes: 2 additions & 2 deletions backend/sock.js
Original file line number Diff line number Diff line change
@@ -1,7 +1,7 @@
const { EventEmitter } = require("events");
const { app: { DEFAULT_USERNAME } } = require("../config");
const { getPlayableSets, getLatestReleasedSet, getBoosterRulesVersion } = require("./data");
const { getVersion } = require("./mtgjson");
const { getPlayableSets, getLatestReleasedSet, getBoosterRulesVersion } = require("./core/data");
const { getVersion } = require("./core/mtgjson");

// All sockets currently connected to the server.
let allSocks = [];
Expand Down
2 changes: 1 addition & 1 deletion backend/util.js
Original file line number Diff line number Diff line change
@@ -1,6 +1,6 @@
const assert = require("assert");
const {countBy} = require("lodash");
const { getSet, getCardByName } = require("./data");
const { getSet, getCardByName } = require("./core/data");
const BASICS = [
"Forest",
"Island",
Expand Down