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

[Mark Kanda]

On 10/5/2018 12:39 PM, Cole Robinson wrote:
> 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
> 
Okay.

Thanks Cole,

-Mark