Merge lp:~sil2100/ubuntu-system-image/server-redirect_in_scripts into lp:ubuntu-system-image/server

[Łukasz Zemczak]

Add proper checks for handling the per-device channel redirects in our scripts (import-image, copy-image, tag-image and set-phased-percentage). No unit tests as currently we have no unit-test support for scripts and the testability branch is yet to land.

Add proper checks for handling the per-device channel redirects in our scripts (import-image, copy-image, tag-image and set-phased-percentage). No unit tests as currently we have no unit-test support for scripts and the testability branch is yet to land.

These changes will be propagated to the testability branch, so that we don't miss the same checks [1].

[1] https://code.launchpad.net/~sil2100/ubuntu-system-image/server_script_testability

[Steve Langasek (vorlon)]

Please see inline comments.

review: Needs Fixing

[Łukasz Zemczak (sil2100)]

Ok, will remove the refactoring as mentioned. I simply didn't want to write tons of additional boilerplate code as I had to do channels = pub.list_channels() anyway since otherwise I wouldn't be able to fit in the 80 column size-limit for PEP8. So I thought I might as well just use it everywhere.

As for the redirect, see my inline reply - but the consensus is: we would generally want to support this as this would be one of the use-cases.

[Łukasz Zemczak (sil2100)]

Steve, with all due respect, can't I leave the small refectoring in place? This just introduces additional unnecessary work for me (and testing, since I need to make sure it's still working correctly and I didn't break anything with a semi-whole revert).

[lp:~sil2100/ubuntu-system-image/server-redirect_in_scripts]

293. By Łukasz Zemczak on 2016-06-24

Change output wording.

[Steve Langasek (vorlon)]

I was not asking for a revert, I was asking for the unrelated changes to be isolated into separate commits (which would require a rebase of this branch).

I've split out the refactor work into a commit and landed this separately; your branch can then land on top. This branch looks good now, and doesn't introduce testsuite regressions, so merging.

review: Approve

=== modified file 'bin/copy-image'
--- bin/copy-image	2015-11-12 12:47:12 +0000
+++ bin/copy-image	2016-06-24 11:06:41 +0000
@@ -78,38 +78,49 @@
     pub = tree.Tree(conf)
 
     # Do some checks
-    if args.source_channel not in pub.list_channels():
+    channels = pub.list_channels()
+    if args.source_channel not in channels:
         parser.error("Invalid source channel: %s" % args.source_channel)
 
-    if args.destination_channel not in pub.list_channels():
+    if args.destination_channel not in channels:
         parser.error("Invalid destination channel: %s" %
                      args.destination_channel)
 
-    if args.device not in pub.list_channels()[args.source_channel]['devices']:
+    if args.device not in channels[args.source_channel]['devices']:
         parser.error("Invalid device for source channel: %s" %
                      args.device)
 
     if args.device not in \
-            pub.list_channels()[args.destination_channel]['devices']:
+            channels[args.destination_channel]['devices']:
         parser.error("Invalid device for destination channel: %s" %
                      args.device)
 
-    if "alias" in pub.list_channels()[args.source_channel] and \
-            pub.list_channels()[args.source_channel]['alias'] \
+    if "alias" in channels[args.source_channel] and \
+            channels[args.source_channel]['alias'] \
             != args.source_channel:
         parser.error("Source channel is an alias.")
 
-    if "alias" in pub.list_channels()[args.destination_channel] and \
-            pub.list_channels()[args.destination_channel]['alias'] \
+    if "alias" in channels[args.destination_channel] and \
+            channels[args.destination_channel]['alias'] \
             != args.destination_channel:
         parser.error("Destination channel is an alias.")
 
-    if "redirect" in pub.list_channels()[args.source_channel]:
+    if "redirect" in channels[args.source_channel]:
         parser.error("Source channel is a redirect.")
 
-    if "redirect" in pub.list_channels()[args.destination_channel]:
+    if "redirect" in channels[args.destination_channel]:
         parser.error("Destination channel is a redirect.")
 
+    src_device = channels[args.source_channel]['devices'][args.device]
+    if "redirect" in src_device:
+        parser.error("Source device is a redirect. Use original channel "
+                     "%s instead." % src_device['redirect'])
+
+    dest_device = channels[args.destination_channel]['devices'][args.device]
+    if "redirect" in dest_device:
+        parser.error("Destination device is a redirect. Use original channel "
+                     "%s instead." % dest_device['redirect'])
+
     if args.tag and "," in args.tag:
         parser.error("Image tag cannot include the character ','.")
 

=== modified file 'bin/import-images'
--- bin/import-images	2015-03-11 20:55:00 +0000
+++ bin/import-images	2016-06-24 11:06:41 +0000
@@ -82,6 +82,12 @@
         for device_name in pub.list_channels()[channel_name]['devices']:
             logging.info("Processing device: %s" % device_name)
 
+            device_entry = \
+                pub.list_channels()[channel_name]['devices'][device_name]
+            if "redirect" in device_entry:
+                logging.info("Device is a redirect, not considering.")
+                continue
+
             device = pub.get_device(channel_name, device_name)
 
             # Extract last full version

=== modified file 'bin/set-phased-percentage'
--- bin/set-phased-percentage	2014-04-17 16:34:11 +0000
+++ bin/set-phased-percentage	2016-06-24 11:06:41 +0000
@@ -59,23 +59,29 @@
     pub = tree.Tree(conf)
 
     # Do some checks
-    if args.channel not in pub.list_channels():
+    channels = pub.list_channels()
+    if args.channel not in channels:
         parser.error("Invalid channel: %s" % args.channel)
 
-    if args.device not in pub.list_channels()[args.channel]['devices']:
+    if args.device not in channels[args.channel]['devices']:
         parser.error("Invalid device for source channel: %s" %
                      args.device)
 
     if args.percentage < 0 or args.percentage > 100:
         parser.error("Invalid value: %s" % args.percentage)
 
-    if "alias" in pub.list_channels()[args.channel] and \
-            pub.list_channels()[args.channel]['alias'] != args.channel:
+    if "alias" in channels[args.channel] and \
+            channels[args.channel]['alias'] != args.channel:
         parser.error("Channel is an alias.")
 
-    if "redirect" in pub.list_channels()[args.channel]:
+    if "redirect" in channels[args.channel]:
         parser.error("Channel is a redirect.")
 
+    target_device = channels[args.channel]['devices'][args.device]
+    if "redirect" in target_device:
+        parser.error("Target device is a redirect. Use original channel "
+                     "%s instead." % target_device['redirect'])
+
     dev = pub.get_device(args.channel, args.device)
     logging.info("Setting phased-percentage of '%s' to %s%%" %
                  (args.version, args.percentage))

=== modified file 'bin/tag-image'
--- bin/tag-image	2015-11-12 12:47:12 +0000
+++ bin/tag-image	2016-06-24 11:06:41 +0000
@@ -73,20 +73,26 @@
     pub = tree.Tree(conf)
 
     # Do some checks
-    if args.channel not in pub.list_channels():
+    channels = pub.list_channels()
+    if args.channel not in channels:
         parser.error("Invalid channel: %s" % args.channel)
 
-    if args.device not in pub.list_channels()[args.channel]['devices']:
+    if args.device not in channels[args.channel]['devices']:
         parser.error("Invalid device for channel: %s" % args.device)
 
-    if "alias" in pub.list_channels()[args.channel] and \
-            pub.list_channels()[args.channel]['alias'] \
+    if "alias" in channels[args.channel] and \
+            channels[args.channel]['alias'] \
             != args.channel:
         parser.error("Channel is an alias.")
 
-    if "redirect" in pub.list_channels()[args.channel]:
+    if "redirect" in channels[args.channel]:
         parser.error("Channel is a redirect.")
 
+    target_device = channels[args.channel]['devices'][args.device]
+    if "redirect" in target_device:
+        parser.error("Target device is a redirect. Use original channel "
+                     "%s instead." % target_device['redirect'])
+
     if "," in args.tag:
         parser.error("Image tag cannot include the character ','.")