-
Notifications
You must be signed in to change notification settings - Fork 208
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
Add the transpiled artifacts to build/ for a slimmer native Node Dock… #306
Conversation
@@ -5,7 +5,9 @@ let express = require('express'); | |||
let router = express.Router(); | |||
let logger = require('../utils/logger'); | |||
const MIN_BTC_BLOCK = 670000; | |||
console.log('using config', JSON.stringify(config)); | |||
if (process.env.NODE_ENV !== 'prod') { |
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 password logs in production.
@@ -34,10 +32,11 @@ COPY --from=perms /etc/group /etc/passwd /etc/shadow /etc/ | |||
COPY --from=builder /lndhub /lndhub | |||
|
|||
# Create logs folder and ensure permissions are set correctly | |||
RUN mkdir /lndhub/logs && chown -R lndhub:lndhub /lndhub | |||
RUN mkdir -p /lndhub/logs && chown -R lndhub:lndhub /lndhub |
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 broke the Docker build if the directory already existed.
|
||
# Delete git data as it's not needed inside the container | ||
RUN rm -rf .git | ||
|
||
FROM node:16-bullseye-slim | ||
FROM node:16-alpine |
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.
Reduces the Docker image by ~100MB.
@AaronDewes can you pls take a look..? |
Sorry for the delay, while I agree the base image should be changed, #314 is better than this PR because it drops babel entirely. @kwizzn Can I merge this into #314? |
Sure @AaronDewes, go ahead. |
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.
lgtm
so, what's the verdict? |
This can probably get merged, I'll improve the other pr later |
thanks! |
hmm, after merging this i get build fail:
|
This adds the transpiled artifacts to a new
build/
directory for a slimmer (node:16-alpine) Docker image that does not require babel-node. This is a first iteration that makes LndHub run in our environment, I'm contributing this upstream in case it helps anyone. If it is appreciated, I have some future improvements in the pipeline. :)