-
Notifications
You must be signed in to change notification settings - Fork 17
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
feat(background_processor): include redis docker configuration #329
Conversation
d126a3f
to
69803f2
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.
Te puse un mini comentario pero se ve 💯 !
No sé si había tests para esta receta, si no igual no los crearía, creo que hay que repensar el approach de los tests y no seguiría agregando nuevos, pero si ya existe, sí lo modificaría para reflejar los cambios.
Otra cosa, te tinca pedirle un ojo a alguien más por si le ven algo más (por si a alguien no le gusta lo de redis_url por ej).
69803f2
to
24db3da
Compare
7c50625
to
6a3df14
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.
lo veo perfect, pero quizás sería bueno separar la configuración de redis de sidekiq? Podríamos querer usar redis sin sidekiq sobre todo ahora que anda volando la idea de tener como default a delayed job.
También lo conversamos con @rjherrera pero nos pareció que eso iría más en otro PR que se encargue de implementar redis como recipe por ejemplo. De momento como solo se ocupa acá no tiene tanto valor el separarlo. |
Adds the necessary configuration to docker-compose.yml and the corresponding env vars.
Also changes the
redis.yml
config file to force the definition of theREDIS_URL
env var in development in an effort to make it more consistent with production.closes #328