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

Docker changes and optimisations #148

Open
wants to merge 9 commits into
base: master
Choose a base branch
from
Open

Conversation

kawaii
Copy link
Contributor

@kawaii kawaii commented Apr 26, 2019

Some small yet quite important changes. All tested and documented below:

  • Removes the redundant mkdir /opt/camo layer of the Dockerfile since WORKDIR makes the directory if it doesn't already exist.
  • Adds a .dockerignore file and changes 3 ADD layers into a single one with the same effect.
  • Bases the image from Alpine Linux (still an official node image) to greatly reduce the size and attack surface of the image/container.

@kawaii
Copy link
Contributor Author

kawaii commented Apr 26, 2019

I've pushed another commit, ccaacf5 - which was a change already proposed as part of PR #134. Figured we could hopefully kill many birds with one stone.

@rmm5t
Copy link
Contributor

rmm5t commented Apr 26, 2019

@kawaii node-8.16 is now the latest node 8 version.

@kawaii
Copy link
Contributor Author

kawaii commented Apr 26, 2019

@rmm5t thank you for informing me of this, I simply based my commit from your earlier PR without checking Docker Hub for updated information. Perhaps it would be best for us to test the application under a more modern Node.js version such as 11 or 12?

@rmm5t
Copy link
Contributor

rmm5t commented Apr 27, 2019

Odd numbers are unstable versions. Version 12 was just released this week. Node 10 is probably the most appropriate for now.

@kawaii
Copy link
Contributor Author

kawaii commented Apr 27, 2019

I've made some more pushes with some general quality of life improvements to the Dockerfile, such as using COPY instead of ADD as suggested by Docker Captain @BretFisher! :)

@kawaii
Copy link
Contributor Author

kawaii commented Jun 19, 2019

Bump! Anyone with merge powers able to check this over? :)

Copy link
Contributor

@rmm5t rmm5t left a comment

Choose a reason for hiding this comment

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

I don't have merge powers, but maybe a code review and another set of eyes on this will help.

Dockerfile Outdated Show resolved Hide resolved
@kawaii
Copy link
Contributor Author

kawaii commented Jul 28, 2019

Screams into cyberspace, but no one hears.

@wallopthecat
Copy link

This really should be merged. Thanks, using your fork in a project now

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