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

Make the purpose of db/seeds.rb clearer through a better description and sample code. #47222

Merged
merged 1 commit into from Feb 15, 2023

Conversation

rafaelsales
Copy link
Contributor

@rafaelsales rafaelsales commented Feb 2, 2023

Motivation / Background

Many developers assume that db/seeds.rb is a good place to put sample development data or test fixtures. As most rails projects evolve with time, devs realize that there's a reason this file is simply called "seeds.rb" -- nothing indicates that it's exclusive to an environment, which means that it should be good to run in every environment.

Detail

This Pull Request changes the db/seeds.rb template file to make the purpose of that file clearer.

Additional information

Checklist

Before submitting the PR make sure the following are checked:

  • This Pull Request is related to one change. Changes that are unrelated should be opened in separate PRs.
  • Commit message has a detailed description of what changed and why. If this PR fixes a related issue include it in the commit message. Ex: [Fix #issue-number]
  • Tests are added or updated if you fix a bug or add a feature.
  • CHANGELOG files are updated for the changed libraries if there is a behavior change or additional feature. Minor bug fixes and documentation changes should not be included.

@rails-bot rails-bot bot added the railties label Feb 2, 2023
@rafaelsales rafaelsales changed the title Update seeds.rb.tt Make the purpose of db/seeds.rb clearer through a better description and sample code. Feb 2, 2023
@matthewd
Copy link
Member

matthewd commented Feb 2, 2023

Hmm... I've seen seeds used to good effect for [consciously idempotent] definition of development sample data.

(And on the other hand, there are few first-look experiences as painful as cloning a nontrivial new-to-me application, starting it up, and staring at a completely empty UI.)

@rafaelsales
Copy link
Contributor Author

rafaelsales commented Feb 2, 2023

@matthewd It is possible to make good use through environment checks, but that should be obvious once you know the correct purpose of db seeds. The point is: it should be ok to run db:setup and db:seed in production.

In my opinion, the current description is almost there when it says "to seed the database with its default values" -- it's clearly not environment-specific. But, it would be better to emphasize that just a bit more, which is what I'm trying to do here.

An example of possible acceptable use IMO.

zoo_foo_type = FooType.find_or_create_by(name: 'zoo')
boo_foo_type = FooType.find_or_create_by(name: 'boo')

if Rails.env.development?
   Foo.find_or_create_by(slug: 'bar', type: zoo_foo_type)
end

Knowing that it should be ok to run seed on any environment will make the dev realize that if they want env-specific logic, they need to run that section with a conditional.

@rafaelsales
Copy link
Contributor Author

Perhaps a cleaner and flexible option would be to offer seed files per environment:

# Always loaded:
db/seed/shared.rb

# Loaded depending on Rails.env
db/seed/development.rb
db/seed/test.rb
db/seed/production.rb 

@eileencodes
Copy link
Member

I do not think people should be running seeds in production except for maybe in the very beginning of application development. Even then, I'd probably create separate demo data if I really wanted seed-like data in production. I think encouraging their use in production without sufficient warning is a recipe for a production incident and lost data. IMO seeds are only for dev environments, I would never use them elsewhere.

@zzak
Copy link
Member

zzak commented Feb 13, 2023

Agree with Eileen here, and if they are split by environment (e.g. db/seed/<env>.rb) what is the difference between seeds and fixtures at that point (besides one being written in yaml)?

I think the more conservative approach is to raise a warning when trying to run seeds in production mode.

@rafaelsales
Copy link
Contributor Author

@eileencodes @zzak Read DHH's comment when seeds was introduced:

Added db/seeds.rb as a default file for storing seed data for the database. Can be loaded with rake db:seed (or created alongside the db with db:setup). (This is also known as the "Stop Putting Gawd Damn Seed Data In Your Migrations" feature) [DHH]
Source: 4932f7b#diff-0c54156721edc0b89fe7a5609da124f353684aeae0f75416e7a2d63c4af30520R3

Other comments on that commit worth reading:
image
Source: 4932f7b#commitcomment-21127

@dhh perhaps you can chime in here to help us with some verbiage that clarifies the purpose of seeds.rb.

Its purpose was not fake or test data. Many applications require some records to exist in every environment for its basic functionality. The example in this PR is movie genres. Some other cases that I've seen:

  • OAuth Application records for the web frontend and mobile apps
  • An admin record
  • Initial/minimum/required roles for role-based access control.
  • Enumeration records -- yes, some companies need enum values to exist as a relation on the database so that data engineering and analytics can see possible values without trying to read code that they're not expert on.

@dhh
Copy link
Member

dhh commented Feb 13, 2023

db/seeds.rb is indeed intended to work for production data. When the app requires the database to have some basic seeding in place. I do agree with Elaine that this seeding is meant for the INITIAL setup of the database, and we should either make that clear, or make it clear that seeds.rb needs to be idempotent.

@rafaelsales
Copy link
Contributor Author

rafaelsales commented Feb 13, 2023

@dhh If the seeds.rb isn't idempotent, then you would need to write migrations to add more seed data as the application evolves - invalidating the initial purpose of seeds.rb. For that reason, I've always written idempotent seeds.rb -- and also the reason I used find_or_create_by in the new example of seeds.rb

is meant for the INITIAL setup of the database

If that was the case, I'm afraid that seeds.rb would become a legacy file too early in the development of an app.

@zzak
Copy link
Member

zzak commented Feb 14, 2023

@rafaelsales Can you squash your commits please?

@rafaelsales
Copy link
Contributor Author

@zzak done.

@zzak zzak merged commit 0d72275 into rails:main Feb 15, 2023
@p8
Copy link
Member

p8 commented Feb 15, 2023

Hmm, I think this got merged a bit too quickly.

I think the name seed still implies an initial task that should be run for setting up a blank application.
This is also how the guides describe it:

To add initial data after a database is created, Rails has a built-in 'seeds' feature that speeds up the process. This is especially useful when reloading the database frequently in development and test environments.
...
This is generally a much cleaner way to set up the database of a blank application.

https://guides.rubyonrails.org/active_record_migrations.html#migrations-and-seed-data

@rafaelsales Do you always run the seeds automatically for each deployment?
What happens if someone doesn't know this and changes the seeds to remove data for a clean development app?
What happens if you want to rollback this change?
Do you have tests for your seeds?

I'm also afraid someone might run rails db:seed by accident on production, or worse rails db:seeds:replant (not sure if that is even possible).

The example of movie genres doesn't imply a seed file that could be run anytime on production.
I don't think anyone would update the allowed movie genres on production by editing the seeds file. What happens if a genre should be deleted? Do you delete it in the seed file?

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.

None yet

7 participants