-
Notifications
You must be signed in to change notification settings - Fork 27
Fix docker setup #233
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
Fix docker setup #233
Conversation
|
Ok, so there is a slight hack here. The docker image must have the right |
7d14a67 to
09ed9c6
Compare
|
So you say that we are not able anymore to deploy that container outside of the fixed environments we specify on build? And we are not able to specify vars (which would be „variable“) at runtime anymore? That point is critical as we have to be able to deploy fast with changes settings without waiting for a whole build every time. The standard behavior for docker images should be always, build once, deploy everywhere. We really need to keep that workflow intact. |
175170b to
8f790f3
Compare
appinteractive
left a comment
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.
error Found incompatible module```
trying to fix it...
| WORKDIR /WebApp/ | ||
| # --no-cache: download package index on-the-fly, no need to cleanup afterwards | ||
| # --virtual: bundle packages, remove whole bundle at once, when done | ||
| RUN apk --no-cache --virtual build-dependencies add git python make g++ |
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.
@roschaefer What about security updates? Dont we need them?
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.
Also why do you need phython, make and g++?
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.
I was having troubles to build other dependencies. Surprisingly. I haven't researched the issue but as far as I remember that was related to the newer node version of this docker container. 🤔
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.
with node:8-alpine it does work, with node:alpine not
Dockerfile
Outdated
| @@ -1,46 +1,32 @@ | |||
| FROM node:8.9-alpine | |||
| FROM node: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.
Are you sure that we should go with the cutting edge versions? Not at least use LTS or similar?
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.
👍
| ENV WEBAPP_HOST=0.0.0.0 | ||
| # optional git commit hash | ||
| ARG BUILD_COMMIT | ||
| ENV BUILD_COMMIT=$BUILD_COMMIT |
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.
Changing parts should always go lower to increase cache efficienty at build time.
| # provide defaults | ||
| COPY .env.template /WebApp/.env | ||
| RUN yarn run build | ||
| CMD ["pm2", "start", "node", "build/main.js", "-n", "frontend", "-i", "2", "--attach"] |
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.
maybe use yarn start:pm2?
plugins/api.js
Outdated
| clear: () => { | ||
| const res = app.$cookies.removeAll() | ||
| if (process.env.NODE_ENV === 'development') { | ||
| if (env.NODE_ENV === 'development') { |
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.
are you sure you will geht that from the nuxt env?
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.
Yes, that's a nuxtjs feature.
Via process.env.baseUrl.
Via context.env.baseUrl, see context API.
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.
Ah, you mean NODE_ENV in particular? Let me see..
| const secrets = dotenv.parse(await readFileSync(resolve('./server/.env-secrets'))) | ||
|
|
||
| if (!process.client && secrets.SENTRY_DNS_PRIVATE) { | ||
| if (!process.client && process.env.SENTRY_DNS_PRIVATE) { |
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 approach would get you the variable at build time but not at runtime as nuxt edges them in there. Or do I miss something?
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 file is executed on the server. I double checked at the time, that console.log(process.env.WHATEVER) spits out the WHATEVER variable server-side but not client-side.
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.
If we had env variables that we need on the client, we would have to explicitly whitelist them.
nuxt.config.js
Outdated
| modules: [ | ||
| 'cookie-universal-nuxt', | ||
| '@nuxtjs/dotenv' | ||
| ['@nuxtjs/dotenv', { |
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.
don't you not need to also overwrite the nuxt config like discribed here: https://github.com/nuxt-community/dotenv-module#using-env-file-in-nuxtconfigjs
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.
? The config file name? I was playing with if statements in here based on the current process.env.NODE_ENV when nuxt reads in this file but that never fully worked as it should. So I went back to env-sub.
|
@roschaefer how was your procedure for testing if the env vars work correctly? |
|
Just as a reference after we had a talk together: the goal is to not build the application every time the docker image is started but only run it while at the same time providing current environemnt variables that might be passed to the docker command to the application (without rebuild) |
|
@appinteractive changing the app via env variables should be possible now. |
Still getting the same issue: Changing the minimum node version in the package.json does not help |
|
I fixed the issue by pinning the node version to the future Node 10.* LTS. @roschaefer how about the env issues? Any updates there? Sounded like there are still not resolved!? |
|
@appinteractive I didn't get any build issues at the time, my last commit also greened the build server. Glad if it is fixed on more than one machine now 😆 . This branch here is OK 👌 the env issues are a problem in #228 but not here. |
|
@roschaefer I still could not test it as I can’t access the docker image via browser. Any suggestions how to test if the bars are really there? |
- no bash scripts * error prone * platform dependent)
* .nuxt seems to be a build folder and should not be synced * load config into process.env for local development
... and remove redundancy in docker-compose files
With server side rendering it is simply not possible to run the webapp without a bridged network
The point of this docker-compose file is to give a running *local* environment. It is not complete up to this point. Eventually we could share this configuration with our staging system online. Also this commit passes secret environment variables from host system to container.
* Install nuxt-env * Replace env with app.$env when possible * Remove custom plugins/env.js
* add yarn run build to Dockerfile * remove envsub (now unnecessary) - all configurations are controlled via env variables passed from host to container * remove .env.template
12d35b3 to
b5058ed
Compare
We will need that for the client in the future, anyways.
cbbe3b7 to
656a35e
Compare
|
@appinteractive & @roschaefer Ready for #233 ? |
|
Can test it as soon as Monday. Staging is down until Monday too. Would like to first get it working to see if this merge brakes something. |
|
Okay |
* less redundancy * different environments (local staging + local development)
| # install current version of yarn | ||
| before_install: | ||
| - curl -o- -L https://yarnpkg.com/install.sh | bash | ||
| - export PATH="$HOME/.yarn/bin:$PATH" |
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.
@appinteractive is this redundant?
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.
Yes it can be removed. Was caused by the unversionated bode image before.
aeefc0a to
01054f4
Compare
x