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

Enhance docker bootstrap to also build webpack, email images #757

Merged
merged 1 commit into from
Sep 30, 2020

Conversation

solebared
Copy link
Collaborator

Why

Currently, bin/dev/bootstrap only builds the app image and when we run bin/dev/serve or other dependent command, the webpack and email images are built. While there's value in building these 'just-in-time', i'd prefer bootstrap build all three images.

This will surface any issues when a user first sets up their docker env and subsequent commands will fire up more quickly.

Pre-Merge Checklist

  • All new features have been described in the pull request
  • New features have been documented, and the code is understandable and well commented
  • All outstanding questions and concerns have been resolved
  • Any next steps that seem like good ideas have been created as issues for future discussion & implementation

Testing

Tested locally but would love another set of eyes to verify.

Outstanding Questions, Concerns and Other Notes

  • The dc function in bin/dev/bootstrap uses the $@ variable which seems to work but is new to me; is this an approprate use? Are there any gotcha's with it?

Currently, `bin/dev/bootstrap` only builds the app image and when we run
`bin/dev/serve` or other dependent command, the webpack and email images
are built. While there's value in building these 'just-in-time', i'd
prefer that bootstrap builds all three images.

This will surface any issues when a user first sets up their docker env
and subsequent commands will fire up much quicker.
Copy link
Collaborator

@indiebrain indiebrain left a comment

Choose a reason for hiding this comment

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

Tested locally but would love another set of eyes to verify.

LGTM, also ran through a local bin/dev/bootstrap and poked around the application, locally, myself as a "Yup, looks good over here too!". So I say 📦 🚀 🌔

I'll tease it here; I'm working on a change set that would allow the app and webpacker services share their cache layers; keeping the build process from doing the same things more than once - the "secret-sauce" utilizes Docker multi-stage builds. It's a non-trivial change to how the development Dockerfile currently works - will definitely be asking that folks have a run with it before I'd consider it "merge-able". I'll hopefully have a branch up in the next few days.

run --rm \
app bin/rails db:reset RAILS_ENV="${environment}"
function dc() {
docker-compose -f docker/development/docker-compose.yml run --rm "$@"
Copy link
Collaborator

Choose a reason for hiding this comment

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

The dc function in bin/dev/bootstrap uses the $@ variable which seems to work but is new to me; is this an approprate use? Are there any gotcha's with it?

The $@ basically means "All arguments expanded positionally" and seems entirely appropriate here.

The only gotcha I can think of is sometimes $@ can expand in unexpected ways depending on if it's contained in a set of "s - I don't remember all the rules on when / why this happens, but something about word boundaries and how IFS is set comes to mind. I don't think that should pose a problem here, though. Figured I'd drop a few of my go-to bash resources as they're a much more authoritative source than my old and forgetful brain:

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Thanks for looking over this, and the bash links!

@solebared solebared merged commit d625bf2 into main Sep 30, 2020
@solebared solebared deleted the enhance-docker-bootstrap branch September 30, 2020 05:51
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.

3 participants