-
-
Notifications
You must be signed in to change notification settings - Fork 2.9k
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
Remove CELERY_BROKER_URL in favor of REDIS_URL #4861
base: master
Are you sure you want to change the base?
Remove CELERY_BROKER_URL in favor of REDIS_URL #4861
Conversation
557856b
to
641d833
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good at first glance - could you please merge/rebase the latest master to do some final chackes?
641d833
to
4709a30
Compare
# Optional: set broker URL if using Celery | ||
$ export CELERY_BROKER_URL=redis://localhost:6379/0 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should this tell people to set the REDIS_URL
instead?
{%- if cookiecutter.use_celery == "y" %} | ||
CELERY_BROKER_URL="" \ | ||
{%- endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think that was required because the env variable has no default in the settings, which is still the case with REDIS_URL
{% if cookiecutter.use_celery == 'y' %} | ||
# N.B. If only .env files supported variable expansion... | ||
export CELERY_BROKER_URL="${REDIS_URL}" | ||
{% endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🎉
{%- if cookiecutter.use_celery == 'y' %} | ||
os.environ["CELERY_BROKER_URL"] = os.getenv("REDIS_URL", "redis://redis:6379") | ||
{%- endif %} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This one might be still be required too...
As discussed in #4811 this is how it looks like to remove
CELERY_BROKER_URL
.Considering the amount of deletions, this looks like the right path.