RefugeRestrooms / refugerestrooms

REFUGE restrooms indexes and maps safe restroom locations for trans, intersex, and gender nonconforming individuals.
http://www.refugerestrooms.org
GNU Affero General Public License v3.0
894 stars 261 forks source link

Replace ruby base-image in Dockerfile with slimmer version #504

Closed darpr closed 6 years ago

darpr commented 6 years ago

Context

Summary of Changes

Checklist

Tests passed:

$ docker-compose run -e "RAILS_ENV=test" web rake db:test:prepare spec
Creating network "refugerestrooms_default" with the default driver
Creating refugerestrooms_db_1 ... done
Created database ‘bathrooms_test'
..
..
..
Randomized with seed 54156
.....................................Puma starting in single mode...
* Version 3.11.2 (ruby 2.3.7-p456), codename: Love Song
* Min threads: 0, max threads: 1
* Environment: test
* Listening on tcp://0.0.0.0:39645
Use Ctrl-C to stop
...................

Finished in 23.82 seconds (files took 8.49 seconds to load)
56 examples, 0 failures

Randomized with seed 54156

Coverage report generated for RSpec to /refugerestrooms/coverage. 541 / 594 LOC (91.08%) covered.

Screenshots

Could launch application after the changes: image

Before

With the previous base-image, host machine had a 941 MB Ruby base image.

After

With these changes, the Ruby base image footprint on host is just 254 MB.

DeeDeeG commented 6 years ago

This looks cool, thanks for the pull request!

I am in favor of merging, and we may have another maintainer get a chance to look at this over the weekend. Will wait until then so other maintainers can discuss and give their thoughts.

In any case, happy Hacktoberfest! :jack_o_lantern: :computer: :fallen_leaf: :sparkles:

darpr commented 6 years ago

Thanks for the feedback, @DeeDeeG! Appreciate the work that you guys are doing through this project.

tkwidmer commented 6 years ago

This looks fine to me. @mi-wood do you want to weigh in as the only person with more Ops experience?

DeeDeeG commented 6 years ago

Just a comment: We can look into how many of these apt commands (including from before this pull request) we really need. I have a branch where I removed almost every apt-get update and apt-get install from the dockerfile and it went fine.

The reason our setup can be so simple with regard to apt packages is that the (recently added) nodejs and yarn installation scripts download a whole lot of apt packages (like build-essential) that are generally useful later in the Dockerfile.

But the overall concept of using a slimmer base image sounds good IMO.

I think the criterion for merging this should be "does the app work" vs. errors during docker-compose build or docker-compose up or when testing the app.

DeeDeeG commented 6 years ago

I think we could copy some of the approach taken here: https://github.com/RefugeRestrooms/refugerestrooms/compare/develop...btyy77c:dockerAlpine

Noteworthy techniques in this branch:


See also this branch I made: https://github.com/RefugeRestrooms/refugerestrooms/compare/develop...DeeDeeG:cleanup-Dockerfile

Noteworthy techniques in this branch:

DeeDeeG commented 6 years ago

In terms of simply merging this PR, I would say we can remove the rm /var/lib/apt/lists/* command, or move it to after any apt-getcommands. But everything else is pretty much good within the scope of this pull request.

For a fuller discussion of optimizing the Dockerfile, I opened #509.

darpr commented 6 years ago

Thanks @DeeDeeG !