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

Load Redis client from existing environment variable #914

Merged
merged 2 commits into from
Jul 3, 2018

Conversation

lucassz
Copy link
Contributor

@lucassz lucassz commented Jul 2, 2018

Load Redis client from existing environment variable instead of hardcoding an unchangeable hostname for it. Necessary for #913 in an auxiliary way, and seems like a good practice. It's not totally clear to me what the distinction between CELERY_BROKER_URL and CELERY_RESULT_BACKEND (which we set to the same value in practice) is, so do let me know if it should be the other one.

@hdoupe
Copy link
Collaborator

hdoupe commented Jul 2, 2018

@lucassz this looks good to me. Thanks for doing the update.

It's not totally clear to me what the distinction between CELERY_BROKER_URL and CELERY_RESULT_BACKEND (which we set to the same value in practice) is, so do let me know if it should be the other one.

Some where in the celery docs it discusses ways to combine redis and rabbitmq or something like that. However, for us, just using redis gets the job done.

@lucassz
Copy link
Contributor Author

lucassz commented Jul 2, 2018

@hdoupe Thanks for the clarification.

Added a semi-related update to DOCKER.md.

@hdoupe
Copy link
Collaborator

hdoupe commented Jul 2, 2018

@lucassz is #914 ready to merge?

@lucassz
Copy link
Contributor Author

lucassz commented Jul 2, 2018

@hdoupe Yes, looks good to me.

@hdoupe
Copy link
Collaborator

hdoupe commented Jul 3, 2018

Thanks for the contribution @lucassz!

@hdoupe hdoupe merged commit 8bd9cff into ospc-org:master Jul 3, 2018
@lucassz lucassz mentioned this pull request Jul 5, 2018
hdoupe added a commit that referenced this pull request Jul 5, 2018
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