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: Removed unused tools.detail_route() #20765

Merged
merged 2 commits into from Mar 12, 2018

Conversation

sebastian-philipp
Copy link
Contributor

Refactored corresponding RESTControllerTest:

  • Made RESTControllerTest inherent from ControllerTestCase
  • Refactored ControllerTestCase
  • Simplified all tests that inherent from ControllerTestCase

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

@tchaikov
Copy link
Contributor

tchaikov commented Mar 7, 2018

retest this please

@sebastian-philipp
Copy link
Contributor Author

@tchaikov looks like the dashboard_v2 smoke test needs an improvement.

@tchaikov
Copy link
Contributor

tchaikov commented Mar 7, 2018

@sebastian-philipp i believe the improvement has been merged as a part of #20768. that's why i re-triggerred the "make check" run after the merge. =)

@rjfd
Copy link
Contributor

rjfd commented Mar 7, 2018

@sebastian-philipp the backend API tests are failing:

2018-03-07 11:58:25,137.137 INFO:tasks.mgr.dashboard_v2.helper:request POST to http://rdias-suse-laptop.rdias.home.pt:7789//api/auth
2018-03-07 11:58:25,380.380 INFO:tasks.mgr.dashboard_v2.helper:request GET to http://rdias-suse-laptop.rdias.home.pt:7789//api/cluster_conf/admin_socket
2018-03-07 11:58:25,391.391 INFO:__main__:test_get (tasks.mgr.dashboard_v2.test_cluster_configuration.ClusterConfigurationTest) ... FAIL
2018-03-07 11:58:25,391.391 INFO:__main__:run args=['./bin/ceph', 'log', 'Ended test tasks.mgr.dashboard_v2.test_cluster_configuration.ClusterConfigurationTest.test_get']
2018-03-07 11:58:25,391.391 INFO:__main__:Running ['./bin/ceph', 'log', 'Ended test tasks.mgr.dashboard_v2.test_cluster_configuration.ClusterConfigurationTest.test_get']
2018-03-07 11:58:25,657.657 INFO:__main__:Stopped test: test_get (tasks.mgr.dashboard_v2.test_cluster_configuration.ClusterConfigurationTest) in 1.896289s
2018-03-07 11:58:25,658.658 INFO:__main__:
2018-03-07 11:58:25,658.658 INFO:__main__:======================================================================
2018-03-07 11:58:25,658.658 INFO:__main__:FAIL: test_get (tasks.mgr.dashboard_v2.test_cluster_configuration.ClusterConfigurationTest)
2018-03-07 11:58:25,658.658 INFO:__main__:----------------------------------------------------------------------
2018-03-07 11:58:25,658.658 INFO:__main__:Traceback (most recent call last):
2018-03-07 11:58:25,658.658 INFO:__main__:  File "/home/rdias/Work/ceph/qa/tasks/mgr/dashboard_v2/helper.py", line 23, in decorate
2018-03-07 11:58:25,658.658 INFO:__main__:    return func(self, *args, **kwargs)
2018-03-07 11:58:25,658.658 INFO:__main__:  File "/home/rdias/Work/ceph/qa/tasks/mgr/dashboard_v2/test_cluster_configuration.py", line 43, in test_get
2018-03-07 11:58:25,658.658 INFO:__main__:    self.assertStatus(200)
2018-03-07 11:58:25,659.659 INFO:__main__:  File "/home/rdias/Work/ceph/qa/tasks/mgr/dashboard_v2/helper.py", line 119, in assertStatus
2018-03-07 11:58:25,659.659 INFO:__main__:    self.assertEqual(self._resp.status_code, status)
2018-03-07 11:58:25,659.659 INFO:__main__:AssertionError: 500 != 200
2018-03-07 11:58:25,659.659 INFO:__main__:
2018-03-07 11:58:25,659.659 INFO:__main__:----------------------------------------------------------------------

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.

the api tests are failing...

@rjfd
Copy link
Contributor

rjfd commented Mar 7, 2018

@sebastian-philipp I just caught the same failure while testing another PR, maybe the failure is caused by some commit merged to master recently and not your PR.

@sebastian-philipp
Copy link
Contributor Author

sebastian-philipp commented Mar 7, 2018

@rjfd This fixes my issue:

git diff src/common/config.cc | cat 
diff --git a/src/common/config.cc b/src/common/config.cc
index a1f0fbdbf2..1932e0d3b3 100644
--- a/src/common/config.cc
+++ b/src/common/config.cc
@@ -503,7 +503,9 @@ void md_config_t::config_options(Formatter *f)
   f->open_array_section("options");
   for (const auto& i: schema) {
     const Option &opt = i.second;
+    f->open_object_section("option");
     opt.dump(f);
+    f->close_section();
   }
   f->close_section();
 }

As the current API endpoint returns garbage right now.

@rjfd
Copy link
Contributor

rjfd commented Mar 7, 2018

@sebastian-philipp that part of the of common/config.cc code was not changed in the last two months.
Although it might fix the problem I would like to know what was the commit that introduced the bug and why does it cause a failure, so that we can fix it properly in a different PR.

@rjfd
Copy link
Contributor

rjfd commented Mar 7, 2018

I created an issue in http://tracker.ceph.com/issues/23265 to keep track of this bug

@sebastian-philipp
Copy link
Contributor Author

See #20782

@s0nea s0nea added the mgr label Mar 8, 2018
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.

There are two logical changes in this commit. Please split the commit in two, where one takes care of the detail_route removal, and the other removes the requests package dependency from the unit tests.

Signed-off-by: Sebastian Wagner <sebastian.wagner@suse.com>
* Made `RESTControllerTest` inherent from `ControllerTestCase`
* Refactored `ControllerTestCase`
* Simplified all tests that inherent from  `ControllerTestCase`

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

@rjfd commit is now split in two.

@rjfd
Copy link
Contributor

rjfd commented Mar 12, 2018

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

@LenzGr LenzGr merged commit 9d6eaa7 into ceph:master Mar 12, 2018
@sebastian-philipp sebastian-philipp deleted the dashboard_v2-no-detail_route branch March 13, 2018 12:41
epuertat pushed a commit to rhcs-dashboard/ceph that referenced this pull request Jul 17, 2018
…detail_route

mgr/dashboard_v2: Removed unused `tools.detail_route()`

Reviewed-by: Ricardo Dias <rdias@suse.com>
(cherry picked from commit 9d6eaa7)
Signed-off-by: Ernesto Puerta <epuertat@redhat.com>
epuertat pushed a commit to rhcs-dashboard/ceph that referenced this pull request Jul 24, 2018
…detail_route

mgr/dashboard_v2: Removed unused `tools.detail_route()`

Reviewed-by: Ricardo Dias <rdias@suse.com>
(cherry picked from commit 9d6eaa7)
Signed-off-by: Ernesto Puerta <epuertat@redhat.com>
epuertat pushed a commit to rhcs-dashboard/ceph that referenced this pull request Jul 26, 2018
…detail_route

mgr/dashboard_v2: Removed unused `tools.detail_route()`

Reviewed-by: Ricardo Dias <rdias@suse.com>
(cherry picked from commit 9d6eaa7)
Signed-off-by: Ernesto Puerta <epuertat@redhat.com>
epuertat pushed a commit to rhcs-dashboard/ceph that referenced this pull request Jul 26, 2018
…detail_route

mgr/dashboard_v2: Removed unused `tools.detail_route()`

Reviewed-by: Ricardo Dias <rdias@suse.com>
(cherry picked from commit 9d6eaa7)
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
5 participants