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

Docker changes and optimisations - #148

Open
kawaii wants to merge 9 commits into
atmos:masterfrom
kawaii:master
Open

Docker changes and optimisations #148
kawaii wants to merge 9 commits into
atmos:masterfrom
kawaii:master

Conversation

@kawaii

@kawaii kawaii commented Apr 26, 2019

Copy link
Copy Markdown

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

kawaii commented Apr 26, 2019

Copy link
Copy Markdown
Author

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

rmm5t commented Apr 26, 2019

Copy link
Copy Markdown
Contributor

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

@kawaii

kawaii commented Apr 26, 2019

Copy link
Copy Markdown
Author

@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

rmm5t commented Apr 27, 2019

Copy link
Copy Markdown
Contributor

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

@kawaii

kawaii commented Apr 27, 2019

Copy link
Copy Markdown
Author

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

kawaii commented Jun 19, 2019

Copy link
Copy Markdown
Author

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

@rmm5t rmm5t left a comment

Copy link
Copy Markdown
Contributor

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.

Comment thread Dockerfile Outdated
@kawaii

kawaii commented Jul 28, 2019

Copy link
Copy Markdown
Author

Screams into cyberspace, but no one hears.

@wallopthecat

Copy link
Copy Markdown

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