Merge lp:~mariogrip/ubuntu-system-image/device-alias into lp:ubuntu-system-image/server

[Marius Gripsgard ]

Add support for Device alias

[Steve Langasek (vorlon)]

This doesn't look like a maintainable design to me. You are attaching device aliases to individual channels. This means that the same device alias, on two different channels, can point to two different devices. And if a device is removed from a channel, there's nothing in this code that ensures the alias is removed with it; so a channel might have dangling aliases to a device that has been removed from that channel.

I would prefer to see device aliases handled globally, not per channel.

review: Disapprove

[Łukasz Zemczak (sil2100)]

First of all, thanks for the proposition!

Sadly I must agree with Steve here, although a bit less strictly. Even though handling aliases per-channel might look like a logical thing because, well, we have per-channel devices, there's one key thing to remember here. Even though devices are essentially defined per-channel, any device is global - i.e. if you copy an image for device 'mako' from channel foo to channel bar, system-image assumes that the device 'mako' is the same as the one in channel foo. If we let defining aliases per channel this might become a bit hairy and confusing - one alias could point to one device in one channel and to another one in a different one, but those devices could potentially different as it's just an 'alias'.

In this particular case I would also opt for a global approach instead.

But continuing the review beyond the design considerations: first of all, remember we would need tests for this implemented as well. The tree parts are well unit-testable so we would need tests for all the new functionality added.
Another thing: for internal configuration in system-image we try to follow the dash-separated form for naming, not camelCase - so deviceAlias would be better off being device-alias instead.

review: Needs Fixing

Unmerged revisions

284. By Marius Gripsgard  on 2016-01-19

Add support for Device alias

=== modified file 'lib/systemimage/tree.py'
--- lib/systemimage/tree.py	2015-10-27 15:25:38 +0000
+++ lib/systemimage/tree.py	2016-01-27 18:19:19 +0000
@@ -306,6 +306,54 @@
 
         return True
 
+    def change_device_alias(self, channel_name, device_name, new_device_name, target_device_name):
+        """
+            Change a device alias entry
+        """
+
+        with channels_json(self.config, self.indexpath, True) as channels:
+            if channel_name not in channels:
+                raise KeyError("Couldn't find channel: %s" % channel_name)
+
+            if target_device_name not in channels[channel_name]['devices']:
+                raise KeyError("Couldn't find target device: %s" % target_device_name)
+
+            if device_name not in channels[channel_name]['deviceAlias']:
+                raise KeyError("Device alias does not exists: %s" % device_name)
+
+
+            channels[channel_name]['deviceAlias'].pop(device_name)
+
+            alias = {}
+            alias["device"] = target_device_name
+            channels[channel_name]['deviceAlias'][new_device_name] = alias
+
+        return True
+
+    def create_device_alias(self, channel_name, device_name, target_device_name):
+        """
+            Creates a new device alias entry
+        """
+
+        with channels_json(self.config, self.indexpath, True) as channels:
+            if channel_name not in channels:
+                raise KeyError("Couldn't find channel: %s" % channel_name)
+
+            if target_device_name not in channels[channel_name]['devices']:
+                raise KeyError("Couldn't find target device: %s" % target_device_name)
+
+            if "deviceAlias" in channels[channel_name]:
+                if device_name in channels[channel_name]['deviceAlias']:
+                    raise KeyError("Device alias already exists: %s" % device_name)
+            else:
+                channels[channel_name]['deviceAlias'] = {}
+
+            alias = {}
+            alias["device"] = target_device_name
+            channels[channel_name]['deviceAlias'][device_name] = alias
+
+        return True
+
     def create_device(self, channel_name, device_name, keyring_path=None):
         """
             Creates a new device entry in the tree.
@@ -516,6 +564,25 @@
 
         return True
 
+    def remove_device_alias(self, channel_name, device_name):
+        """
+            Removes a device alias entry
+        """
+
+        with channels_json(self.config, self.indexpath, True) as channels:
+            if channel_name not in channels:
+                raise KeyError("Couldn't find channel: %s" % channel_name)
+
+            if device_name not in channels[channel_name]['deviceAlias']:
+                raise KeyError("Couldn't find device alias: %s" % device_name)
+
+            channels[channel_name]['deviceAlias'].pop(device_name)
+
+            if not channels[channel_name]['deviceAlias']:
+                channels[channel_name].pop('deviceAlias')
+
+        return True
+
     def remove_device(self, channel_name, device_name):
         """
             Remove a device and everything it contains.