FloChehab commented on pull request #12010: URL: https://github.com/apache/airflow/pull/12010#issuecomment-757365144
> Hi everyone, sorry I just saw this change, and I have a few questions: > > * in `values.yaml`, shouldn't this new item `brokerUrl` be added under `redis` rather than `data`? `data` is for database based on what's under it earlier (metadatabase and broker backend db, both are actually database). > > * the name `brokerUrl` itself is misleading, because broker can be something else, like RabbitMQ. > > > Please let me know your thoughts @mik-laj @kaxil @potiuk . In my view, some follow-up changes will be needed. Please let me know if I have misunderstood anything. Cheers. I had several headaches on the values.yaml file while adding this feature. * For the naming, I tried to stay consistent with the current names used in the chart and airflow (https://github.com/apache/airflow/pull/12164). I guess what could be clearer is that it can be something else than redis. * For the location, I also thought about putting it under the redis value, but then you imply that you can only use redis and the logic on whether or not to spawn a redis instance with the chart would rely on whether or not the `redis.brokerUrl` value is set which might not be super obvious; so I went for `redis.enabled` and added the brokerUrl under `data` as it seemed "appropriate". Here was my thought process. Definitely open to evolution (in general, I feel that the values.yaml would need a bit of reorganization). ---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected]
