Skip to content
This repository has been archived by the owner on Jun 24, 2021. It is now read-only.

dockerize lhd #45

Merged
merged 7 commits into from
Oct 15, 2019
Merged

dockerize lhd #45

merged 7 commits into from
Oct 15, 2019

Conversation

kozr
Copy link
Contributor

@kozr kozr commented Sep 27, 2019

please check if it's correct

Copy link
Contributor

@Ciuffi Ciuffi left a comment

Choose a reason for hiding this comment

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

It doesn't work for me :( I still get a seg fault because its trying to run mac modules in a linux container. Check out what I wrote on slack for how to fix this.

command: yarn run dev
working_dir: /usr/src/nuxt-app
volumes:
- .\:/usr/src/nuxt-app
Copy link
Contributor

Choose a reason for hiding this comment

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

.\ doesn't work on unix machines, like macs, use just . instead

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Wish I could join u guys for dinner instead of doing hwk and stuff ;(

Dockerfile Outdated
WORKDIR /usr/src/nuxt-app
COPY package*.json yarn.lock ./
RUN yarn install
RUN npm rebuild node-sass
Copy link
Contributor

Choose a reason for hiding this comment

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

This shouldn't need to happen if the modules are installed for linux already

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you referring to the npm rebuild node-sass?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sass/node-sass#2536 I was getting some error highly related with this. I had to use npm rebuild node-sass to fix it on my computer.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think this makes it work because it rebuilds the node modules in your host computers node folder into Linux versions. What we want is tell docker to use the node modules built into the docker image already. I explained on slack how to do this, it’s a pretty simple addition to the volumes field.

Copy link
Contributor

Choose a reason for hiding this comment

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

Add a new volume:
- /usr/src/nuxt-app/node_modules
Under the other volume mount in the docker compose

version: '3.7'
services:
nuxt:
image: kozr/lhd2020_nuxt
Copy link
Contributor

Choose a reason for hiding this comment

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

We probably want to be able to rebuild the container in case we add more node modules, we should use build instead of a preset image.

.dockerignore Outdated
@@ -0,0 +1,4 @@
node_modules
Copy link
Contributor

Choose a reason for hiding this comment

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

Just an FYI, dockerignore only affects the building of the image, not the volume mounts

@kozr kozr requested a review from Ciuffi October 10, 2019 18:07
Copy link
Contributor

@Ciuffi Ciuffi left a comment

Choose a reason for hiding this comment

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

Lookin good now!

@ianmah
Copy link
Contributor

ianmah commented Oct 14, 2019

can you add instructions for using this cuz idk what im supposed to do

@Ciuffi
Copy link
Contributor

Ciuffi commented Oct 15, 2019

@ianmah Theres some in the Readme, do you think we should add more?

@Ciuffi Ciuffi merged commit 69bcfef into dev Oct 15, 2019
@Ciuffi
Copy link
Contributor

Ciuffi commented Oct 15, 2019

oops i didn't mean to merge this..

@kozr kozr deleted the dockerfile branch October 15, 2019 21:29
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants