leorochael commented on issue #4631: Fix flower broker configuration URL: https://github.com/apache/incubator-superset/pull/4631#issuecomment-374687104 > In terms of enforcing that the BROKER_URL is present isn't this required by the Celery worker? It seems that the convention is that it's assumed that these configuration variables exist and thus I'm uncertain whether the check is required. Before this PR, the behaviour of `superset flower` on a missing broker config was to invoke: celery flower --broker=None --port=5555 --address=localhost Which resulted in the following output: ``` [I 180320 14:16:57 command:139] Visit me at http://localhost:5555 [I 180320 14:16:57 command:144] Broker: amqp://guest:**@None:5672// ``` With this PR, but without the check, the resulting command on a missing broker config would be: celery flower -A superset.cli.celery_app --port=5555 --address=localhost With the output: ``` 2018-03-20 14:24:48,106:INFO:flower.command:Visit me at http://localhost:5555 2018-03-20 14:24:48,111:INFO:flower.command:Broker: amqp://guest:**@localhost:5672// ``` In both cases `flower` guessed wrongly that there is a broker configured using AMQP, and accessing port 5555 in the browser results in a page that never finishes loading. I think it would be better to just fail early in this case. As for the deprecation of external command wrapping, if you believe that there is little value in fixing the flower invocation, feel free to close this PR, but as long as there is a `superset flower` command, I'd rather it did the right thing.
---------------------------------------------------------------- This is an automated message from the Apache Git Service. To respond to the message, please log on GitHub and use the URL above to go to the specific comment. For queries about this service, please contact Infrastructure at: [email protected] With regards, Apache Git Services
