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

Ignore un-existence of database.yml #460

Closed
wants to merge 3 commits into from

Conversation

maxmeyer
Copy link
Contributor

@maxmeyer maxmeyer commented Sep 14, 2017

  • This makes unicorn happy if either DATABASE_URL is set or the config/database.yml is available
  • Silence output of ActiveRecord - depending on LOG_LEVEL

@gabrielpoca
Copy link
Contributor

@maxmeyer thank you for the contribution. The code seems to be breaking a rule in rubocop, could you take care of it before I merge?

@maxmeyer
Copy link
Contributor Author

@gabrielpoca Yes, of course. Done.

@maxmeyer
Copy link
Contributor Author

@gabrielpoca Ok?

@gabrielpoca
Copy link
Contributor

Thank you @maxmeyer! I only now got the time to look at this change more carefully, if the config/database.yml already uses <%= ENV["DATABASE_URL"] %> in production, what is the scenario in which you need this change?

@maxmeyer
Copy link
Contributor Author

maxmeyer commented Oct 2, 2017

The problem is this: I removed the file in my container, as active_record can be used with DATABASE_URL defined, but starting stringer fails because File.load fails because the file does not exist.

@gabrielpoca
Copy link
Contributor

@maxmeyer I hope you don't mind me insisting, but it's not clear for me yet. If config/database.yml is already using DATABASE_URL, why are you removing the file?

@maxmeyer
Copy link
Contributor Author

@gabrielpoca Because I was unaware of this and overhead it in your comment.

Thinking more about it, the approach of having this indirection with config/database.yml seems to be less flexible as I'm not able to run a container in development environment configured via DATABASE_URL, correct?

@gabrielpoca
Copy link
Contributor

@maxmeyer yes, you are right, it's less flexible. It does work in production, but in development you would have to replace the file. If no one has complained about it not working when running in development, do you still think we should merge this?

@maxmeyer
Copy link
Contributor Author

@gabrielpoca I think yes, normally I try to build my docker images that they work in all environments the same. This https://12factor.net/config may give some more info why it makes sense to configure an application via environment variables.

@pascalw You added the docker support. What do you think? Shall the image work in all environments -
prod, staging, ... the same?

@pascalw
Copy link
Contributor

pascalw commented Oct 28, 2017

@maxmeyer @gabrielpoca I do think ideally the Docker image works in every environment.

However I'm not convinced the changes in this PR are required to achieve that. For example I'm able to run Stringer with RACK_ENV=development and specifying a DB url like this:

docker run --rm -it -e DATABASE_URL="sqlite3:':memory:'" -e RACK_ENV=development -p 8080:8080 mdswanson/stringer

Afaik Activerecord prioritizes DATABASE_URL over config.yml.

@bitboxer
Copy link
Contributor

@mockdeep I think this one can be closed

@mockdeep
Copy link
Collaborator

Sure, sounds good. @maxmeyer if you have any other thoughts on this, happy to hear them.

@mockdeep mockdeep closed this Apr 19, 2022
@maxmeyer
Copy link
Contributor Author

This is fine for me. I'm not using Stringer anymore.

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.

6 participants