Merge lp:~tobijk/livecd-rootfs/image-sets into lp:livecd-rootfs

Overall, I really like this. The code is well laid out, and the series concept should serve us well in the future due to its flexibility. Thanks!

(I have some inline comments on the Python script, but none of these relate to the overall behaviour of the script or this MP.)

Thanks a lot for the detailed review, @daniel-thewatkins! I think I have addressed everything you brought up in the new submission below. I also verified that the script works as Python3-only on Xenial.

Very nice approach and the Readme is very helpful. I would like some docstrings in the python script though as I know future me will need them ;p

Apart from that I am +1

Thank you @philroche, let me know if this works or if anything needs further clarification.

Is the intention that the execution order of hooks be identical once this change lands? That currently isn't the case and may lead to changes in the images. For example, the following generates:

./make-hooks --hooks-dir ../hooks vmdk vagrant
...
[HOOK] 005-ssh_authentication.chroot => chroot/ssh_authentication.chroot
...
[HOOK] 016-vagrant.binary => base/vagrant.binary

But prior to this change, these two hooks were in the opposite order:

042-vagrant.binary
052-ssh_authentication.chroot

Am I missing something (like are all the chroot hooks executed before the binary hooks)?

I also have a concern with defining the RESOURCE map in the make-hooks script (see inline comment).

Otherwise, I think this is a good approach to the problem. Thanks for working on it.

On Thu, Oct 25, 2018 at 03:47:11PM -0000, Francis Ginther wrote:
> Am I missing something (like are all the chroot hooks executed before the
> binary hooks)?

Yes, all chroot hooks are executed before the binary hooks.

> Am I missing something (like are all the chroot hooks executed before the
> binary hooks)?

Yes, exactly, chroot hooks and binary hooks are run in separate batches, it is not possible to interleave them. See the ACHTUNG note in the script header.

> > Am I missing something (like are all the chroot hooks executed before the
> > binary hooks)?
>
> Yes, exactly, chroot hooks and binary hooks are run in separate batches, it is
> not possible to interleave them. See the ACHTUNG note in the script header.

DOH! Thanks for already mentioning this in the comments.

My only remaining concern is the definition of the extra RESOURCES.

@fginther, nobody reads the comments ;) I think you having missed that piece of information is really just bad documentation. I added the same note to the README, where it really belongs.

Thank you for suggesting to take out the resource mappings. That really makes a lot of sense. Following your suggestion, the script now looks for base/resources.yaml and extra/resources.yaml.

I also updated

https://code.launchpad.net/~cloudware/livecd-rootfs/+git/cpc_packaging.extra/+merge/356266

to match these changes.

@daniel-thewatkins, @philroche, I believe I have addressed your concerns, as well. Please have another look.

Thanks for working on the resource map. I'm happy with this approach.

I do prefer to use yaml.safe_load() over yaml.load(), but I think it's use is fine here considering we're only loading our own code.

Thanks!

Happy to go merge with the two current approvals

Apologies for taking so long to give feedback on this. I knew that such a significant refactor was owed a thorough review, and I didn't find time to do so until now.

Comments inline.

Also revamped this taking into account @xnox's comments on

https://code.launchpad.net/~tobijk/livecd-rootfs/snap-coherence/+merge/358632

@vorlon, thank you so much for taking the time to review this in depth!

* There is now also a series file for the qcow2 target.

* It turned out that copying resources was not even necessary, because all scripts except one reference resources relative to their location. So I have removed that part completely.

I would like to keep the series file for the chroot, because:

* I find it more consistent to have all hooks placed/generated in the same way in the CPC subproject.

* We will want to augment the syntax of the series files in the future to also take into account arch, subarch and other attributes of the build that are currently handled by case statements in the scripts and this can also be useful for chroot customization,

* This is not on the table, right now, but I can imagine that we will find it useful to drop the ability to build different image target sets together and do per target set (and arch) chroot customizations. There can be advantages to this such being able to seed all target specific packages and snaps into the chroot instead of having to mount, fiddle and re-sparsen an image.

On Fri, Nov 30, 2018 at 01:01:15PM -0000, Tobias Koch wrote:
> * This is not on the table, right now, but I can imagine that we will find it useful to drop the ability to build different image target sets together and do per target set (and arch) chroot customizations. There can be advantages to this such being able to seed all target specific packages and snaps into the chroot instead of having to mount, fiddle and re-sparsen an image.

+1, we definitely want to leave the door open to be able to do this in
the future.

On Fri, Nov 30, 2018 at 02:27:22PM -0000, Dan Watkins wrote:
> On Fri, Nov 30, 2018 at 01:01:15PM -0000, Tobias Koch wrote:
> > * This is not on the table, right now, but I can imagine that we will
> > find it useful to drop the ability to build different image target
> > sets together and do per target set (and arch) chroot customizations.
> > There can be advantages to this such being able to seed all target
> > specific packages and snaps into the chroot instead of having to
> > mount, fiddle and re-sparsen an image.

> +1, we definitely want to leave the door open to be able to do this in
> the future.

I would argue, however, that until this actually *is* on the table, such
handling of chroot scripts is still just unnecessary complexity.

So is that a veto? Subsystem maintainers are +1.

> So is that a veto? Subsystem maintainers are +1.

Yes, I think I do have to insist that the changes to chroot hooks not be included here. Having to keep the list of chroot hooks in sync between the directory contents and the series file is error-prone, and should be avoided unless and until there is a reason to define a series that doesn't include all the chroot hooks.

@vorlon, I have made the necessary changes. Please check, if you are ok with this. Thanks!