diff --git a/remoteappmanager/docker/container_manager.py b/remoteappmanager/docker/container_manager.py index 3885ef0a9..b5455c531 100644 --- a/remoteappmanager/docker/container_manager.py +++ b/remoteappmanager/docker/container_manager.py @@ -14,8 +14,8 @@ from remoteappmanager.logging.logging_mixin import LoggingMixin from remoteappmanager.utils import ( url_path_join, - without_end_slash, - deprecated) + without_end_slash) + from tornado import gen from traitlets import ( @@ -168,42 +168,6 @@ def stop_and_remove_container(self, container_id): finally: self._stop_pending.remove(container_id) - @gen.coroutine - @deprecated - def containers_from_mapping_id(self, user_name, mapping_id): - """Returns the currently running containers for a given user and - mapping_id. - - Parameters - ---------- - user_name: str - The username - mapping_id : str - The unique id to identify the container - - Return - ------ - A list of Container objects, or an empty list if nothing is found. - """ - return (yield self.find_containers(user_name=user_name, - mapping_id=mapping_id)) - - @gen.coroutine - @deprecated - def container_from_url_id(self, url_id): - """Retrieves and returns the container by its url_id, if present. - If not present, returns None. - """ - return (yield self.find_container(url_id=url_id)) - - @gen.coroutine - @deprecated - def running_containers(self): - """Returns all the running containers""" - containers = yield self.find_containers() - - return containers - @gen.coroutine def containers_with_labels(self, labels): filters = { @@ -344,14 +308,10 @@ def _start_container(self, self.log.info('Got container image: {}'.format(image_name)) # Check if the container is present. - containers = yield self.containers_from_mapping_id( - user_name, mapping_id) - - if len(containers) != 0: - # we assume only one present. The API is made for potential - # of multiple instances. - container = containers[0] + container = yield self.find_container( + user_name=user_name, mapping_id=mapping_id) + if container is not None: # Make sure we stop and remove it if by any chance is already # there. This will guarantee a fresh start every time. self.log.info('Container for image {} ' diff --git a/remoteappmanager/docker/tests/test_container_manager.py b/remoteappmanager/docker/tests/test_container_manager.py index bc4060065..1ec5c4bf9 100644 --- a/remoteappmanager/docker/tests/test_container_manager.py +++ b/remoteappmanager/docker/tests/test_container_manager.py @@ -56,10 +56,12 @@ def test_start_stop(self): self.assertTrue(mock_client.remove_container.called) @gen_test - def test_containers_from_mapping_id(self): + def test_find_from_mapping_id(self): """ Test containers_for_mapping_id returns a list of Container """ - result = yield self.manager.containers_from_mapping_id("username", - "mapping_id") + result = yield self.manager.find_containers( + user_name="username", + mapping_id="mapping_id") + expected = Container(docker_id='container_id1', mapping_id="mapping_id", name='/myrealm-username-mapping_5Fid', @@ -76,9 +78,10 @@ def test_containers_from_mapping_id(self): utils.assert_containers_equal(self, result[0], expected) @gen_test - def test_containers_from_url_id(self): + def test_find_from_url_id(self): """ Test containers_for_mapping_id returns a list of Container """ - result = yield self.manager.container_from_url_id("url_id") + result = yield self.manager.find_container( + url_id="url_id") expected = Container(docker_id='container_id1', mapping_id="mapping_id", name='/myrealm-username-mapping_5Fid', @@ -95,12 +98,12 @@ def test_containers_from_url_id(self): utils.assert_containers_equal(self, result, expected) @gen_test - def test_containers_from_url_id_exceptions(self): + def test_find_from_url_id_exceptions(self): """ Test containers_for_mapping_id returns a list of Container """ docker_client = self.mock_docker_client docker_client.port = mock.Mock(side_effect=Exception("Boom!")) - result = yield self.manager.container_from_url_id("url_id") + result = yield self.manager.find_container(url_id="url_id") self.assertEqual(result, None) # Making it so that no valid dictionary is returned. @@ -108,12 +111,12 @@ def test_containers_from_url_id_exceptions(self): self.mock_docker_client = docker_client self.manager._docker_client._sync_client = docker_client - result = yield self.manager.container_from_url_id("url_id") + result = yield self.manager.find_container(url_id="url_id") self.assertEqual(result, None) @gen_test - def test_running_containers(self): - result = yield self.manager.running_containers() + def test_find_containers(self): + result = yield self.manager.find_containers() self.assertEqual(len(result), 1) @gen_test @@ -289,11 +292,11 @@ def test_different_realm(self): realm="anotherrealm") manager._docker_client._sync_client = create_docker_client() - result = yield manager.containers_from_mapping_id("user_name", - "mapping_id") - self.assertEqual(result, []) + result = yield manager.find_container( + user_name="user_name", mapping_id="mapping_id") + self.assertIsNone(result) - result = yield manager.running_containers() + result = yield manager.find_containers() self.assertEqual(result, []) @gen_test diff --git a/remoteappmanager/handlers/admin/containers_handler.py b/remoteappmanager/handlers/admin/containers_handler.py index 07885be6b..6fcaf7e21 100644 --- a/remoteappmanager/handlers/admin/containers_handler.py +++ b/remoteappmanager/handlers/admin/containers_handler.py @@ -10,7 +10,7 @@ class ContainersHandler(BaseHandler): @gen.coroutine def get(self): manager = self.application.container_manager - containers = (yield manager.running_containers()) + containers = (yield manager.find_containers()) self.render('admin/containers.html', containers=containers, diff --git a/remoteappmanager/tests/utils.py b/remoteappmanager/tests/utils.py index 30715c947..702a9bb1c 100644 --- a/remoteappmanager/tests/utils.py +++ b/remoteappmanager/tests/utils.py @@ -116,6 +116,7 @@ def mock_coro_factory(return_value=None, side_effect=None): @gen.coroutine def coro(*args, **kwargs): coro.called = True + coro.call_args = (args, kwargs) yield gen.sleep(0.1) if side_effect: if isinstance(side_effect, Exception): diff --git a/remoteappmanager/webapi/admin/container.py b/remoteappmanager/webapi/admin/container.py index e98bf1f1e..32d9bf3f4 100644 --- a/remoteappmanager/webapi/admin/container.py +++ b/remoteappmanager/webapi/admin/container.py @@ -12,7 +12,7 @@ class Container(Resource): def delete(self, identifier): """Stop the container.""" container_manager = self.application.container_manager - container = yield container_manager.container_from_url_id(identifier) + container = yield container_manager.find_container(url_id=identifier) if not container: self.log.warning("Could not find container for id {}".format( diff --git a/remoteappmanager/webapi/admin/tests/test_container.py b/remoteappmanager/webapi/admin/tests/test_container.py index dbaa541ec..a216c7aa8 100644 --- a/remoteappmanager/webapi/admin/tests/test_container.py +++ b/remoteappmanager/webapi/admin/tests/test_container.py @@ -18,7 +18,7 @@ def get_app(self): return app def test_delete(self): - self._app.container_manager.container_from_url_id = mock_coro_factory( + self._app.container_manager.find_container = mock_coro_factory( DockerContainer(user="username") ) self.delete("/user/username/api/v1/containers/found/", @@ -26,13 +26,13 @@ def test_delete(self): self.assertTrue(self._app.reverse_proxy.unregister.called) - self._app.container_manager.container_from_url_id = \ + self._app.container_manager.find_container = \ mock_coro_factory(return_value=None) self.delete("/user/username/api/v1/containers/notfound/", httpstatus.NOT_FOUND) def test_delete_failure_reverse_proxy(self): - self._app.container_manager.container_from_url_id = mock_coro_factory( + self._app.container_manager.find_container = mock_coro_factory( DockerContainer(user="username") ) self._app.reverse_proxy.unregister.side_effect = Exception() @@ -42,7 +42,7 @@ def test_delete_failure_reverse_proxy(self): def test_delete_failure_stop_container(self): manager = self._app.container_manager - manager.container_from_url_id = mock_coro_factory( + manager.find_container = mock_coro_factory( DockerContainer(user="username") ) manager.stop_and_remove_container = mock_coro_factory( diff --git a/remoteappmanager/webapi/application.py b/remoteappmanager/webapi/application.py index 95ec89504..6ddaf00df 100644 --- a/remoteappmanager/webapi/application.py +++ b/remoteappmanager/webapi/application.py @@ -29,9 +29,9 @@ def retrieve(self, identifier): # available in docker. raise NotFound() - containers = yield container_manager.containers_from_mapping_id( - self.current_user.name, - identifier) + containers = yield container_manager.find_containers( + user_name=self.current_user.name, + mapping_id=identifier) representation = { "image": { diff --git a/remoteappmanager/webapi/container.py b/remoteappmanager/webapi/container.py index 73ed1c4a5..3ce7e5d3a 100644 --- a/remoteappmanager/webapi/container.py +++ b/remoteappmanager/webapi/container.py @@ -89,16 +89,15 @@ def create(self, representation): def retrieve(self, identifier): """Return the representation of the running container.""" container_manager = self.application.container_manager - container = yield container_manager.container_from_url_id(identifier) + container = yield container_manager.find_container( + url_id=identifier, + user_name=self.current_user.name) if container is None: self.log.warning("Could not find container for id {}".format( identifier)) raise exceptions.NotFound() - if container.user != self.current_user.name: - raise exceptions.NotFound() - return dict( name=container.name, image_name=container.image_name @@ -109,16 +108,16 @@ def retrieve(self, identifier): def delete(self, identifier): """Stop the container.""" container_manager = self.application.container_manager - container = yield container_manager.container_from_url_id(identifier) + container = yield container_manager.find_container( + url_id=identifier, + user_name=self.current_user.name + ) if not container: self.log.warning("Could not find container for id {}".format( identifier)) raise exceptions.NotFound() - if container.user != self.current_user.name: - raise exceptions.NotFound() - try: yield self.application.reverse_proxy.unregister( container.urlpath @@ -156,15 +155,11 @@ def items(self): # available in docker. We just move on. continue - containers = yield container_manager.containers_from_mapping_id( - self.current_user.name, - mapping_id) + container = yield container_manager.find_container( + user_name=self.current_user.name, + mapping_id=mapping_id) - # We assume that we can only run one container only (although the - # API considers a broader possibility for future extension. - if len(containers): - container = containers[0] - running_containers.append(container.url_id) + running_containers.append(container.url_id) return running_containers diff --git a/remoteappmanager/webapi/tests/test_application.py b/remoteappmanager/webapi/tests/test_application.py index 51e576a65..0163bca62 100644 --- a/remoteappmanager/webapi/tests/test_application.py +++ b/remoteappmanager/webapi/tests/test_application.py @@ -38,7 +38,7 @@ def get_app(self): app.container_manager = Mock() app.container_manager.image = mock_coro_factory( return_value=Image(name="boo", ui_name="foo_ui")) - app.container_manager.containers_from_mapping_id = mock_coro_factory( + app.container_manager.find_containers = mock_coro_factory( return_value=[]) application_mock_1 = Mock() application_mock_1.image = "hello1" @@ -94,7 +94,7 @@ def test_retrieve(self): }, 'mapping_id': 'one'}) - self._app.container_manager.containers_from_mapping_id = \ + self._app.container_manager.find_containers = \ mock_coro_factory(return_value=[Container( name="container", image_name="xxx", diff --git a/remoteappmanager/webapi/tests/test_container.py b/remoteappmanager/webapi/tests/test_container.py index 69056cad9..444a275dd 100644 --- a/remoteappmanager/webapi/tests/test_container.py +++ b/remoteappmanager/webapi/tests/test_container.py @@ -25,7 +25,7 @@ def get_app(self): def test_items(self): manager = self._app.container_manager manager.image = mock_coro_factory(Image()) - manager.containers_from_mapping_id = mock_coro_factory( + manager.find_containers = mock_coro_factory( [DockerContainer()]) code, data = self.get( @@ -221,7 +221,7 @@ def test_create_fails_for_invalid_mapping_id(self): "message": "unrecognized mapping_id"}) def test_retrieve(self): - self._app.container_manager.container_from_url_id = mock_coro_factory( + self._app.container_manager.find_container = mock_coro_factory( DockerContainer(user="username") ) _, data = self.get("/user/username/api/v1/containers/found/", @@ -230,21 +230,21 @@ def test_retrieve(self): self.assertEqual(data["image_name"], "") self.assertEqual(data["name"], "") - self._app.container_manager.container_from_url_id = \ + self._app.container_manager.find_container = \ mock_coro_factory(return_value=None) self.get("/user/username/api/v1/containers/notfound/", httpstatus.NOT_FOUND) def test_prevent_retrieve_from_other_user(self): - self._app.container_manager.container_from_url_id = mock_coro_factory( - DockerContainer(user="foo") - ) + self._app.container_manager.find_container = mock_coro_factory(None) self.get("/user/username/api/v1/containers/found/", httpstatus.NOT_FOUND) + kwargs = self._app.container_manager.find_container.call_args[1] + self.assertEqual(kwargs["user_name"], "username") def test_delete(self): - self._app.container_manager.container_from_url_id = mock_coro_factory( + self._app.container_manager.find_container = mock_coro_factory( DockerContainer(user="username") ) @@ -252,25 +252,28 @@ def test_delete(self): httpstatus.NO_CONTENT) self.assertTrue(self._app.reverse_proxy.unregister.called) - self._app.container_manager.container_from_url_id = \ + self._app.container_manager.find_container = \ mock_coro_factory(return_value=None) self.delete("/user/username/api/v1/containers/notfound/", httpstatus.NOT_FOUND) def test_prevent_delete_from_other_user(self): - self._app.container_manager.container_from_url_id = mock_coro_factory( - DockerContainer(user="foo") + self._app.container_manager.find_container = mock_coro_factory( + None ) self.delete("/user/username/api/v1/containers/found/", httpstatus.NOT_FOUND) + kwargs = self._app.container_manager.find_container.call_args[1] + self.assertEqual(kwargs["user_name"], "username") + def test_post_start(self): with patch("remoteappmanager" ".webapi" ".container" ".wait_for_http_server_2xx", new_callable=mock_coro_factory): - self._app.container_manager.containers_from_mapping_id = \ + self._app.container_manager.find_containers = \ mock_coro_factory(return_value=[DockerContainer()]) self.assertFalse(self._app.reverse_proxy.register.called) diff --git a/selenium_tests/test_admin_name_header_bug.py b/selenium_tests/test_admin_name_header_bug.py index 12113aa0c..14134e8e4 100644 --- a/selenium_tests/test_admin_name_header_bug.py +++ b/selenium_tests/test_admin_name_header_bug.py @@ -5,7 +5,6 @@ class TestAdminNameHeaderBug(SeleniumTestBase): def test_admin_name_header_bug(self): driver = self.driver with self.login("admin"): - print("hello") driver.find_element_by_link_text("Users").click() driver.find_element_by_link_text("Show").click() self.wait_for(lambda: