Merge lp://staging/~boyang-niu/maas/enable_custom_images into lp://staging/~maas-committers/maas/trunk

Hi there Boyang,

Thanks for your submission, this is the first community-submitted patch, congratulations!

Just a few quick pre-review comments:
 1. You have conflicts; the template stuff has changed recently so that it all moved to separate template files
 2. Your solution to auto-enumerate releases won't work in a development environment as it hard-codes the path. This is also bad because it assumes packaging knowledge of installation locations. Ultimately we want to get rid of this enum and possibly store the available distros in a new database table, which will be populated by using distro-info.
 3. All functionality needs tests and this branch has no new tests. We can help you out with that though.
 4. I'm not sure if you talked to anyone about how to approach this but ideally we'd make this driven through the UI a bit more. We need to be able to specify custom images on a per-node basis as well as this global solution. Also I think it would be better to configure additional custom image locations via the API/UI. It would also be nice to have a way of making sure the images are consistent across different clusters (but that's not strictly necessary up front).

So at the moment this can't land at least because of (1), (2) and (3). (4) is quite a lot of work so it's something that can be added later by anyone.

Also it looks like your branch is based off the 1.3 release and this MP is targeting trunk. You need to apply your revisions to a trunk-based branch instead, so I shall mark this as "resubmit" because of that.

Julian,

Thanks for the feedback, I really appreciate the help I'm getting from
everyone!

As a possible solution to #2, could I take the TFTP root path from a config
file and use list_boot_images, as done in provisoningserver/boot_images.py
and provisioningserver/pxe/tftppath.py? This would keep the enum for the
moment but it's better than hardcoded paths (and it knows the same
information about available images as the region controller, so it enforces
certain standards).

The auto-enumerate was added in order to support per-node custom images
from the UI, since before it was impossible to select any distro other than
the hardcoded enum releases. I can implement the importing of a local file
for the custom image through UI and get it showing up on the UI menu; I'll
start working on that next.

Keep in touch!
Boyang

On Sun, Jun 16, 2013 at 10:47 PM, Julian Edwards <
<email address hidden>> wrote:

> Review: Resubmit
>
> Hi there Boyang,
>
> Thanks for your submission, this is the first community-submitted patch,
> congratulations!
>
> Just a few quick pre-review comments:
> 1. You have conflicts; the template stuff has changed recently so that it
> all moved to separate template files
> 2. Your solution to auto-enumerate releases won't work in a development
> environment as it hard-codes the path. This is also bad because it assumes
> packaging knowledge of installation locations. Ultimately we want to get
> rid of this enum and possibly store the available distros in a new database
> table, which will be populated by using distro-info.
> 3. All functionality needs tests and this branch has no new tests. We
> can help you out with that though.
> 4. I'm not sure if you talked to anyone about how to approach this but
> ideally we'd make this driven through the UI a bit more. We need to be
> able to specify custom images on a per-node basis as well as this global
> solution. Also I think it would be better to configure additional custom
> image locations via the API/UI. It would also be nice to have a way of
> making sure the images are consistent across different clusters (but that's
> not strictly necessary up front).
>
> So at the moment this can't land at least because of (1), (2) and (3).
> (4) is quite a lot of work so it's something that can be added later by
> anyone.
>
> Also it looks like your branch is based off the 1.3 release and this MP is
> targeting trunk. You need to apply your revisions to a trunk-based branch
> instead, so I shall mark this as "resubmit" because of that.
> --
>
> https://code.launchpad.net/~boyang-niu/maas/enable_custom_images/+merge/169074
> You are the owner of lp:~boyang-niu/maas/enable_custom_images.
>

On 18/06/13 03:58, Boyang Niu wrote:
> Julian,
>
> Thanks for the feedback, I really appreciate the help I'm getting from
> everyone!

Very welcome.

> As a possible solution to #2, could I take the TFTP root path from a config
> file and use list_boot_images, as done in provisoningserver/boot_images.py
> and provisioningserver/pxe/tftppath.py? This would keep the enum for the
> moment but it's better than hardcoded paths (and it knows the same
> information about available images as the region controller, so it enforces
> certain standards).

We had a chat about it and there's a couple of reasons why this approach
is fundamentally bad:

1. Are the files always present on the same system that needs to scan them?

2. Executing shell commands directly from the module is bad practice.
Even lint checkers will import that module, and I don't like the idea of
even "ls" running as a side effect. You can't even patch it out in a test.

> The auto-enumerate was added in order to support per-node custom images
> from the UI, since before it was impossible to select any distro other than
> the hardcoded enum releases. I can implement the importing of a local file
> for the custom image through UI and get it showing up on the UI menu; I'll
> start working on that next.

I think this is the best approach. We want to store the distro-info in
the database so that all parts of the system can access that data. The
enum should die in favour of a DB query.

Please, if you need any more help, just drop another email to the list.
 There are quite a few people who can offer advice (TZ-dependent if on
IRC) and help with UI design.

Hope that helps!