Build webapp assets during Docker build instead of checking them into git#611
Conversation
… git - Add webapp build stage to Dockerfile and Dockerfile.build-static using node:lts-alpine - Move build-webapp.sh to scripts/ directory - Add .dockerignore to exclude node_modules and reduce build context - Update .gitignore to exclude compiled webapp assets - Remove compiled webapp assets from repository
gabriel-samfira
left a comment
There was a problem hiding this comment.
I've been meaning to address this for a while, but have had very little time lately. So thanks!
Just a few comments for the build-static bit. Let me know what you think
| @@ -1,3 +1,13 @@ | |||
| # Stage 1: Build webapp | |||
There was a problem hiding this comment.
This Dockerfile was initially written to be built once and reused many times by just creating a new temporary container from it and running the build-static.sh script. But given that we might need to also build and copy the webapp now, I think we can expand it a bit more and just run docker build to get the static binary. See bellow comments.
| # Copy webapp build output to assets directory | ||
| COPY --from=webapp /build/webapp/build/ /build/garm/webapp/assets/ | ||
|
|
||
| CMD ["/bin/sh"] |
There was a problem hiding this comment.
We can replace this with
| CMD ["/bin/sh"] | |
| ARG GARM_REF="" | |
| ENV GARM_REF=${GARM_REF} | |
| RUN /build-static.sh | |
| # Stage 3: Export only the binaries | |
| FROM scratch AS export | |
| COPY --from=builder /build/output/ / | |
| CMD ["/bin/sh"] |
See the Makefile comment for the last bit.
| ##@ Build | ||
|
|
||
| .PHONY : build-static test install-lint-deps lint go-test fmt fmtcheck verify-vendor verify create-release-files release | ||
| build-static: ## Build garm statically |
There was a problem hiding this comment.
Then this section can be replaced with something like:
build-static: ## Build garm statically
@echo Building garm
mkdir -p build
$(IMAGE_BUILDER) build --no-cache $(EXTRA_ARGS) \
--build-arg GARM_REF=$(GARM_REF) \
--output type=local,dest=$(PWD)/build \
--target=export \
-f Dockerfile.build-static .
@echo Binaries are available in $(PWD)/buildThere was a problem hiding this comment.
We should probably also add --no-cache to the build command.
gabriel-samfira
left a comment
There was a problem hiding this comment.
I tried to add the changes to your branch, but couldn't. I added a couple of more suggestions to the review. I would love to merge this once changes are made.
Here is a full diff to your branch with the requested changes:
diff --git a/.dockerignore b/.dockerignore
index bd85b168..1cc2cce8 100644
--- a/.dockerignore
+++ b/.dockerignore
@@ -3,7 +3,6 @@ webapp/node_modules/
webapp/build/
# Go
-vendor/
# IDE
.vscode/
diff --git a/Dockerfile.build-static b/Dockerfile.build-static
index 34d95932..411ebfd2 100644
--- a/Dockerfile.build-static
+++ b/Dockerfile.build-static
@@ -8,7 +8,7 @@ COPY webapp/ ./
RUN npm run build
# Stage 2: Build Go binaries
-FROM docker.io/golang:alpine
+FROM docker.io/golang:alpine AS builder
WORKDIR /root
USER root
@@ -27,4 +27,10 @@ ADD . /build/garm
# Copy webapp build output to assets directory
COPY --from=webapp /build/webapp/build/ /build/garm/webapp/assets/
-CMD ["/bin/sh"]
+ARG GARM_REF
+ENV GARM_REF=${GARM_REF}
+RUN /build-static.sh
+
+# Stage 3: Export only the binaries
+FROM scratch AS export
+COPY --from=builder /build/output/ /
diff --git a/Makefile b/Makefile
index 080919ef..fda63e87 100644
--- a/Makefile
+++ b/Makefile
@@ -39,9 +39,12 @@ default: build
.PHONY : build-static test install-lint-deps lint go-test fmt fmtcheck verify-vendor verify create-release-files release
build-static: ## Build garm statically
@echo Building garm
- $(IMAGE_BUILDER) build $(EXTRA_ARGS) --tag $(IMAGE_TAG) -f Dockerfile.build-static .
mkdir -p build
- $(IMAGE_BUILDER) run --rm -e USER_ID=$(USER_ID) -e GARM_REF=$(GARM_REF) -e USER_GROUP=$(USER_GROUP) -v $(PWD)/build:/build/output:z $(IMAGE_TAG) /build-static.sh
+ $(IMAGE_BUILDER) build --no-cache $(EXTRA_ARGS) \
+ --build-arg GARM_REF=$(GARM_REF) \
+ --output type=local,dest=$(PWD)/build \
+ --target=export \
+ -f Dockerfile.build-static .
@echo Binaries are available in $(PWD)/build
clean: ## Clean up build artifacts
diff --git a/scripts/build-static.sh b/scripts/build-static.sh
index 1f81752e..9fa6a770 100755
--- a/scripts/build-static.sh
+++ b/scripts/build-static.sh
@@ -1,4 +1,5 @@
#!/bin/sh
+set -e
GARM_SOURCE="/build/garm"
git config --global --add safe.directory /build/garm| webapp/build/ | ||
|
|
||
| # Go | ||
| vendor/ |
There was a problem hiding this comment.
This should be removed, otherwise build-static.sh will fail. As the build commands there use -mod vendor
| ##@ Build | ||
|
|
||
| .PHONY : build-static test install-lint-deps lint go-test fmt fmtcheck verify-vendor verify create-release-files release | ||
| build-static: ## Build garm statically |
There was a problem hiding this comment.
We should probably also add --no-cache to the build command.
|
will merge as-is and apply the diff in a separate branch |
This closes #609
DockerfileandDockerfile.build-staticusingnode:lts-alpinebuild-webapp.shtoscripts/directory.dockerignoreto excludenode_modulesand reduce build context.gitignoreto exclude compiledwebappassetswebappassets from repository, keeping it up to date was a constant source of conflict and doing this was bad practice generally anyway