Skip to content

Conversation

@roschaefer
Copy link
Contributor

@roschaefer roschaefer commented Aug 14, 2018

x

@roschaefer
Copy link
Contributor Author

Ok, so there is a slight hack here. The docker image must have the right .env at build time. So, I added all environment variables which are public to .env.staging and .env.example respectively. We still need .env.production. During build, depending on build argument ENV_FILE, the desired config file is moved into place. All secret environment variables can be passed as environment variables at runtime and are not be exposed to the client. As long as we have no secrets that need to be available at build time, this approach should work.

@roschaefer roschaefer requested review from appinteractive and visualjerk and removed request for appinteractive August 16, 2018 15:55
@roschaefer roschaefer force-pushed the fix-docker-setup branch 2 times, most recently from 7d14a67 to 09ed9c6 Compare August 16, 2018 16:17
@appinteractive
Copy link
Member

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.

@roschaefer roschaefer force-pushed the fix-docker-setup branch 3 times, most recently from 175170b to 8f790f3 Compare August 19, 2018 11:19
Copy link
Member

@appinteractive appinteractive left a 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++
Copy link
Member

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?

Copy link
Member

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++?

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 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. 🤔

Copy link
Member

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
Copy link
Member

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?

Copy link
Contributor Author

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
Copy link
Member

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"]
Copy link
Member

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') {
Copy link
Member

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?

Copy link
Contributor Author

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.

See https://nuxtjs.org/api/configuration-env/

Copy link
Contributor Author

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) {
Copy link
Member

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?

Copy link
Contributor Author

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.

Copy link
Contributor Author

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', {
Copy link
Member

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

Copy link
Contributor Author

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.

@appinteractive
Copy link
Member

@roschaefer how was your procedure for testing if the env vars work correctly?

@appinteractive
Copy link
Member

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)

@roschaefer
Copy link
Contributor Author

@appinteractive changing the app via env variables should be possible now.

@appinteractive appinteractive mentioned this pull request Sep 2, 2018
2 tasks
@appinteractive appinteractive removed the request for review from visualjerk September 17, 2018 09:53
@appinteractive
Copy link
Member

docker build -t humanconnection/frontend-nuxt .

Still getting the same issue:

Step 13/17 : RUN yarn install --production=false --frozen-lockfile --non-interactive
 ---> Running in 05485f872598
yarn install v0.23.2
[1/4] Resolving packages...
[2/4] Fetching packages...
error [email protected]: The engine "node" is incompatible with this module. Expected version ">=8.0.0".
error Found incompatible module

Changing the minimum node version in the package.json does not help

"engines": {
    "node": ">= 10.0.0",
    "yarn": ">= 1.3.0"
  },

@appinteractive
Copy link
Member

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!?

@roschaefer
Copy link
Contributor Author

@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.

@appinteractive
Copy link
Member

@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?

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
@roschaefer roschaefer force-pushed the fix-docker-setup branch 2 times, most recently from cbbe3b7 to 656a35e Compare September 19, 2018 17:47
@Lulalaby
Copy link
Contributor

@appinteractive & @roschaefer Ready for #233 ?

@appinteractive
Copy link
Member

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.

@Lulalaby
Copy link
Contributor

Okay

@Lulalaby Lulalaby changed the title Fix docker setup [WIP] Fix docker setup Sep 22, 2018
Michael Both and others added 3 commits September 30, 2018 13:58
@roschaefer roschaefer changed the title [WIP] Fix docker setup Fix docker setup Oct 1, 2018
# install current version of yarn
before_install:
- curl -o- -L https://yarnpkg.com/install.sh | bash
- export PATH="$HOME/.yarn/bin:$PATH"
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@appinteractive is this redundant?

Copy link
Member

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.

@roschaefer roschaefer merged commit 8053a24 into develop Oct 1, 2018
@delete-merged-branch delete-merged-branch bot deleted the fix-docker-setup branch October 1, 2018 12:04
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants