[pve-devel] [PATCH pve-manager] Add "SSD" property to IDE, SATA, and SCSI hard drives

[Thomas Lamprecht]

On 11/6/18 5:45 PM, Nick Chevsky wrote:
> On Tue, Nov 6, 2018 at 4:28 AM Thomas Lamprecht <t.lamprecht at proxmox.com>
> wrote:
> 
>> On 11/6/18 10:19 AM, Dominik Csapak wrote:
>>> patch looks good, but maybe we want to put this in the advanced options
>> instead ? do you have any strong opinion on that @thomas?
>>
>> Sounds OK, I guess - no strong feeling either way...
>>
> 
> I decided on the top section because:
> 
>    1. Rotational vs. solid-state is a major defining property of a drive;
>    if you were shopping for a physical one, this would probably be your first
>    filter.
>    2. This property affects how the drive is presented to the guest, unlike
>    most of the advanced properties which are transparent to the guest.
>    3. Putting this in the advanced section would've meant adding a new row
>    to the already crowded bottom half of the dialog box.
> 
> That said, I don't mind moving it at all—just let me know your final
> thoughts.

In general I don't mind either way, your arguments sound OK and I
do not find it intrusive, even OK, so it's Dominik's call.

But two things:

1) I also have the checkbox enabled for SCSI with virtio-scsi controller,
   and it's not needed in that case, AFAICT.

2) I'd add a boxLabel with gettext('autodetected') (or similar) when it gets
   disabled, so that user know that they just do not need to set it in those
   cases, not that someone thinks that the virtio stuff is inferior because it
   can't even handle SSDs properly :) Somewhat similar in principle to:
   https://pve.proxmox.com/pipermail/pve-devel/2018-November/034260.html

> 
>> But in general an sentence/note in qm.adoc would be nice regarding this,
>> SSD may be a bit confusing (see also below for another comment) else.
>>
> 
> I do have a pve-docs patch coming up; I wanted to wait until this one was
> merged in case changes were requested that would affect the documentation.
> 

Awesome!

> 
>>>> +        xtype: 'proxmoxcheckbox',
>>>> +        fieldLabel: gettext('SSD'),
>>
>> remove the gettext, SSD is verbatim OK and I cannot imagine translations
>> of it.
>>
> 
> Done! I'll send an updated patch once you guys confirm your preference on
> the location of the checkbox.
> 

OK, much thanks!