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

Update app setup to use docker for postgres/redis #1720

Closed

Conversation

Drew-Goddyn
Copy link

@Drew-Goddyn Drew-Goddyn commented Oct 22, 2024

related: #464
related: #1620

What are you doing?

Started playing around with bullet train
(🙌) and didn't like having to
install bare-metal postgres / redis.

Personally, I tend to avoid from running background services directly as I often forget about them, and also have conflicts across various repos using different versions. Also, lets promote developers dipping their toes into Docker as it's a critical tool to learn. To that end, I've updated the setup/dev scripts to expect and use docker for those services.

Specific changes:

  • Separated the configure/setup scripts into their own folders
  • Use a shared utils.rb script for configure/setup/dev
  • Added a check_docker.rb to script/setup to check the user has Docker installed and running
  • Added various helper functions to utils.rb for interacting with docker containers
  • Change setup/dev to start and stop containers as needed so the system is unchanged when running the app
  • Re-wrote the bin/dev script to be Ruby since we seem to have decided to go all in on Ruby scripts for /bin

What does all this actually do?

Generally configure/setup/dev remain mostly unchanged.

  • bin/setup now checks that docker is installed and running (give option to start it if not)
  • bin/setup now starts postgres to run migrations and stops it after
  • bin/setup doesn't check if postgres / redis are installed as they are container services now
  • bin/dev takes care of starting/stopping services as needed when the app boots

Testing

I tested on Mac os specifcally:

clone repo as normal

  • bin/configure works as expected
  • bin/setup works as expected
  • bin/dev works as expected
  • ctrl + c out of overmind stops running containers

@Drew-Goddyn Drew-Goddyn force-pushed the upstream-docker branch 3 times, most recently from 42b037c to 4b0f15d Compare October 22, 2024 16:51

def system!(*args)
system(*args) || abort("\n== Command #{args} failed ==")
end
Copy link
Author

Choose a reason for hiding this comment

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

Everything after this in utils.rb is new

system(*args) || abort("\n== Command #{args} failed ==")
end

def overmind_quit
Copy link
Author

Choose a reason for hiding this comment

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

Helps recover from overmind dirty exiting and leaving the .sock file round. Should avoid getting that error message: overmind: listen unix /Users/joekur/dev/reverb/.overmind.sock: bind: address already in use

volumes:
- postgres-data:/var/lib/postgresql/data
environment:
POSTGRES_HOST_AUTH_METHOD: trust
Copy link
Author

Choose a reason for hiding this comment

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

should we default to a basic password?

@Drew-Goddyn Drew-Goddyn marked this pull request as ready for review October 22, 2024 17:39
@Drew-Goddyn Drew-Goddyn changed the title Upstream docker Update app setup to use docker for postgres/redis Oct 22, 2024
@Drew-Goddyn Drew-Goddyn force-pushed the upstream-docker branch 2 times, most recently from 7cc31d8 to de6e4f6 Compare October 22, 2024 17:52
@jagthedrummer
Copy link
Contributor

@Drew-Goddyn, thanks for the PR! Unfortunately this does way too much for us to be able to accept it as is.

If you can split it into several smaller PRs then we'll be able to consider each logical change on their own merits. We try to stick with small focused PRs so that they're easier to review by members of the core team, and so that downstream developers have an easier time of seeing exactly what's changed from version to version. I hope you can see where we're coming from on that point.

Other than the issue of the giant PR, a lot of these look like reasonable changes, but some of these we probably won't be able to accept at all.

Rewriting bin/dev in ruby seems like a good call and is also something that we'd be very likely to accept in a dedicated PR.

Using a single shared utils.rb between configure and setup would be something we could probably accept in a dedicated PR that does only that.

But reorganizing bin so that things live in bin/scripts/setup instead of bin/setup-scripts seems unnecessary and will potentially cause merge conflicts and/or confusion when people go to update, so we probably wouldn't accept that, even in a stand-alone PR. If you can make a solid case for why it's necessary or desirable we'd certainly give your reasons some consideration, but it would need to be something concrete and tangible. It seems like just a matter of personal organizational preference. Since people directly pull changes from the starter repo into their own projects we have to exercise more discretion than your average project when it comes to cosmetic reorganization.

All the stuff about running services in Docker is something that's less clear cut. There's been some discussion around this sort of thing in the past (as you noticed with the linked PRs) and we're not sure that this is something we're prepared to maintain and support. Speaking only for myself, I don't really like running those kinds of services in Docker because I find it to generally be slower and more "fiddly" than just running native services. If you can make a much smaller PR for only the Docker stuff then we'd be able to give your particular implementation of the idea a proper evaluation. I think we'd be more likely to accept it if it were something that people could opt into. Making a wholesale change that amounts to "this is how we do it now whether you like it or not" is going to be a much harder sell.

I'm going to close this since we can't accept it in the current form, but please do open some smaller PRs for things that you'd like to see changed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants