[virt-tools-list] virt-install: Add warn if 'single-use' options are defined multiple times?

[Cole Robinson]

On 10/04/2018 04:41 PM, Mark Kanda wrote:
> On 10/4/2018 11:55 AM, Cole Robinson wrote:
>> On 10/04/2018 12:29 PM, Mark Kanda wrote:
>>>
>>>
>>> On 10/1/2018 12:16 PM, Cole Robinson wrote:
>>>> On 09/14/2018 04:28 PM, Mark Kanda wrote:
>>>>> Hi all,
>>>>>
>>>>> I recently discovered, for 'single-use' command line options, 
>>>>> virt-install silently ignores all but the last definition. For 
>>>>> example, if the command line has '--memory X' and later '--memory 
>>>>> Y', then 'X' is silently ignored.
>>>>>
>>>>> I think virt-install should warn the user if 'single use' options 
>>>>> are specified multiple times, and I'd like to implement a fix.
>>>>>
>>>>> However, before I implement such a fix, I'd like to know if this is 
>>>>> 'by design', or if anyone would otherwise object to such a check..
>>>>>
>>>>
>>>> Sorry for the late response. This isn't really intentional and more 
>>>> just a side effect of how our argparse options are initialized. I 
>>>> hacked up the attached diff that will essentially merge all ex. 
>>>> --memory options,
>>>> so --memory 123,maxmem=456 --memory 555 will be equivalent to 
>>>> --memory 555,maxmem=456. Is that fine for you needs?
>>>>
>>>> I think it's a bit nicer and has benefits if you want to pass in an 
>>>> extra option to a virt-install script or similar
>>>>
>>>
>>> Thanks Cole,
>>>
>>> Yes, this does look better. However, IMO, the user should be warned 
>>> if they do something like '--memory 123,maxmem=456 --memory 555' 
>>> because it's not clear if they want '123' or '555'.
>>>
>>> I was thinking of defining and using a custom argpase action to warn 
>>> the user - see attached patch (incomplete - likely missing many 
>>> single-use args).
>>>
>>> What do you think of this approach?
>>>
>>
>> I dunno... are there any other command line tools that warn in this case?
> 
> I don't know. In addition, I was a bit surprised to find python argparse 
> doesn't have some facility to prevent options from being specified 
> multiple times. Perhaps this lends credence to idea that this isn't 
> something other command line tools typically do..
> 
>> I can't think of any offhand. The warning may save users a bit of time 
>> if they accidentally specify an option twice and get unexpected 
>> results, but it will also be noise for other users.
>>
>> For example, I know it's a common pattern for people to treat 
>> virt-install effectively as 'write once' and stuff a working 
>> invocation in script. So say a user has a simple script like
>>
>> #!/bin/bash
>> virt-install --name test --memory 2048 --disk size=10 --cpu host-model 
>> "$@"
>>
>> They invoke it like 'myscript --location $URL' or similar. But later 
>> on, or for some invocations, they want to specify more memory 
>> (myscript --memory 4096...) or a different cpu config (myscript --cpu 
>> host-passthrough). Now they see a warning even though it's entirely 
>> intended. So it's not a total win either way IMO,
>>
>> Can you describe the case where this bit you? Maybe it will help me 
>> understand better
>>
> 
> Similar to what you mentioned, we stumbled upon this when we made a 
> mistake in a script which invokes 'virt-install'. We had a 'single-use' 
> option defined twice, and were left a bit puzzled as to why it wasn't 
> working as intended. TBH, as you can probably imagine, it really wasn't 
> a big deal to chase down and fix. However, I still believe it would be 
> good to warn users in this situation.
> 
> That being said, I don't feel strongly about it, so if there is 
> resistance to adding such a thing, I'm okay with that.
> 

I appreciate the contribution but my feeling is that it's questionable 
benefit to justify adding code+tests for it, so i'll just push the patch 
I attached to my first mail

Thanks,
Cole