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: Add backend support for changing dashboard configuration settings via the REST API #22457

Merged
merged 1 commit into from Aug 14, 2018

Conversation

Projects
None yet
9 participants
@p-na
Contributor

p-na commented Jun 7, 2018

Enables to change (set/unset) values of settings of the dashboard using the REST API.

Fixes: https://tracker.ceph.com/issues/24273

Signed-off-by: Patrick Nawracay pnawracay@suse.com

try:
default, data_type = getattr(Options, attr)
except AttributeError:
raise cherrypy.HTTPError(404)

This comment has been minimized.

@votdev

votdev Jun 8, 2018

Contributor

Consider to use DashboardException(http_status_code=404) here.

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Jun 8, 2018

Member

I'd use cherrypy.NotFound()

try:
setattr(Settings, self._to_native(name), value)
except AttributeError:
raise cherrypy.HTTPError(404)

This comment has been minimized.

@votdev

votdev Jun 8, 2018

Contributor

Consider to use DashboardException(http_status_code=404) here.

try:
delattr(Settings, self._to_native(name))
except AttributeError:
raise cherrypy.HTTPError(404)

This comment has been minimized.

@votdev

votdev Jun 8, 2018

Contributor

Consider to use DashboardException(http_status_code=404) here.

for name, value in kwargs.items():
setattr(Settings, self._to_native(name), value)
except AttributeError:
raise cherrypy.HTTPError(404)

This comment has been minimized.

@votdev

votdev Jun 8, 2018

Contributor

Consider to use DashboardException(http_status_code=404) here.

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Jun 8, 2018

Member

does it make sense to refactor this into a separate method or function?

try:
default, data_type = getattr(Options, attr)
except AttributeError:
raise cherrypy.HTTPError(404)

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Jun 8, 2018

Member

I'd use cherrypy.NotFound()

def _to_native(setting):
return setting.upper().replace('-', '_')
def list(self):

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Jun 8, 2018

Member
def list(self):
    return [self.get(name) for name in Options.__dict__]

??

This comment has been minimized.

@p-na

p-na Jun 11, 2018

Contributor
return [
    json.loads(self.get(name)) for name in Options.__dict__
    if name.isupper() and not name.startswith('_')
]

That would work, but it requires to unserialize the result of get as the result is automatically serialized. Do you prefer this solution?

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Jun 11, 2018

Member

why do you need to run json.loads?

This comment has been minimized.

@p-na

p-na Jun 11, 2018

Contributor

The list method of the RESTController class is being modified at initialization, so that it returns the serialized version of the output of list. I resolved this by creating a _get method, like you proposed in our call.

for name, value in kwargs.items():
setattr(Settings, self._to_native(name), value)
except AttributeError:
raise cherrypy.HTTPError(404)

This comment has been minimized.

@sebastian-philipp

sebastian-philipp Jun 8, 2018

Member

does it make sense to refactor this into a separate method or function?

@LenzGr LenzGr changed the title from mgr/dashboard: Add support for changing configuration settings via the UI to mgr/dashboard: Add support for changing dashboard configuration settings via the UI Jun 11, 2018

@p-na p-na force-pushed the p-na:dash-settings branch 2 times, most recently from 9695fcf to 8be01b6 Jun 11, 2018

@votdev

votdev approved these changes Jun 13, 2018

@votdev votdev added the needs-review label Jun 15, 2018

@LenzGr LenzGr changed the title from mgr/dashboard: Add support for changing dashboard configuration settings via the UI to mgr/dashboard: Add backend support for changing dashboard configuration settings via the REST API Jun 15, 2018

@LenzGr LenzGr added the needs-rebase label Jun 26, 2018

@p-na p-na force-pushed the p-na:dash-settings branch from 8be01b6 to c61e12f Jul 23, 2018

outdated

@p-na p-na removed the needs-rebase label Jul 24, 2018

@p-na p-na force-pushed the p-na:dash-settings branch 2 times, most recently from c286808 to 20d3218 Jul 25, 2018

@p-na p-na force-pushed the p-na:dash-settings branch from 20d3218 to 82c9aa6 Jul 26, 2018

@p-na

This comment has been minimized.

Contributor

p-na commented Jul 26, 2018

jenkins retest this please

@p-na

This comment has been minimized.

Contributor

p-na commented Jul 26, 2018

jenkins retest this please

1 similar comment
@s0nea

This comment has been minimized.

Contributor

s0nea commented Jul 30, 2018

jenkins retest this please

@s0nea

s0nea approved these changes Aug 1, 2018

@rjfd

Please add integration (QA) tests for testing the Settings controller.

@p-na p-na force-pushed the p-na:dash-settings branch from 82c9aa6 to 452f59a Aug 3, 2018

Implemented

@p-na p-na force-pushed the p-na:dash-settings branch from 452f59a to 3f82a65 Aug 6, 2018

@p-na p-na removed the needs-review label Aug 7, 2018

@p-na p-na removed the needs-review label Aug 7, 2018

@callithea

This comment has been minimized.

@ricardoasmarques

I've added some suggestions to improve the swagger API documentation.

with self._attribute_handler(name) as sname:
delattr(Settings, self._to_native(sname))
def bulk_set(self, **kwargs):

This comment has been minimized.

@ricardoasmarques

ricardoasmarques Aug 9, 2018

Member

The swagger API docs generator doesn't play well with this **kargs parameter (documentation says there is not parameters):

screenshot_20180809_160044

Is this something that can be fixed?

This comment has been minimized.

@p-na

p-na Aug 10, 2018

Contributor

I've looked into it and yes, I think it can be fixed but it's a general issue with generating the API documentation (or how the Endpoints are generated) for the bulk_set method of the RestController. Hence, I'd say it shouldn't be fixed in this PR.

This comment has been minimized.

@ricardoasmarques

ricardoasmarques Aug 10, 2018

Member

Thanks for the info. Agree, this should be fixed in a separate PR.

@ApiController('/settings', Scope.CONFIG_OPT)
class SettingsController(RESTController):

This comment has been minimized.

@ricardoasmarques

ricardoasmarques Aug 9, 2018

Member

Shouldn't we rename this controller to Settings, for consistency in the API documentation?

screenshot_20180809_160142

@p-na p-na force-pushed the p-na:dash-settings branch from 3f82a65 to 5becf7b Aug 10, 2018

mgr/dashboard/backend: API to manage dashboard settings
Enables to change (set/unset) values of settings of the dashboard using
the REST API.

Fixes: https://tracker.ceph.com/issues/24273

Signed-off-by: Patrick Nawracay <pnawracay@suse.com>

@p-na p-na force-pushed the p-na:dash-settings branch from 5becf7b to 7340e70 Aug 10, 2018

@s0nea

This comment has been minimized.

Contributor

s0nea commented Aug 13, 2018

I don't have the feeling that the error is related to this pull request:

2018-08-10T14:25:46.695 INFO:tasks.cephfs_test_runner:======================================================================
2018-08-10T14:25:46.696 INFO:tasks.cephfs_test_runner:FAIL: test_list (tasks.mgr.dashboard.test_rgw.RgwUserTest)
2018-08-10T14:25:46.696 INFO:tasks.cephfs_test_runner:----------------------------------------------------------------------
2018-08-10T14:25:46.696 INFO:tasks.cephfs_test_runner:Traceback (most recent call last):
2018-08-10T14:25:46.696 INFO:tasks.cephfs_test_runner:  File "/home/teuthworker/src/git.ceph.com_ceph-c_wip-lpaduano-testing-22457-2/qa/tasks/mgr/dashboard/test_rgw.py", line 260, in test_list
2018-08-10T14:25:46.696 INFO:tasks.cephfs_test_runner:    self.assertStatus(200)
2018-08-10T14:25:46.696 INFO:tasks.cephfs_test_runner:  File "/home/teuthworker/src/git.ceph.com_ceph-c_wip-lpaduano-testing-22457-2/qa/tasks/mgr/dashboard/helper.py", line 325, in assertStatus
2018-08-10T14:25:46.696 INFO:tasks.cephfs_test_runner:    self.assertEqual(self._resp.status_code, status)
2018-08-10T14:25:46.696 INFO:tasks.cephfs_test_runner:AssertionError: 500 != 200
2018-08-10T14:25:46.696 INFO:tasks.cephfs_test_runner:
2018-08-10T14:25:46.696 INFO:tasks.cephfs_test_runner:----------------------------------------------------------------------
2018-08-10T14:25:46.697 INFO:tasks.cephfs_test_runner:Ran 62 tests in 753.519s
2018-08-10T14:25:46.697 INFO:tasks.cephfs_test_runner:
2018-08-10T14:25:46.697 INFO:tasks.cephfs_test_runner:FAILED (failures=1)
2018-08-10T14:25:46.697 INFO:tasks.cephfs_test_runner:
2018-08-10T14:25:46.697 INFO:tasks.cephfs_test_runner:======================================================================
2018-08-10T14:25:46.697 INFO:tasks.cephfs_test_runner:FAIL: test_list (tasks.mgr.dashboard.test_rgw.RgwUserTest)
2018-08-10T14:25:46.697 INFO:tasks.cephfs_test_runner:----------------------------------------------------------------------
2018-08-10T14:25:46.697 INFO:tasks.cephfs_test_runner:Traceback (most recent call last):
2018-08-10T14:25:46.697 INFO:tasks.cephfs_test_runner:  File "/home/teuthworker/src/git.ceph.com_ceph-c_wip-lpaduano-testing-22457-2/qa/tasks/mgr/dashboard/test_rgw.py", line 260, in test_list
2018-08-10T14:25:46.697 INFO:tasks.cephfs_test_runner:    self.assertStatus(200)
2018-08-10T14:25:46.697 INFO:tasks.cephfs_test_runner:  File "/home/teuthworker/src/git.ceph.com_ceph-c_wip-lpaduano-testing-22457-2/qa/tasks/mgr/dashboard/helper.py", line 325, in assertStatus
2018-08-10T14:25:46.697 INFO:tasks.cephfs_test_runner:    self.assertEqual(self._resp.status_code, status)
2018-08-10T14:25:46.698 INFO:tasks.cephfs_test_runner:AssertionError: 500 != 200

@ricardoasmarques ricardoasmarques merged commit 2a6ce09 into ceph:master Aug 14, 2018

5 checks passed

Docs: build check OK - docs built
Details
Signed-off-by all commits in this PR are signed
Details
Unmodified Submodules submodules for project are unmodified
Details
make check make check succeeded
Details
make check (arm64) make check succeeded
Details

@s0nea s0nea referenced this pull request Dec 11, 2018

Merged

mgr/dashboard: Settings service #25327

2 of 2 tasks complete
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment