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

[Mark Kanda]

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.

Thanks,

-Mark