search

hanami / cli

Hanami command line
MIT License
27 stars 28 forks source link

Generate db files during hanami new #180

Closed cllns closed 2 months ago

cllns commented 3 months ago

Resolves #147.

A couple things.

  1. I didn't add any DATABASE_URL to .env yet. I like the pattern of putting those into .env.development and .env.test. I know there was some discussion about an automatic translation from a development DATABASE_URL to a test one but not sure where we landed on that. When deploying to production, it's a feature that it won't deploy without DATABASE_URL set on the production server, so the development one isn't used accidentally.
  2. I added some constants without private_constant so I can re-use them. I don't really like where they are now, any ideas of a better place to put them?
  3. I removed passing along the rest of the kwargs to the context, due to some syntax errors on 3.0 and 3.1 since I broke up the method calls (I think). Not sure if this is a problem for extensibility?
  4. I have the base repo inherit from Hanami::DB::Repo and added a FIXME for when rename that to Hanami::Repo
  5. Do we want to add ROM transaction support in our base Operation? I think so.
cllns commented 3 months ago

Addressed all your comments.

Should we also add default DATABASE_URL's to .env.development and .env.test?

timriley commented 2 months ago

Addressed all your comments.

Thanks @cllns, those changes look good!

Should we also add default DATABASE_URL's to .env.development and .env.test?

I reckon we should actually add it directly to a generated .env file!

Reasons:

Together, this means a single .env with a single DATABASE_URL will be enough to cover all local development use cases! This feels a lot neater than having to generate two files and requiring new users to remember that the DATABASE_URL needs to be kept in sync across both.

Now, as the dotenv docs point out, if you check in a .env, then it's imperative you use the counterpart files that are not checked in (.env.development.local, etc.) to store any sensitive values.

To this end, I think we should:

  1. Generate a .env with the DATABASE_URL in it
  2. Put a comment at the top of .env making it clear that sensitive values should go in the other files
  3. Change our .gitignore so that .env is no longer ignored, and add .env.local and .env.*.local in its place

How does this sound to you, @cllns?

Once this is done I reckon we should finally be at the point of a ready-to-go database-backed app after hanami new! 🎉

timriley commented 2 months ago

p.s. I re-ran the failing tests and they turned green (due to my making the latest dry-configurable gem release), so I think this should come good after the next push. It might need a rebase to get the latest GitHub actions workflow from the main branch though (with 3.0 removed).

cllns commented 2 months ago

Addressed all the comments and we're green again.

I also added include Dry::Operation::Extensions::ROM into the base Operation so users will be able to use transaction (unless they --skip-db)

cllns commented 2 months ago

Right now the app/db/repo.rb is generated as AppName::Repo, not AppName::DB::Repo. I feel like that's fine but just want to confirm that we're all OK with that, since it might be surprising. We could also alias it so it exists in both places, but that's a little strange too.

cllns commented 2 months ago

Sweet. All good now, with the base repo going into app/db/repo.rb as MyApp::DB::Repo

timriley commented 2 months ago

Thanks @cllns! I'm going to merge this now, since I want to start updating our getting started guide, and being able to generate a new app will help with that :)