Skip to content

Conversation

@bibby
Copy link

@bibby bibby commented Mar 31, 2015

removed credentials in favor of dockerfile ENV vars with defaults. docker-compose example file.

lib/config.js Outdated
Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

should be env.USE_SSL!

@bibby
Copy link
Author

bibby commented Apr 1, 2015

I recognize now that my changes are not friendly to Vagrant users. Sorry for that. Hopefully we can turn this into a happy medium.

@matejkramny
Copy link
Contributor

Hmm. Maybe change this PR so the config file is still supported, while keeping the names of the env variables as you've got them.

Eg config file would be entirely optional if environment variables are present.

That seems like a good compromise between both worlds. What do you think?

@bibby
Copy link
Author

bibby commented Apr 1, 2015

I honestly don't know how Vagrant likes to configure its applications, but I should put the config file back.
Recognizing Docker could be as sneaky/easy as:

ENV IS_DOCKER 1

..and swapping behavior based on that.

@matejkramny
Copy link
Contributor

I do not agree with keeping all default env variables inside the dockerfile because the app is also deployed outside of docker.

Running the app outside of docker therefore will not work.

In order for to merge this PR:

@bibby
Copy link
Author

bibby commented Apr 2, 2015

That's ok, bro. I took a docker pattern and over-extended it.

@bibby bibby closed this Apr 2, 2015
@matejkramny
Copy link
Contributor

no probs. Thanks for your time.

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