[Kimchi-devel] [PATCH] [Kimchi 0/3] Issue #372: Add support to netboot installation.

[Paulo Ricardo Paz Vital]

On 02/17/2016 08:47 AM, Daniel Henrique Barboza wrote:
> Reviewed-by: Daniel Barboza <dhbarboza82 at gmail.com>
> 
> 
> Minor comments to be looked at in the case you end up sending a v2
> (not worth sending a v2 because of them alone):
> 
> - copyright format is wrong in new file xmlutils/bootorder.py. It must
> be 'Copyright IBM Corp, 2016'. This copyright will be fixed by WoK's
> copyright script when it is deployed in Kimchi, so don't need to rush.

I did a copy&paste of the header from a different file from xmlutils.
Since the new script will fix all files, I don't think it's necessary a
V2 for this.

> 
> - I am not fond of comments when the code is clear enough. This is
> the case in your code below:
> 
> 
> +        # Add information of CD-ROM device only if not setup to netboot.
> +        # Also, setup the correct boot order
> +        if params['cdrom'] == 'netboot':
> +            params['boot_order'] = get_bootorder_xml(network=True)
> 
> 
> 
> The thing about this kind of comment is that it doesn't add up anything
> (the code is easy to understand) but then we'll need to keep the comment
> updated if the code logic changes for some reason.

I agree with you that comments for simple code is not necessary.
However, the comment is pertinent for all if-else block that is setting
the information related to cd-rom - the else part is more robust than
the if in this case.

In addition, all other information process in the method has comments to
inform what kind of information is being processed. Just added one more
to keep the understand of the method easier to anyone.

> 
> 
> 
> Daniel
> 
> 
> 
> 
> On 02/16/2016 07:40 PM, pvital at linux.vnet.ibm.com wrote:
>> From: Paulo Vital <pvital at linux.vnet.ibm.com>
>>
>> This patchset adds support in backend to create templates and guests
>> to netboot
>> without setting a cdrom path or URL as image to install. Once created
>> a guest
>> to boot via network, the guest will request DHCP/TFTP/(NFS/HTTP/FTP)
>> network
>> installation servers to download the configured images and start the
>> install.
>>
>> To test, use the curl commands:
>>
>> curl -k -u test -H "Content-Type: application/json" -H \
>> "Accept: application/json"
>> 'https://localhost:8001/plugins/kimchi/templates' \
>> -X POST -d '{"name": "test-netboot"}'
>> Enter host password for user 'test':
>> {
>>    "cpu_info":{
>>      "maxvcpus":1,
>>      "vcpus":1
>>    },
>>    "graphics":{
>>      "type":"vnc",
>>      "listen":"127.0.0.1"
>>    },
>>    "cdrom":"netboot",
>>    "networks":[
>>      "default"
>>    ],
>>    "icon":"plugins/kimchi/images/icon-vm.png",
>>    "os_distro":"unknown",
>>    "name":"test-netboot",
>>    "disks":[
>>      {
>>        "index":0,
>>        "format":"qcow2",
>>        "pool":{
>>          "type":"dir",
>>          "name":"/plugins/kimchi/storagepools/default"
>>        },
>>        "size":10
>>      }
>>    ],
>>    "invalid":{},
>>    "os_version":"unknown",
>>    "memory":1024,
>>    "folder":[]
>> }
>>
>> curl -k -u test -H "Content-Type: application/json" -H \
>> "Accept: application/json" 'https://localhost:8001/plugins/kimchi/vms'
>> -X POST \
>> -d
>> '{"name":"1netboot-test","template":"/plugins/kimchi/templates/test-netboot"}'
>>
>> Enter host password for user 'test':
>> {
>>    "status":"running",
>>    "message":"OK",
>>    "id":"1",
>>    "target_uri":"/plugins/kimchi/vms/1netboot-test"
>> }
>>
>>
>> Paulo Vital (3):
>>    Add support to create templates without ISO image.
>>    Add support to create guests without ISO image.
>>    Update test cases to support netboot.
>>
>>   model/templates.py       | 35 ++++++++++++++++++++++-------------
>>   tests/test_template.py   | 18 ++++++++++++++----
>>   tests/test_vmtemplate.py | 16 ++++++++++++++++
>>   vmtemplate.py            | 29 ++++++++++++++++++-----------
>>   xmlutils/bootorder.py    | 44
>> ++++++++++++++++++++++++++++++++++++++++++++
>>   5 files changed, 114 insertions(+), 28 deletions(-)
>>   create mode 100644 xmlutils/bootorder.py
>>
>> -- 
>> 2.5.0
>>
>> _______________________________________________
>> Kimchi-devel mailing list
>> Kimchi-devel at ovirt.org
>> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
> 
> _______________________________________________
> Kimchi-devel mailing list
> Kimchi-devel at ovirt.org
> http://lists.ovirt.org/mailman/listinfo/kimchi-devel
>