Visitar URL original
Ignore un-existence of database.yml by maxmeyer · Pull Request #460 · stringer-rss/stringer · GitHub
Skip to content

Ignore un-existence of database.yml - #460

Closed
maxmeyer wants to merge 3 commits into
stringer-rss:mainfrom
maxmeyer:patch-1
Closed

maxmeyer wants to merge 3 commits into
stringer-rss:mainfrom
maxmeyer:patch-1

Conversation

@maxmeyer

@maxmeyer maxmeyer commented Sep 14, 2017 •

Copy link
Copy Markdown
Contributor
  • 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
Copy Markdown
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
Copy Markdown
Contributor Author

@gabrielpoca Yes, of course. Done.

@maxmeyer

Copy link
Copy Markdown
Contributor Author

@gabrielpoca Ok?

@gabrielpoca

Copy link
Copy Markdown
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

maxmeyer commented Oct 2, 2017

Copy link
Copy Markdown
Contributor Author

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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

pascalw commented Oct 28, 2017

Copy link
Copy Markdown
Contributor

@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
Copy Markdown
Contributor

@mockdeep I think this one can be closed

@mockdeep

Copy link
Copy Markdown
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
Copy Markdown
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