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

[Mark Kanda]

On 10/5/2018 2:16 PM, Mark Kanda wrote:
> 
> 
> 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
>>

BTW, I did review and test that patch (and agree that it improves 
things).. So, if it helps, please add:

Reviewed-by: Mark Kanda <mark.kanda at oracle.com>

Thanks,

-Mark