Merge "Introduce overwrite functionality in ``osc router set``"

2 files changed, 50 insertions, 27 deletions

diff --git a/openstackclient/network/v2/router.py b/openstackclient/network/v2/router.py
index 45507b53..958a95b3 100644
--- a/openstackclient/network/v2/router.py
+++ b/openstackclient/network/v2/router.py
@@ -98,8 +98,6 @@ def _get_attrs(client_manager, parsed_args):
         ).id
         attrs['tenant_id'] = project_id
 
-    # TODO(tangchen): Support getting 'external_gateway_info' property.
-
     return attrs
 
 
@@ -464,7 +462,8 @@ class SetRouter(command.Command):
             help=_("Set router to centralized mode (disabled router only)")
         )
         routes_group = parser.add_mutually_exclusive_group()
-        routes_group.add_argument(
+        # ToDo(Reedip):Remove mutual exclusiveness once clear-routes is removed
+        parser.add_argument(
             '--route',
             metavar='destination=<subnet>,gateway=<ip-address>',
             action=parseractions.MultiKeyValueAction,
@@ -479,7 +478,9 @@ class SetRouter(command.Command):
         routes_group.add_argument(
             '--no-route',
             action='store_true',
-            help=_("Clear routes associated with the router")
+            help=_("Clear routes associated with the router. "
+                   "Specify both --route and --no-route to overwrite "
+                   "current value of route.")
         )
         routes_group.add_argument(
             '--clear-routes',
@@ -539,20 +540,22 @@ class SetRouter(command.Command):
             attrs['ha'] = True
         elif parsed_args.no_ha:
             attrs['ha'] = False
-        if parsed_args.no_route:
-            attrs['routes'] = []
-        elif parsed_args.clear_routes:
-            attrs['routes'] = []
+        if parsed_args.clear_routes:
             LOG.warning(_(
                 'The --clear-routes option is deprecated, '
                 'please use --no-route instead.'
             ))
-        elif parsed_args.routes is not None:
-            # Map the route keys and append to the current routes.
-            # The REST API will handle route validation and duplicates.
+
+        if parsed_args.routes is not None:
             for route in parsed_args.routes:
                 route['nexthop'] = route.pop('gateway')
-            attrs['routes'] = obj.routes + parsed_args.routes
+            attrs['routes'] = parsed_args.routes
+            if not (parsed_args.no_route or parsed_args.clear_routes):
+                # Map the route keys and append to the current routes.
+                # The REST API will handle route validation and duplicates.
+                attrs['routes'] += obj.routes
+        elif parsed_args.no_route or parsed_args.clear_routes:
+            attrs['routes'] = []
         if (parsed_args.disable_snat or parsed_args.enable_snat or
            parsed_args.fixed_ip) and not parsed_args.external_gateway:
                 msg = (_("You must specify '--external-gateway' in order"
diff --git a/openstackclient/tests/unit/network/v2/test_router.py b/openstackclient/tests/unit/network/v2/test_router.py
index a24a34c5..b837afd1 100644
--- a/openstackclient/tests/unit/network/v2/test_router.py
+++ b/openstackclient/tests/unit/network/v2/test_router.py
@@ -704,10 +704,10 @@ class TestSetRouter(TestRouter):
         parsed_args = self.check_parser(self.cmd, arglist, verifylist)
 
         result = self.cmd.take_action(parsed_args)
-
+        routes = [{'destination': '10.20.30.0/24',
+                   'nexthop': '10.20.30.1'}]
         attrs = {
-            'routes': self._router.routes + [{'destination': '10.20.30.0/24',
-                                             'nexthop': '10.20.30.1'}],
+            'routes': routes + self._router.routes
         }
         self.network.update_router.assert_called_once_with(
             self._router, **attrs)
@@ -733,21 +733,31 @@ class TestSetRouter(TestRouter):
             self._router, **attrs)
         self.assertIsNone(result)
 
-    def test_set_route_no_route(self):
+    def test_set_route_overwrite_route(self):
+        _testrouter = network_fakes.FakeRouter.create_one_router(
+            {'routes': [{"destination": "10.0.0.2",
+                         "nexthop": "1.1.1.1"}]})
+        self.network.find_router = mock.Mock(return_value=_testrouter)
         arglist = [
-            self._router.name,
+            _testrouter.name,
             '--route', 'destination=10.20.30.0/24,gateway=10.20.30.1',
             '--no-route',
         ]
         verifylist = [
-            ('router', self._router.name),
+            ('router', _testrouter.name),
             ('routes', [{'destination': '10.20.30.0/24',
                          'gateway': '10.20.30.1'}]),
             ('no_route', True),
         ]
-
-        self.assertRaises(tests_utils.ParserException, self.check_parser,
-                          self.cmd, arglist, verifylist)
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        result = self.cmd.take_action(parsed_args)
+        attrs = {
+            'routes': [{'destination': '10.20.30.0/24',
+                        'nexthop': '10.20.30.1'}]
+        }
+        self.network.update_router.assert_called_once_with(
+            _testrouter, **attrs)
+        self.assertIsNone(result)
 
     def test_set_clear_routes(self):
         arglist = [
@@ -769,21 +779,31 @@ class TestSetRouter(TestRouter):
             self._router, **attrs)
         self.assertIsNone(result)
 
-    def test_set_route_clear_routes(self):
+    def test_overwrite_route_clear_routes(self):
+        _testrouter = network_fakes.FakeRouter.create_one_router(
+            {'routes': [{"destination": "10.0.0.2",
+                         "nexthop": "1.1.1.1"}]})
+        self.network.find_router = mock.Mock(return_value=_testrouter)
         arglist = [
-            self._router.name,
+            _testrouter.name,
             '--route', 'destination=10.20.30.0/24,gateway=10.20.30.1',
             '--clear-routes',
         ]
         verifylist = [
-            ('router', self._router.name),
+            ('router', _testrouter.name),
             ('routes', [{'destination': '10.20.30.0/24',
                          'gateway': '10.20.30.1'}]),
             ('clear_routes', True),
         ]
-
-        self.assertRaises(tests_utils.ParserException, self.check_parser,
-                          self.cmd, arglist, verifylist)
+        parsed_args = self.check_parser(self.cmd, arglist, verifylist)
+        result = self.cmd.take_action(parsed_args)
+        attrs = {
+            'routes': [{'destination': '10.20.30.0/24',
+                        'nexthop': '10.20.30.1'}]
+        }
+        self.network.update_router.assert_called_once_with(
+            _testrouter, **attrs)
+        self.assertIsNone(result)
 
     def test_set_nothing(self):
         arglist = [