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 docker-compose to run seeder #55

Merged
merged 2 commits into from
Mar 29, 2023
Merged

Conversation

divagueame
Copy link
Contributor

These are some changes to docker-compose so that the dev environment is seeded. The local DB is reset each time, so that might not be ideal but it might be a good a starting point to build upon.

Copy link
Member

@lucianghinda lucianghinda left a comment

Choose a reason for hiding this comment

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

Hi @divagueame

Thank you for submitting this PR. It looks good in general.
I have one thing that I think it should be changed. I wrote how I see the change in my comment but please feel free to approach it in the way if you think there might be a better solution.

The main thing is that I would not want to recreate the development DB every time the developer is executing docker compose up but make it optional.

@@ -24,7 +24,11 @@ services:
- .:/app
- node_modules:/app/node_modules
- bundle:/usr/local/bundle
command: ["rails", "server", "-b", "0.0.0.0"]
command: bash -c "bundle exec rake db:drop &&
Copy link
Member

Choose a reason for hiding this comment

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

nipitck suggestion

I think if we add ,this line it will mean that everytime a developer starts the docker it will drop the database and recreate it.
In my experience, rarely is this needed, and if someone works over multiple days on a feature could mean it will delete their manual testing data.

Thus I recommend the following:

  1. Make a script under ./bin/recreate_db which will execute two commands:
#!/bin/bash

iif [[ "${RAILS_ENV}" == "development" || "${RAILS_ENV}" == "test" ]]; then
  if [[ "${RECREATE_DB}" = true ]]; then
    bundle exec rails db:drop
    bundle exec rails db:prepare
  else
    bundle exec db:migrate
    echo "If you want to drop the database and recreate it set env variable RECREATE_DB to \"true\""
  fi
else
  echo "Unsafe execution of dangerous command in other RAILS_ENV than test or development"
fi

This script will only recreate the DB if RAILS_ENV is set to development or test and if an env var called RECREATE_DB is set to true.

  1. Add a variable to the environment part of app service that will read a variable from host environment in docker
    I think something like this should work:
environment:
  DATABASE_URL: postgres://postgres:password@db:5432/rubyandrails_info_development
+  RECREATE_DB: ${RECREATE_DB}

I did not test this so there might be some bugs, but I think the general direction could be this.

PS: db:prepare is equivalent with doing db:create && db:migrate && db:seed. See db:prepare and then DatabaseTasks.prepare_all

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Hi Lucian, thanks a lot for your suggestions. They were indeed very helpful.
I changed it so that the default won't drop the database every time.
There is two options in case somebody wants to drop the database, 1) just like you said, setting a environment variable ${RECREATE_DB} to true, 2) access the container, and run "rails db:drop". Both methods worked on my machine. I added that to the Readme as well:

Access the shell in the app container

docker-compose run --rm --entrypoint sh app

Also changed the seeder to only run if there are no entries of the model.

Copy link
Member

Choose a reason for hiding this comment

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

Great work @divagueame

I will merge this now

@lucianghinda lucianghinda merged commit a247b22 into ShortRuby:main Mar 29, 2023
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.

None yet

2 participants