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

Migration to node 12 #160

Merged
merged 2 commits into from
Aug 11, 2019
Merged

Conversation

UlisesGascon
Copy link
Collaborator

@UlisesGascon UlisesGascon commented Aug 7, 2019

Context:

Original issue: #150
Release Target: 1.5

  • Add support for Node 12
  • Add support for Node 10
  • Add support for Node 8
  • Update travis.yaml with the builds for those environments
  • Update the readme.md with the relevant info

Changelog

  • 1ff89cb Added Node v8 and v10 to Travis
  • 1ff89cb Update references in doc
  • ae31b3e Dockerfile upgraded to Node v12

@UlisesGascon
Copy link
Collaborator Author

This PR will help #149 #156 CC: @servatj

@servatj
Copy link
Contributor

servatj commented Aug 7, 2019

Indeed, Once this #160 and these #161 #156 are approved and merged I will be able to continue with #149 @UlisesGascon @ckarande 👍

Copy link
Member

@ckarande ckarande left a comment

Choose a reason for hiding this comment

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

@UlisesGascon Thanks for the PR 👍

Do you have a separate issue to update the Dockerfile to upgrade the version of Node to v12?

@UlisesGascon
Copy link
Collaborator Author

Great catch @ckarande. 👍

I forgot to update the dockerfile. Now is added to the PR 😉

@ckarande
Copy link
Member

ckarande commented Aug 8, 2019

Awesome. Thanks @UlisesGascon

Should we update the apline-node version? Were you able to run the NodeGoat app in docker with these changes?

@lirantal @binarymist can you please help reviewing the docker config changes as well?

@ckarande ckarande requested review from lirantal and binarymist August 8, 2019 15:40
@UlisesGascon
Copy link
Collaborator Author

I am thinking that alpine version is fine, as we are not using any special OS Core functionality. But I will appreciate also the good advice from @lirantal @binarymist 👍

Alpine Linux is a distribution that was almost purpose-built for Docker images and other small, container-like uses. It clocks in at a whopping 5MB of drive space for the base operating system.

By the time you add in the Node.js runtime requirements, this image does move up to around 50MB in space. But even at 10x the original Alpine size, it’s still 1/5 the size of the the -slim option.

Quotes source and More info

@ckarande
Copy link
Member

ckarande commented Aug 9, 2019

Great! Thanks for sharing the info on the alpine version. I am fine with leaving the version as-is as you proposed. It seem @binarymist is also fine with it. I have approved the PR. 👍 We can adjust it later if needed based on any additional feedback.

Great work 🎉

@binarymist
Copy link
Collaborator

My thumbs up was to give switching to alpine a go. Personally I use the alpine flavours where ever possible.

@UlisesGascon
Copy link
Collaborator Author

I will merge it now then ;-)

@UlisesGascon UlisesGascon merged commit 5f21b4c into OWASP:release-1.5 Aug 11, 2019
This was referenced Aug 11, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants