Skip to content
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

mgr/dashboard v2: implement can_run method #20728

Merged
merged 1 commit into from Mar 14, 2018

Conversation

jcsp
Copy link
Contributor

@jcsp jcsp commented Mar 5, 2018

This is just to give us friendlier behaviour if anything the dashboard expects is missing.

@jcsp jcsp changed the title [DNM] mgr/dashboard v2: implement can_run method mgr/dashboard v2: implement can_run method Mar 6, 2018
@jcsp jcsp removed the DNM label Mar 6, 2018
@@ -74,6 +79,20 @@ def __init__(self, *args, **kwargs):
mgr.init(self)
self._url_prefix = ''

def can_run(self):
if cherrypy is None:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we also need "bcrypt" package for the authentication to work, can we also add that check to this method?

Copy link
Contributor

@LenzGr LenzGr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, thank you!

@rjfd
Copy link
Contributor

rjfd commented Mar 12, 2018

@jcsp regarding the bcrypt comment I left, do you think it's not worth it to add at this point?

@jcsp
Copy link
Contributor Author

jcsp commented Mar 12, 2018

@rjfd the awkward thing about checking for bcrypt is that it's not directly used in module.py, so we'd need a more general approach where we're catching the ImportError from the other modules too -- it could be interesting to do that by separating the web server into a separate .py so that we could readily catch all import errors while importing that from module.py, but that's probably for another day

@rjfd
Copy link
Contributor

rjfd commented Mar 12, 2018

@jcsp I agree with you, importing bcrypt is not very elegant. In that case I'm fine with merging the PR as it is.

Copy link
Contributor

@rjfd rjfd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm

@rjfd rjfd added the needs-qa label Mar 12, 2018
@@ -74,6 +79,20 @@ def __init__(self, *args, **kwargs):
mgr.init(self)
self._url_prefix = ''

def can_run(self):
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the can_run method in ../mgr_module.py is declared as a static method. As it is declared here breaks the module loading. Please make this a static method.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oops. Updated. Had to un-property-ize the frontend_path method to get it usable from here too.

Should be especially handy in development environments
for giving a clear message for people who have forgotten
to build frontend bits.

Signed-off-by: John Spray <john.spray@redhat.com>
@rjfd
Copy link
Contributor

rjfd commented Mar 14, 2018

@LenzGr LenzGr merged commit 5dfdecc into ceph:master Mar 14, 2018
epuertat pushed a commit to rhcs-dashboard/ceph that referenced this pull request Jul 17, 2018
mgr/dashboard v2: implement can_run method

Reviewed-by: Ricardo Dias <rdias@suse.com>
Reviewed-by: Sebastian Wagner <sebastian.wagner@suse.com>
(cherry picked from commit 5dfdecc)
Signed-off-by: Ernesto Puerta <epuertat@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants