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

Export REDIS_URL when enabling/starting Redis #8

Closed
wants to merge 1 commit into from
Closed

Export REDIS_URL when enabling/starting Redis #8

wants to merge 1 commit into from

Conversation

Flink
Copy link
Contributor

@Flink Flink commented Mar 11, 2015

Since this plugin is using docker links, it’s really easy to export a complete URL for Redis that will not change too often. And REDIS_URL seems to be used as a standard to set connection to the Redis server in various libraries (at least in the Ruby world ;)).

@msjyoo
Copy link
Owner

msjyoo commented Mar 16, 2015

I see :) Will merge soon!

@Flink
Copy link
Contributor Author

Flink commented Mar 16, 2015

Great :)

@msjyoo
Copy link
Owner

msjyoo commented Mar 16, 2015

But is it not possible (in the Ruby world) to craft the REDIS_URL variable inside the bootstrapper / application logic? I think its best if I just exposed default Docker Link variables and let the applications suit themselves to it, to keep the plugin simple and as non language/library specific as possible.

Or just use the provided env variable names by specifying the host and port explicitly to the Redis connector by getting the env variables?

@Flink
Copy link
Contributor Author

Flink commented Mar 16, 2015

Yes it’s possible just it requires no configuration if REDIS_URL is set: https://github.com/redis/redis-rb :)

@Flink
Copy link
Contributor Author

Flink commented Mar 16, 2015

I’ll check if other redis libraries (for other languages) uses REDIS_URL or not

@Flink
Copy link
Contributor Author

Flink commented Mar 16, 2015

Well it seems REDIS_URL is quite ruby oriented but to have a database url is much more heroku-like. Python package seems to support it since about 3 years: redis/redis-py#246

@msjyoo
Copy link
Owner

msjyoo commented Mar 17, 2015

Sounds good, but I'll just move the config to start instead of enable since resetting of that variable is expected on a rebuild.

@msjyoo
Copy link
Owner

msjyoo commented Mar 17, 2015

Hello, please look at #10. I've moved the config setting to :start so it gets reset on rebuilds. Apart from that, looks good.

@msjyoo msjyoo closed this Mar 17, 2015
msjyoo added a commit that referenced this pull request Mar 17, 2015
Export REDIS_URL when enabling/starting Redis. Fixes #8
msjyoo added a commit that referenced this pull request Mar 17, 2015
@Flink
Copy link
Contributor Author

Flink commented Mar 17, 2015

Ok thanks :) I moved the variable from start to enable because start is called in pre-deploy I think. And when you set an ENV variable in dokku, it automatically restarts the app so when deploying your application it restarts twice (IIRC).

@msjyoo
Copy link
Owner

msjyoo commented Mar 17, 2015

Yeah, I'm fixing it right now by custom implementing the config set without the restart. Will let you know when that's committed.

@Flink
Copy link
Contributor Author

Flink commented Mar 17, 2015

Ok great!

@msjyoo
Copy link
Owner

msjyoo commented Mar 17, 2015

@Flink I haven't tested this myself but faacfce should fix the issue. Please let me know if it works or not. I've tested the libconfig file but not the plugin itself on dokku yet

@msjyoo
Copy link
Owner

msjyoo commented Mar 17, 2015

Nevermind. What I have on master won't work. I had to separate pre-release and pre-deploy so that env variables are set at pre-release but redis is started at pre-deploy (for it to start on reboots.) Just tested it like this and it works. I will clean up and commit this to master.

@Flink
Copy link
Contributor Author

Flink commented Mar 17, 2015

Ok thank you :)

@msjyoo
Copy link
Owner

msjyoo commented Mar 18, 2015

Done. I should probably start tagging commits...

@Flink
Copy link
Contributor Author

Flink commented Mar 18, 2015

Yes I think you’re right :)

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.

None yet

2 participants