From cfc22cd23be28d3bbeab5825b4363f3001cc25e8 Mon Sep 17 00:00:00 2001 From: Joey Hess Date: Sat, 13 Oct 2018 18:04:11 -0400 Subject: code review --- ...ent_3_8dd7f3dd8c80fda70233e395da2204b2._comment | 33 ++++++++++++++++++++++ 1 file changed, 33 insertions(+) create mode 100644 doc/forum/support_for_non-bootable_disk_images/comment_3_8dd7f3dd8c80fda70233e395da2204b2._comment diff --git a/doc/forum/support_for_non-bootable_disk_images/comment_3_8dd7f3dd8c80fda70233e395da2204b2._comment b/doc/forum/support_for_non-bootable_disk_images/comment_3_8dd7f3dd8c80fda70233e395da2204b2._comment new file mode 100644 index 00000000..d1761e51 --- /dev/null +++ b/doc/forum/support_for_non-bootable_disk_images/comment_3_8dd7f3dd8c80fda70233e395da2204b2._comment @@ -0,0 +1,33 @@ +[[!comment format=mdwn + username="joey" + subject="""comment 3""" + date="2018-10-13T21:41:25Z" + content=""" +Code^Wwhitespace review: + +* I noticed some places were using spaces for indentation; + please use tabs in propellor. +* In "module Propellor.Property.DirectBoot(installed)' + there should be a space after the name of the module. +* Needs comments explaining what properties are for. + +Naming ideas: Basically this is using qemu as the bootloader, rather than +going through an (emulated) BIOS to start a bootloader. So I'm thinking +names like QemuBootloader or NoBootloader, or NoBIOS. Don't want to +bikeshed this too hard, it would be ok to keep the DirectBoot name, but +I think Propellor.Property.DirectBoot at least needs a comment explaining what it's +for, it would be confusing for a propellor user to stumble across that +module without context. + +Your idea to copy the kernel and initrd out of the image so qemu can use +them seems to point toward having a Property that gets one of these images +booted up using qemu. And then the QemuBootloader name would make a lot of +sense, because it would allow for later expansion to other emulators. Not +that you have to build such a thing, but it's worth considering that someone +may later want to. + +(In fact I could use such a thing, but I don't know how I'd want it to +work. Should propellor only use the chroot for initial image build, and +then ssh into the booted VM and run propellor in there when there are +config updates? Or restart the VM when the image is changed?) +"""]] -- cgit v1.2.3