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.

Sign up for GitHub

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

Jump to bottom

mgr/dashboard: Add minimalistic browsable API #20873

Merged
merged 3 commits into from Mar 27, 2018
Merged

mgr/dashboard: Add minimalistic browsable API #20873

merged 3 commits into from Mar 27, 2018

Conversation

sebastian-philipp
Copy link
Contributor

@sebastian-philipp sebastian-philipp commented Mar 13, 2018 •
edited

Also provides a simple HTML form to POST
data to a RESTController's create() method.

obligatory Screenshot:

html-debug-view

Signed-off-by: Sebastian Wagner sebastian.wagner@suse.com

@rjfd
Copy link
Contributor

rjfd commented Mar 14, 2018

This HTML form generator looks nice and helpful to test some API endpoints during development, but I have one question on how are we supposed to use this? should we add the @debug_html_page decorator while developing a rest controller endpoint, but then remove the decorator when we push the commits to the PR? or should we leave the decorator in the code? (if it's the latter then we need some config option to disable the debug page in production)

@sebastian-philipp
Copy link
Contributor Author

@rjfd actually, I would keep it enabled also for production use.

  1. If you look at a page, if is obvious that this is not meant to be used in production
  2. It doesn't provide anything that you cannot use via curl.
  3. In openATTIC, we also didn't disable this API viewer.

Another idea would be to rephrase this as preliminary "API documentation" or "(incomplete) browsable API".

@LenzGr
Copy link
Contributor

LenzGr commented Mar 14, 2018

I like the basic idea behind this, but if it's supposed to be enabled by default, the name @debug_html_page is indeed misleading.

Having an automated way of creating a browseable "API reference" is actually a nice side benefit that makes me want to have this :)

In Django, this is a builtin-feature in the Django REST framework (IIRC), where it can be enabled/disabled on a global level. Maybe this would be a better approach than having to add a decorator to every controller endpoint?

@sebastian-philipp sebastian-philipp changed the title mgr/dashboard_v2: Add minimalistic HTML debug representation of the API mgr/dashboard_v2: Add minimalistic browsable API Mar 15, 2018
@sebastian-philipp
Copy link
Contributor Author

@rjfd : Just added ENABLE_BROWSABLE_API setting to our settings.
@LenzGr: Renamed the PR to "browsable API"

I'd favor this setting to be True by default, but I don't mind to set this to False

@sebastian-philipp
Copy link
Contributor Author

@LenzGr: added a disclaimer

@sebastian-philipp sebastian-philipp changed the title mgr/dashboard_v2: Add minimalistic browsable API mgr/dashboard: Add minimalistic browsable API Mar 16, 2018
@sebastian-philipp
Copy link
Contributor Author

jenkins retest this please

s0nea
s0nea requested changes Mar 23, 2018
View reviewed changes
src/pybind/mgr/dashboard/tools.py Outdated
@@ -473,27 +636,27 @@ def stop(cls):
logger.debug("notification queue stopped")

@classmethod
def register(cls, func, types=None):
def register(cls, func, n_types=None):
"""Registers function to listen for notifications

If the second parameter `types` is omitted, the function in `func`
Copy link
Member

Choose a reason for hiding this comment

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

I guess 'types' should here be 'n_types' as well?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

LenzGr
LenzGr approved these changes Mar 24, 2018
View reviewed changes
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.

I tested this and it worked for me. Nice work!

rjfd
rjfd suggested changes Mar 26, 2018
View reviewed changes
src/pybind/mgr/dashboard/tools.py
@@ -473,27 +617,27 @@ def stop(cls):
logger.debug("notification queue stopped")

@classmethod
def register(cls, func, types=None):
def register(cls, func, n_types=None):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you put this renaming part into a separate commit?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fixed

src/pybind/mgr/dashboard/controllers/monitor.py
@@ -14,7 +14,7 @@
class Monitor(BaseController):
@cherrypy.expose
@cherrypy.tools.json_out()
def default(self):
def default(self, *_vpath, **_params):
Copy link
Contributor

Choose a reason for hiding this comment

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

Is there a way to not require to add those parameters in all default methods?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pylint says; W:464, 4: Parameters differ from overridden 'default' method (arguments-differ). In general, I think, it's OK to have consistent method arguments for derived classes.

@sebastian-philipp
mgr/dashboard: Renamed types as it clashes with std lib
f959709 
This will lead to a name clash, when importing `types`

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
@sebastian-philipp
mgr/dashboard: Add minimalistic browsable API
80b6ce8 
Also provides a simple HTML form to POST
data to a `RESTController`'s `create()` method.

Also added ENABLE_BROWSABLE_API setting to the dashboard

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
@sebastian-philipp
mgr/dashboard: New API tests for perf_couters
7a7a17c 
Added OSD and MDS tests.

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
@rjfd
Copy link
Contributor

rjfd commented Mar 27, 2018

QA tests run successful:
http://pulpito.ceph.com/rdias-2018-03-26_16:07:05-rados:mgr-wip-rdias-testing-distro-basic-smithi/
http://pulpito.ceph.com/rdias-2018-03-26_18:46:21-rados-wip-rdias-testing-distro-basic-smithi/

p-se
p-se approved these changes Mar 27, 2018
View reviewed changes
Copy link
Contributor

@p-se p-se left a comment

Choose a reason for hiding this comment

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

Tested and works for me (except for those API endpoints which currently aren't supported yet).

@LenzGr LenzGr merged commit 43e5097 into ceph:master Mar 27, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@LenzGr LenzGr LenzGr approved these changes

@votdev votdev votdev approved these changes

@s0nea s0nea s0nea approved these changes

@p-se p-se p-se approved these changes

@rjfd rjfd rjfd approved these changes

Assignees
No one assigned
Labels
dashboard feature
Projects
None yet
Milestone
No milestone
6 participants
@sebastian-philipp @rjfd @LenzGr @votdev @s0nea @p-se