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 dockerfile -> Adding manually missing Python dependency prior to npm install and npmrc #6

Conversation

danielherrerohernando
Copy link
Member

@danielherrerohernando danielherrerohernando commented Aug 20, 2019

Using node-alpine, when building a docker image, we are getting an error related to some Python missing dependency, this PR fixes that.

Log with the aforementioned error when building image:
https://drive.google.com/file/d/11kyHQ2nxtnEp-pYW1nQB0SafE9K8bGWT/view?usp=sharing

Further info -> nodejs/docker-node#282

Also this PR adds the .npmrc file (@kevinccbsg )

@danielherrerohernando danielherrerohernando changed the title Fix dockerfile -> Adding manually missing Python dependency prior to npm install Fix dockerfile -> Adding manually missing Python dependency prior to npm install and npmrc Aug 20, 2019
Copy link

@kevinccbsg kevinccbsg left a comment

Choose a reason for hiding this comment

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

The .npmrc file looks good to me!!

@danielherrerohernando
Copy link
Member Author

@MatteoDiPaolo
Copy link

@w3dani please have a look at it, I'm not sure if there is another way of doing it.

@feliun
Copy link

feliun commented Aug 22, 2019

I don't think this has bad implications, but I'm interested to know about the root cause for this error, I'd be surprised if the node alpine image wasn't prepared to build because of a missing dep? @w3dani thoughts?

@danielherrerohernando
Copy link
Member Author

@feliun The reason for the error seems to be simple, node-alpine versions do not have Python installed, so when building the image, npm install command throws an error because some dependency needs Python.

Using the --virtual flag in the RUN command (part of Alpine´s APK) we can run npm install with Python installed and, after npm install finishes, Python is uninstalled.

Apart from the original link on the first comment, here you can find other issues where people realize about missing Python on Alpine:
nodejs/docker-node#384
nodejs/docker-node#715

@w3dani
Copy link

w3dani commented Aug 29, 2019

The main goals of using alpine images are:

  • Increase the speed of the CICD pipeline
  • Generate lighter artifacts

This change would impact on the first point so it would be interesting measure the impact. Apart from this, I don't see any problem.
There is another point of view. The problem is we need to install python because bcrypt uses node-gyp and this one needs python. Other solutions could be:

  • Use another package different from bcrypt, which does not use node-gyp.
  • Check the need of add this change to the yeoman template (we are not using bcrypt or any other package using node-gyp in shop, so it could not be really a need for all the projects). Maybe this must be only in those projects which are using those packages (however, sooner or later some other package we need could also use node-gyp)

Copy link

@w3dani w3dani left a comment

Choose a reason for hiding this comment

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

It's ok for me if we agree in looking over the dependencies used in the future

@kevinccbsg
Copy link

@danielherrerohernando @w3dani @MatteoDiPaolo @Betisman could we merge this and before merge this update the Dockerfile version to at least 12?

Screen Shot 2020-07-09 at 09 59 33

@Betisman
Copy link

Betisman commented Jul 9, 2020

In order to approve the PR, I would like to know about the points @w3dani explains in #6 (comment). They seem to be interesting.

Ok, I see @w3dani already approved it, so I guess we're ready to merge this.

@Betisman
Copy link

Betisman commented Jul 9, 2020

Well, @kevinccbsg after a second reading of your comment, you're proposing to update the Dockerfile in this same PR and then merge it, right?

@MatteoDiPaolo
Copy link

Go ahead guys!!!

@danielherrerohernando
Copy link
Member Author

11 months later this PR will be merged, #emotion 😢

@MatteoDiPaolo
Copy link

MatteoDiPaolo commented Jul 9, 2020

11 months later this PR will be merged, #emotion 😢

It took more than deliveringg a baby

@kevinccbsg
Copy link

Well, @kevinccbsg after a second reading of your comment, you're proposing to update the Dockerfile in this same PR and then merge it, right?

Yes, that's my plan and also update dependencies.

@kevinccbsg
Copy link

I cannot modify this branch as it is a PR from another fork so I'm going to merge it

@kevinccbsg kevinccbsg merged commit ca49512 into infinitaslearning:master Jul 9, 2020
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.

6 participants