Merge "Fix router set --route option"

3 files changed, 39 insertions, 11 deletions

diff --git a/openstackclient/network/v2/router.py b/openstackclient/network/v2/router.py
index 56630a23..a32ab5ea 100644
--- a/openstackclient/network/v2/router.py
+++ b/openstackclient/network/v2/router.py
@@ -34,11 +34,20 @@ def _format_external_gateway_info(info):
         return ''
 
 
+def _format_routes(routes):
+    # Map the route keys to match --route option.
+    for route in routes:
+        if 'nexthop' in route:
+            route['gateway'] = route.pop('nexthop')
+    return utils.format_list_of_dicts(routes)
+
+
 _formatters = {
     'admin_state_up': _format_admin_state,
     'external_gateway_info': _format_external_gateway_info,
     'availability_zones': utils.format_list,
     'availability_zone_hints': utils.format_list,
+    'routes': _format_routes,
 }
 
 
@@ -67,11 +76,6 @@ def _get_attrs(client_manager, parsed_args):
             and parsed_args.availability_zone_hints is not None):
         attrs['availability_zone_hints'] = parsed_args.availability_zone_hints
 
-    if 'clear_routes' in parsed_args and parsed_args.clear_routes:
-        attrs['routes'] = []
-    elif 'routes' in parsed_args and parsed_args.routes is not None:
-        attrs['routes'] = parsed_args.routes
-
     # "router set" command doesn't support setting project.
     if 'project' in parsed_args and parsed_args.project is not None:
         identity_client = client_manager.identity
@@ -393,7 +397,19 @@ class SetRouter(command.Command):
         client = self.app.client_manager.network
         obj = client.find_router(parsed_args.router, ignore_missing=False)
 
+        # Get the common attributes.
         attrs = _get_attrs(self.app.client_manager, parsed_args)
+
+        # Get the route attributes.
+        if parsed_args.clear_routes:
+            attrs['routes'] = []
+        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.
+            for route in parsed_args.routes:
+                route['nexthop'] = route.pop('gateway')
+            attrs['routes'] = obj.routes + parsed_args.routes
+
         if attrs == {}:
             msg = "Nothing specified to be set"
             raise exceptions.CommandError(msg)
diff --git a/openstackclient/tests/network/v2/test_router.py b/openstackclient/tests/network/v2/test_router.py
index e8041498..e3f32e4a 100644
--- a/openstackclient/tests/network/v2/test_router.py
+++ b/openstackclient/tests/network/v2/test_router.py
@@ -138,7 +138,7 @@ class TestCreateRouter(TestRouter):
         new_router.id,
         new_router.name,
         new_router.tenant_id,
-        new_router.routes,
+        router._format_routes(new_router.routes),
         new_router.status,
     )
 
@@ -268,7 +268,7 @@ class TestListRouter(TestRouter):
         r = routers[i]
         data_long.append(
             data[i] + (
-                r.routes,
+                router._format_routes(r.routes),
                 router._format_external_gateway_info(r.external_gateway_info),
                 osc_utils.format_list(r.availability_zones),
             )
@@ -399,7 +399,10 @@ class TestRemoveSubnetFromRouter(TestRouter):
 class TestSetRouter(TestRouter):
 
     # The router to set.
-    _router = network_fakes.FakeRouter.create_one_router()
+    _default_route = {'destination': '10.20.20.0/24', 'nexthop': '10.20.30.1'}
+    _router = network_fakes.FakeRouter.create_one_router(
+        attrs={'routes': [_default_route]}
+    )
 
     def setUp(self):
         super(TestSetRouter, self).setUp()
@@ -491,8 +494,8 @@ class TestSetRouter(TestRouter):
         result = self.cmd.take_action(parsed_args)
 
         attrs = {
-            'routes': [{'destination': '10.20.30.0/24',
-                        'gateway': '10.20.30.1'}],
+            'routes': self._router.routes + [{'destination': '10.20.30.0/24',
+                                             'nexthop': '10.20.30.1'}],
         }
         self.network.update_router.assert_called_once_with(
             self._router, **attrs)
@@ -572,7 +575,7 @@ class TestShowRouter(TestRouter):
         _router.id,
         _router.name,
         _router.tenant_id,
-        _router.routes,
+        router._format_routes(_router.routes),
         _router.status,
     )
 
diff --git a/releasenotes/notes/bug-1564460-ab7ad35c02392cb4.yaml b/releasenotes/notes/bug-1564460-ab7ad35c02392cb4.yaml
new file mode 100644
index 00000000..54b9bdd7
--- /dev/null
+++ b/releasenotes/notes/bug-1564460-ab7ad35c02392cb4.yaml
@@ -0,0 +1,9 @@
+---
+fixes:
+  - Fixed the ``--route`` option on the ``router set`` command
+    which did not properly format the new routes to set resulting
+    in a ``Bad Request`` error. In addition, the ``router create``,
+    ``router list`` and ``router show`` command output for routes
+    was fixed to improve readability and to align with the
+    ``--route`` option on the ``router set`` command.
+    [Bug `1564460 <https://bugs.launchpad.net/bugs/1564460>`_]