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

Proposed by Marius Gripsgard 
Status: Needs review
Proposed branch: lp://staging/~mariogrip/ubuntu-system-image/device-alias
Merge into: lp://staging/ubuntu-system-image/server
Diff against target: 84 lines (+67/-0)
1 file modified
lib/systemimage/tree.py (+67/-0)
To merge this branch: bzr merge lp://staging/~mariogrip/ubuntu-system-image/device-alias
Reviewer Review Type Date Requested Status
Łukasz Zemczak (community) Needs Fixing
Steve Langasek (community) Disapprove
Review via email: mp+284169@code.staging.launchpad.net

Description of the change

Add support for Device alias

To post a comment you must log in.
Revision history for this message
Steve Langasek (vorlon) wrote :

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
Revision history for this message
Łukasz Zemczak (sil2100) wrote :

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 

Add support for Device alias

Preview Diff

[H/L] Next/Prev Comment, [J/K] Next/Prev File, [N/P] Next/Prev Hunk
The diff is not available at this time. You can reload the page or download it.

Subscribers

People subscribed via source and target branches