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

feat(database config): use DATABASE_URL in .env for all environments #610

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

thoran
Copy link

@thoran thoran commented Feb 15, 2021

This change allows the use of a single line in .env as per below:

DATABASE_URL=postgres://lucky:developer@localhost:5432/<app_name>

and not have tests trash my development database, whilst still working as is for production.

@jwoertink
Copy link
Member

Hey! Thanks for the PR. I'm trying to understand what you mean by "not have tests trash my development database"? When you run your specs, you're saying that your dev db is truncated?

I haven't seen that with any of my apps. Do you have your specs configured differently? this line sets your env to test which should make this line reference only a test db. I'm wondering if there's some other error, or maybe documentation we might have missed somewhere 🤔

@thoran
Copy link
Author

thoran commented Feb 17, 2021

If I use DATABASE_URL in .env and only DATABASE_URL, then Avram::Credentials.parse?(ENV["DATABASE_URL"]?) doesn't switch for environments, because database_name is used only when the other side of the alternation is being used, such that it pulls in DB_HOST, DB_PORT, etc.

@jwoertink
Copy link
Member

Oh, I see what you're saying. I looked at what my projects are doing for this since we only use .env files, and I noticed that I added this to all of them

# spec/spec_helper.cr
require "dotenv"

Dotenv.load(".env.test")

I definitely agree that this is an issue, but the one thing I'm not too sure of is this basically ignores whatever database name you set in your .env. For me, most of my Lucky apps were ported from older projects where the DB names are different, so they don't always follow the "appname_env" convention.

What are your thoughts on using a .env.test file in this case instead? If you're ok with that, then I think what we can do is update this file to require dotenv and load that file. Then we need to have apps generated with the .env.test file in here, and finally, we could clean up the spec helper by moving these two bits in to the new .env.test file since they would be loaded already.

@thoran
Copy link
Author

thoran commented Feb 22, 2021

Firstly, I should mention that I never thought that the proposed PR would be the final solution even if correct insofar as functionality is concerned, but rather is merely my less ideal solution which I'm using presently to make it work the way that I'd like. I presumed to think that the idea might fly, but that the implementation would be rejected because it's pretty ugly... One of the key aspects of the design ethos of Elixir at least insofar as how it is used is concerned, which which I get the sense is present also in Lucky and which is what attracted me to Lucky rather than using a Rails-clone in Crystal, is to do with obviousness or lack of magic perhaps. Elixir has an anti-DRY (or DRO as I like to call it) disposition because of what I presume to think is our shared experience of trawling through Rails or Rspec, but particularly Rails, seeing layers and layers of abstraction which in reality often makes things harder, not easier. So, your proposed solution works to the extent that I think it contains a little less magic and is therefore more obvious. In fact, I'm pretty sure that I had a similar set up in a Rails project not so long ago... I was wondering if it would it be even less magic and more obvious to have one .env file per environment, possibly with sensible fall backs? I think it would be clearer still if .env.test wasn't a special case. Would .env.development, .env.test, and even a .env.production be too much? My only concern at that point would be making a dotfile mess in the app root. A sensible fallback might be to infer "<%= crystal_project_name %>_#{Lucky::Env.name}" with the credentials in either of .env.test or .env.development depending on what's available, or even less magic and more clarity to just fail. That's possibly a little vague, so I'll rework the PR...

@thoran
Copy link
Author

thoran commented Feb 22, 2021

I'm not sold on what accrues from the additional commit, but rather am just playing with possible solutions, and it may not even work. Will need to spend more time on it later...

@jwoertink
Copy link
Member

obviousness or lack of magic

Exactly. I do love Rails, but sometimes there's just a bit too much magic. With Lucky, we want things to be a bit more obvious and straight forward, even if it means just a little more typing, or a few more files. A little magic here and there is ok, but there has to be a nice balance.

I don't think there's any major rush on this, so we can take some time to come up with a clean solution. If you happen to see what other frameworks are doing in this case (outside of rails or phoenix), feel free to drop some other ideas in here.

Thanks for your help on this!

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