Skip to content
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 Node container errors #29

Merged
merged 3 commits into from
Nov 21, 2018
Merged

Conversation

lbassin
Copy link
Contributor

@lbassin lbassin commented Nov 9, 2018

This PR resolves #28

I changed default command of the node container and add missing packages
With this PR we'll be able to successfully build our js and scss

@cgrandval
Copy link
Contributor

@lbassin I prefer to put dockerfile files in a docker directory too (@tdutrion doesn't agree, I think). So, a little comment to decide definitively what is our final decision as it's not the case for now with the other php dockerfile

@cgrandval cgrandval self-requested a review November 12, 2018 16:58
Copy link
Contributor

@cgrandval cgrandval left a comment

Choose a reason for hiding this comment

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

I have this error

node_1 | yarn install v1.12.1
node_1 | info Visit https://yarnpkg.com/en/docs/cli/install for documentation about this command.
node_1 | error install has been replaced with add to add new dependencies. Run "yarn add && yarn encore dev" instead.

@cgrandval
Copy link
Contributor

If I use the node:latest image (in the docker-compose.yml), everything's fine for me. Perhaps, it's better to change the tag of the node image we're using. What do you think @lbassin ?

@cgrandval cgrandval added the bug Something isn't working label Nov 13, 2018
@tdutrion tdutrion mentioned this pull request Nov 15, 2018
2 tasks
@lbassin
Copy link
Contributor Author

lbassin commented Nov 19, 2018

I agree to use node:latest, this tag is just bigger but it shouldn't be an issue in this case

What should we do about others dockerfile ?
Do we move them to a docker folder to keep a clean and structured root folder ? In this case, i think it should be done in another PR

@cgrandval
Copy link
Contributor

I agree to put everything in a docker folder. I think Delphine did that but @tdutrion asked her to put everything at the root. Don't know why :-)

@lbassin
Copy link
Contributor Author

lbassin commented Nov 19, 2018

Considering this subject isn't related to the node container i am going to open an new issue to talk about it

We can merge this PR if everything is ok for you @cgrandval

@tdutrion tdutrion merged commit fee1752 into DarkmiraTour:develop Nov 21, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix nodejs container error
3 participants