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

Remove root user from docker, update ports and update Node / libraries #281

Merged
merged 13 commits into from Mar 26, 2020
Merged

Remove root user from docker, update ports and update Node / libraries #281

merged 13 commits into from Mar 26, 2020

Conversation

jovanmaric
Copy link
Contributor

@jovanmaric jovanmaric commented Jan 12, 2020

This PR adds the following:

  • Change the default user from root to node in image
    Currently the user is set as root, which is unsafe. I have changed the user to the default node user,
    along with the working directory to /home/node, so that the dependencies and the app are owned by the node user. Keep in mind that the node user can still overlap because of its id and group id (1000, 1000 respectively).
  • Increase the default ports with 1000:
    Ports 80 and 25 are mostly reserved for other containers / host applications. These are changed to
    1080 and 1025, which better isolates this application for development. This fixes Change default Dockerfile port from 80/25 #244 and makes
    Updated default web and smtp ports #273 obsolete. This is a needed fix for podman users as well, if ports '< 1024' aren't enabled in the
    user space.
  • Change the entrypoint to an absolute path:
    Latest Podman is having trouble with relative paths for some reason. This makes the binary location more explicit as well.
  • Updated node to the latest LTS:
    I'm not sure what implications this has for users which aren't using containers. Can this application
    be included as a library? I have made the assumption here that it is meant to be run standalone /
    containerized, and therefore bumping the version to the latest LTS should be fine. I could change
    this to V10(Active LTS until 2021) if the version is too high?
  • Updated libraries and fixed npm audit security issues:
    Fixed the 75 security issues which npm audit reported.

I wondering what you think of this PR. It would be great to have a version bump on dockerhub as well if this gets merged.

@jovanmaric jovanmaric changed the title Features/update maildev Remove root user from docker, update ports and update Node / libraries Jan 12, 2020
@codecov-io
Copy link

codecov-io commented Jan 12, 2020

Codecov Report

Merging #281 into master will not change coverage by %.
The diff coverage is n/a.

@@           Coverage Diff           @@
##           master     #281   +/-   ##
=======================================
  Coverage   71.65%   71.65%           
=======================================
  Files           9        9           
  Lines         515      515           
  Branches      107      107           
=======================================
  Hits          369      369           
  Misses        146      146           

Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
Copy link
Member

@djfarrelly djfarrelly left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution @jovanmaric! I left a couple comments to request changes. One other idea that we could switch to for the Dockerfile is to use a multi-stage build which would allow the dependency install and install in the /root/ dir to be separate from the final image, also reducing layers of our final image.

Would love to hear your thoughts on that too, although, once these couple changes are made, we can merge this in and publish. I'm just now debating if this change should mean a 2.0 upgrade as it's a breaking change for the Docker image. 🤔

Dockerfile Outdated Show resolved Hide resolved
Dockerfile Show resolved Hide resolved
package.json Outdated Show resolved Hide resolved
@djfarrelly djfarrelly mentioned this pull request Mar 19, 2020
@jovanmaric
Copy link
Contributor Author

Thanks for your contribution @jovanmaric! I left a couple comments to request changes. One other idea that we could switch to for the Dockerfile is to use a multi-stage build which would allow the dependency install and install in the /root/ dir to be separate from the final image, also reducing layers of our final image.

Yes I agree, multi-stage builds are cleaner and more elaborative, however only the targeted stage is actually reduced in layers. The ci system would still need to cache all the needed stages to not require a rebuild every time. Also this would require only two stages, as dividing the stages into "development" and "production" only really works with the new buildkit in docker. Otherwise all stages will be automatically built 😅. Though this last bit is more of an optimization.

Would love to hear your thoughts on that too, although, once these couple changes are made, we can merge this in and publish. I'm just now debating if this change should mean a 2.0 upgrade as it's a breaking change for the Docker image.

Maybe a 2.0 upgrade would be a better fit yes, as I can't tell if this is used in production like environments like staging or review apps.

Regarding your other comments, ill take a look later this evening.

Copy link
Member

@djfarrelly djfarrelly left a comment

Choose a reason for hiding this comment

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

🙌 Thanks for incorporating the changes I requested as well as your response re:version @jovanmaric!

I'll merge this today and starting thinking through what the next release will look like as there are lots of other good useful changes in open PRs!

@djfarrelly djfarrelly merged commit 919f56c into maildev:master Mar 26, 2020
@jovanmaric jovanmaric deleted the features/update_maildev branch March 26, 2020 13:55
@jovanmaric
Copy link
Contributor Author

Thanks for incorporating the changes I requested as well as your response re:version @jovanmaric!

I'll merge this today and starting thinking through what the next release will look like as there are lots of other good useful changes in open PRs!

I'm glad this got merged, thanks! :). Feel free to ping me if you need help with any other issue's / PR's. I'm not that experienced with js / node (other than vue/nuxt and some random dabbles in node), but can always help as a sparring partner.

Cheer!

divbzero added a commit to divbzero/warehouse that referenced this pull request Apr 7, 2022
maildev port was updated with maildev/maildev#281 on 25 Mar 2020.
divbzero added a commit to divbzero/warehouse that referenced this pull request Apr 8, 2022
maildev is used as our mock email service during development. It wasn't
working because maildev ports were updated with maildev/maildev#281 on
25 Mar 2020.
divbzero added a commit to divbzero/warehouse that referenced this pull request Apr 8, 2022
maildev is used as our mock email service during development. It wasn't
working because maildev ports were updated with maildev/maildev#281 on
25 Mar 2020.
divbzero added a commit to divbzero/warehouse that referenced this pull request Apr 8, 2022
maildev, the mock email service used for development, wasn't working:

1. The `warehouse.email.send_email` task hit `ConnectionRefusedError`.
2. The maildev interface http://localhost:1080/ wouldn't load.

Turns out that maildev ports were updated with v1.1.1. Updating ports
according to maildev/maildev#281 resolved both issues, and pinning the
version to v2.0.2 ensures that things won't unexpectedly change again.
divbzero added a commit to divbzero/warehouse that referenced this pull request Apr 8, 2022
maildev, the mock email service used for development, wasn't working:

1. The `warehouse.email.send_email` task hit `ConnectionRefusedError`.
2. The maildev interface http://localhost:1080/ wouldn't load.

Turns out that maildev ports were updated with v1.1.1. Updating ports
according to maildev/maildev#281 resolved both issues, and pinning the
version to v2.0.2 will prevent unexpected changes in the future.
di pushed a commit to pypi/warehouse that referenced this pull request Apr 8, 2022
maildev, the mock email service used for development, wasn't working:

1. The `warehouse.email.send_email` task hit `ConnectionRefusedError`.
2. The maildev interface http://localhost:1080/ wouldn't load.

Turns out that maildev ports were updated with v1.1.1. Updating ports
according to maildev/maildev#281 resolved both issues, and pinning the
version to v2.0.2 will prevent unexpected changes in the future.
domdfcoding pushed a commit to domdfcoding/warehouse that referenced this pull request Jun 7, 2022
maildev, the mock email service used for development, wasn't working:

1. The `warehouse.email.send_email` task hit `ConnectionRefusedError`.
2. The maildev interface http://localhost:1080/ wouldn't load.

Turns out that maildev ports were updated with v1.1.1. Updating ports
according to maildev/maildev#281 resolved both issues, and pinning the
version to v2.0.2 will prevent unexpected changes in the future.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Change default Dockerfile port from 80/25
4 participants