Fix flower broker configuration

[leorochael]

In case the celery broker is configured with transport options, these options would not be passed into flower with a --broker parameter.

A clear example is that, when launched with superset flower, the superset.sql_lab.get_sql_results task was not reported by flower on startup:

2018-03-16 16:57:37,208:INFO:flower.command:Registered tasks: 
['celery.accumulate',
 'celery.backend_cleanup',
 'celery.chain',
 'celery.chord',
 'celery.chord_unlock',
 'celery.chunks',
 'celery.group',
 'celery.map',
 'celery.starmap']

More importantly, flower could outright fail to work if a broker with less usual options was configured by superset, since these options were not passed at all into flower.

[leorochael]

I thought about reporting a bug first, but it was easier to just make a PR with the fix.

I'd appreciate if this PR could get a bug label, though.

[codecov-io]

Codecov Report

Merging #4631 into master will decrease coverage by 0.03%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #4631      +/-   ##
==========================================
- Coverage   71.25%   71.22%   -0.04%     
==========================================
  Files         190      190              
  Lines       14916    14879      -37     
  Branches     1102     1098       -4     
==========================================
- Hits        10629    10598      -31     
+ Misses       4284     4278       -6     
  Partials        3        3

Impacted Files	Coverage Δ
superset/cli.py	48.7% <ø> (+0.31%)	⬆️
superset/assets/javascripts/modules/utils.js	44.61% <0%> (-3.59%)	⬇️
...sets/javascripts/SqlLab/components/QuerySearch.jsx	67.39% <0%> (-1.77%)	⬇️
superset/dataframe.py	97.5% <0%> (-1.25%)	⬇️
superset/views/base.py	57.67% <0%> (-0.59%)	⬇️
...set/assets/javascripts/explore/stores/controls.jsx	37.59% <0%> (-0.19%)	⬇️
superset/connectors/druid/models.py	76.31% <0%> (-0.04%)	⬇️
superset/utils.py	87.83% <0%> (+0.08%)	⬆️
superset/assets/javascripts/chart/Chart.jsx	65.51% <0%> (+0.55%)	⬆️

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update fc47729...b1f8b34. Read the comment docs.

[john-bodley]

@leorochael thanks for also fixing the Pylint issue. If it's ok with you @michellethomas is going to incorporate your Pylint fix into her PR (#4632) to limit the scope of each PR. We'll merge it ASAP so you can rebase.

[leorochael]

@john-bodley, sure. It's rebased now.

I took the opportunity to enhance the diff to not start Flower if no BROKER_URL present. I'd appreciate a second opinion on the error message presented.

[john-bodley]

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.

Note we're planning on deprecating wrapping of these external commands per #4451 as they're problematic.

[leorochael]

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.

[john-bodley]

@leorochael for context I just merged #4451.

[leorochael]

Ok so, #4451 is merged deprecating the superset flower command, but in it's place it recommends:

celery flower --app=superset.sql_lab:celery_app

Rather than:

celery flower --app=superset.cli:celery_app

Is there a specific reason for preferring one over the other?

I'm asking to decide whether to close this PR or to increment it with a fix the documentation of how to invoke celery flower that was added in #4451 to use a more "canonical" celery_app.

It seems counter-intuitive to me that the one on sql_lab should be recommended...

[john-bodley]

@leorocheal the reason the documentation suggests using the sql_lab over cli is the cli one is an import from sql_lab and solely exists for the deprecated command.

[leorochael]

The cli one is not actually an import from sql_lab but it is true that sql_lab is the only place other than cli that uses get_celery_app(). And if superset is deprecating wrapped commands, it means the whole cli module will disappear eventually.

If superset ever uses celery someplace other than sql_lab, it should probably grow a celery module with a toplevel app that should be imported by it's common users.

For now, I'll just close this PR.