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/restful: A couple of restful fixes #18649

Merged
merged 4 commits into from Nov 29, 2017
Merged

Conversation

b-ranto
Copy link
Contributor

@b-ranto b-ranto commented Oct 31, 2017

This fixes a couple of bugs in the ceph-mgr restful module.

The CommandsRequest.waiting is an array of arrays, not an array of
command results and so it cannot be represented that way. This commit
fixes that by outputting the array in json diretly.

Signed-off-by: Boris Ranto <branto@redhat.com>
Previously, we forgot to set the value of the argument for the second
group of pool variables.

Signed-off-by: Boris Ranto <branto@redhat.com>
Signed-off-by: Boris Ranto <branto@redhat.com>
@tchaikov tchaikov added the mgr label Oct 31, 2017
@tchaikov tchaikov changed the title A couple of restful fixes mgr/restful: A couple of restful fixes Oct 31, 2017
@jcsp
Copy link
Contributor

jcsp commented Oct 31, 2017

This PR seems like a good time to add some kind of test coverage for the modifying operations

@jcsp
Copy link
Contributor

jcsp commented Nov 27, 2017

@b-ranto ping

Signed-off-by: Boris Ranto <branto@redhat.com>
@b-ranto
Copy link
Contributor Author

b-ranto commented Nov 27, 2017

There already are some tests for the modifying operations. However, we did not test pg_num/pgp_num updates. I have added that to the workunit testing the restful module in the latest commit.

@jcsp jcsp added the needs-qa label Nov 27, 2017
@jcsp
Copy link
Contributor

jcsp commented Nov 27, 2017

I'm a bit concerned that those tests are only going as far as checking the HTTP response from submitting a request: for example the bug behind the "restful: Set the value of the argument" fix would not be caught by this (or would it?). Something like https://github.com/ceph/calamari/blob/master/tests/test_pool_management.py#L196 would be nice.

These tests would fit nicely in qa/tasks/mgr where there is more of a framework for tests written in python.

I don't want to hold up these patches any longer, but if we're maintaining this module on an ongoing basis then I would like to see more complete tests to come with future PRs.

@jcsp jcsp merged commit 967fc9c into ceph:master Nov 29, 2017
@b-ranto b-ranto deleted the wip-restful-fixes branch November 29, 2017 14:17
@tserlin
Copy link
Contributor

tserlin commented May 7, 2018

Is there a backport for Luminous?

@tchaikov
Copy link
Contributor

tchaikov commented May 8, 2018

@tserlin #21871

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
4 participants