On 27/04/2015 17:16, Rodrigo Trujillo wrote:

On 04/27/2015 12:13 PM, Aline Manera wrote:

...

On 17/04/2015 00:05, Rodrigo Trujillo wrote:

...

This patch adds a new field (Disk Format) in Storage tab in Template edit window. Users will then be allowed to select any disk format supported by Libvirt and Kimchi backend. The default disk format is qcow2 (first option), so, for compatibility, if the template register in objecstore does not contain the disk format information, the new field is going to set 'qcow2' automatically. For iscsi storagepools, the new field is going to be disabled and the format is going to be set automatically as 'raw', just like the backend behaves currently, avoiding errors.

Signed-off-by: Rodrigo Trujillo <rodrigo.trujillo@linux.vnet.ibm.com> --- ui/css/theme-default/template-edit.css | 8 ++++-- ui/js/src/kimchi.template_edit_main.js | 52 ++++++++++++++++++++++++++++++---- ui/pages/template-edit.html.tmpl | 16 +++++++++++ 3 files changed, 68 insertions(+), 8 deletions(-)

diff --git a/ui/css/theme-default/template-edit.css b/ui/css/theme-default/template-edit.css index 7abee7c..ed9f02c 100644 --- a/ui/css/theme-default/template-edit.css +++ b/ui/css/theme-default/template-edit.css @@ -95,7 +95,7 @@

#edit-template-tabs .template-storage-cell{ display: inline-block; - width: 230px; + width: 180px; }

#edit-template-tabs .template-storage-cell label { @@ -114,7 +114,11 @@ }

#form-template-storage .template-tab-body .template-storage-name { - width: 220px; + width: 170px; +} + +#form-template-storage .template-tab-body .template-storage-disk-format { + width: 160px; }

#edit-template-tabs .template-tab-body input[readonly] { diff --git a/ui/js/src/kimchi.template_edit_main.js b/ui/js/src/kimchi.template_edit_main.js index 85f76cd..6029a95 100644 --- a/ui/js/src/kimchi.template_edit_main.js +++ b/ui/js/src/kimchi.template_edit_main.js @@ -67,7 +67,7 @@ kimchi.template_edit_main = function() { $('.template-tab-body', '#form-template-storage').append(nodeStorage); var storageOptions = ''; var scsiOptions = ''; - $('select', '#form-template-storage').find('option').remove(); + $('select:first', '#form-template-storage').find('option').remove();

It is really dangerous to do that (assuming that will always be the first select element in the form) I suggest to add an id to the select elements in the form to distinguish them and then use the id to get the element.

We had some conversations last week and I was coding the support to multiple disks together with this patch, then you asked me to stop and do later. So, this patch considered only 1 disk per template. In this case the 'first' and 'last' selection work fine.

I am reviewing only this patch I want to have *this patch* in good shape to have it merged. If you want to make it dependent of other patch, it is up to you, but in that case this patch will be on hold.

...

...

$.each(result, function(index, storageEntities) { if((storageEntities.state === 'active') && (storageEntities.type != 'kimchi-iso')) { if(storageEntities.type === 'iscsi' || storageEntities.type === 'scsi') { @@ -77,7 +77,7 @@ kimchi.template_edit_main = function() { var isSlected = tmpPath === thisName ? ' selected' : ''; scsiOptions += '<option' + isSlected + '>' + tmpPath + '</option>'; }); - $('select', '#form-template-storage').append(scsiOptions); + $('select:first', '#form-template-storage').append(scsiOptions);

Same as commented above, Answer above

Same I said before.

...

...

}, function() {}); } else { var isSlected = storageEntities.name === thisName ? ' selected' : ''; @@ -85,8 +85,20 @@ kimchi.template_edit_main = function() { } } }); - $('select', '#form-template-storage').append(storageOptions); - $('select', '#form-template-storage').change(function() { + $('select:first', '#form-template-storage').append(storageOptions); + + // Set disk format + $('select:last option', '#form-template-storage').each(function() { + if ($(this).text() == storageData.storageDiskFormat) { + $(this).prop('selected', true); + } + }); + + $('select:last', '#form-template-storage').change(function() { + $('.template-storage-disk-format').val($(this).val()); + }); + + $('select:first', '#form-template-storage').change(function() { var selectedItem = $(this).parent().parent(); var tempStorageNameFull = $(this).val(); var tempName = tempStorageNameFull.split('/'); @@ -99,9 +111,25 @@ kimchi.template_edit_main = function() { kimchi.getStoragePoolVolume(tempStorageName, tempName[tempName.length-1], function(info) { volSize = info.capacity / Math.pow(1024, 3); $('.template-storage-disk', selectedItem).attr('readonly', true).val(volSize);

...

+ $('select:last option', selectedItem).each(function() { + this.selected = (this.text == 'raw'); + });

Why do you need a loop to set the right value on select element?

Because the "option" elements only have the text.

I don't think you need a loop to do that: https://api.jquery.com/attribute-equals-selector/

...

...

+ $('select:last', selectedItem).prop('disabled', true).change(); }); + } else if (tempType === 'logical') { + $('.template-storage-disk', selectedItem).attr('readonly', false);

...

+ $('select:last option', selectedItem).each(function() { + this.selected = (this.text == 'raw'); + });

Same question as above

Because the "option" elements only have the text.

Same as above.

...

...

+ $('select:last', selectedItem).prop('disabled', true).change(); } else { $('.template-storage-disk', selectedItem).attr('readonly', false);

...

+ if ($('select:last', selectedItem).prop('disabled') == true) {

Does this 'if' statement is really needed? I mean, the last case if for any read-only pool which means the format drop box will be always enabled. Do you see an user case where it should be disabled?

This if statement do the following: Suppose you have 2 storage pools of DIR type (dir1 and dir2), and a storage pool of iSCSI type (iscsi-pool). When you select 'iscsi-pool' it is going to set disk format as RAW, and disable it. When you select a dir pool, 'dir1' for instance, it is going to enable and set disk format selection as 'qcow2' which is the default format. Now, suppose you have 'dir1' selected and selects format as "vmdk". Then you change the pool to 'dir2', it is NOT going to change the disk format to 'qcow2', keeping the previous selected value. In other words, it avoids change the format to qcow2 every time, it only changes the format when changing from a storage pool with fixed format type ('raw') to a storage pool that accepts multiple formats.

Got it. Thanks.

...

...

+ $('select:last option', selectedItem).each(function() { + this.selected = (this.text == 'qcow2'); + });

Same question about the loop to set the right value on select element

...

+ $('select:last', selectedItem).prop('disabled', false).change(); + } } }); }); @@ -120,7 +148,8 @@ kimchi.template_edit_main = function() { editMode : 'hide', storageName : defaultPool, storageType : defaultType, - storageDisk : diskEntities.size + storageDisk : diskEntities.size, + storageDiskFormat : diskEntities.format ? diskEntities.format : 'qcow2' }

if (diskEntities.volume) { @@ -131,7 +160,17 @@ kimchi.template_edit_main = function() { nodeData.storageDisk = volSize; addStorageItem(nodeData); $('.template-storage-disk').attr('readonly', true); + $('select:last option', '#form-template-storage').each(function() { + this.selected = (this.text == 'raw'); + }); + $('select:last', '#form-template-storage').prop('disabled', true).change(); + }); + } else if (defaultType === 'logical') { + addStorageItem(storageNodeData); + $('select:last option', '#form-template-storage').each(function() { + this.selected = (this.text == 'raw'); }); + $('select:last', '#form-template-storage').prop('disabled', true).change(); } else { addStorageItem(storageNodeData); } @@ -271,7 +310,8 @@ kimchi.template_edit_main = function() { origDisks[0]['volume'] && delete origDisks[0]['volume']; origDisks[0].size = Number($('.template-storage-disk', tmpItem).val()); } - data[field] = origDisks; + origDisks[0].format = $('.template-storage-disk-format', tmpItem).val(); + data[field] = origDisks; } else if (field == 'graphics') { var type = $('#form-template-general [name="' + field + '"]').val(); diff --git a/ui/pages/template-edit.html.tmpl b/ui/pages/template-edit.html.tmpl index c7832c9..e64a30f 100644 --- a/ui/pages/template-edit.html.tmpl +++ b/ui/pages/template-edit.html.tmpl @@ -104,6 +104,7 @@ <span class="template-storage-cell">$_("Storage Pool")</span> <span class="template-storage-cell">$_("Type")</span> <span class="template-storage-cell">$_("Disk(GB)")</span> + <span class="template-storage-cell">$_("Disk Format")</span> <button type="button" id="template-edit-storage-add-button" class="action-area"></button> </div> <div class="template-tab-body"> @@ -160,6 +161,21 @@ <span class="template-storage-cell"> <input class="template-storage-disk" value={storageDisk} type="text" /> </span> + <span class="template-storage-cell"> + <input class="template-storage-disk-format" value={storageDiskFormat} type="text" style="display:none" /> + <select> + <option>qcow2</option> + <option>raw</option> + <option>bochs</option> + <option>cloop</option> + <option>cow</option> + <option>dmg</option> + <option>qcow</option> + <option>qed</option> + <option>vmdk</option> + <option>vpc</option> + </select> + </span> </div> </script> <script id="template-interface-tmpl" type="text/html">