[Freeipa-devel] [PATCH 0078] Enable QR code display by default in otptoken-add

[Petr Viktorin]

On 11/13/2014 04:40 PM, Petr Vobornik wrote:
> On 13.11.2014 16:19, Nathaniel McCallum wrote:
>> On Thu, 2014-11-13 at 16:13 +0100, Petr Vobornik wrote:
>>> On 13.11.2014 16:00, Nathaniel McCallum wrote:
>>>> On Thu, 2014-11-13 at 13:17 +0100, Martin Kosek wrote:
>>>>> On 11/13/2014 07:53 AM, Nathaniel McCallum wrote:
>>>>>> On Thu, 2014-11-06 at 15:33 -0500, Nathaniel McCallum wrote:
>>>>>>> This is possible because python-qrcode's output now fits in a
>>>>>>> standard
>>>>>>> terminal. Also, update ipa-otp-import and otptoken-add-yubikey to
>>>>>>> disable QR code output as it doesn't make sense in these contexts.
>>>>>>>
>>>>>>> https://fedorahosted.org/freeipa/ticket/4703
>>>>>>
>>>>>> I removed the period from the doc string to maintain consistency with
>>>>>> the rest of the plugin.
>>>>>>
>>>>>> Nathaniel
>>>>>>
>>>>>
>>>>> I am not reviewing, just curious. What is the purpose of --qrcode
>>>>> option with
>>>>> default=True?
>>>>>
>>>>>        takes_options = LDAPCreate.takes_options + (
>>>>> -        Flag('qrcode?', label=_('Display QR code')),
>>>>> +        Flag('qrcode', label=_('Display QR code'), default=True),
>>>>>        )
>>>>>
>>>>> How can user turn it off? Both passing no option and passing
>>>>> --qrcode will get
>>>>> the same result, no? :-)
>>>>>
>>>>> Shouldn't we instead change --qrcode to --no-qrcode and process
>>>>> somehow fill
>>>>> qrcode=0 when it's enabled? (To achieve API compatibility with old
>>>>> clients).
>>>>
>>>> I see three options:
>>>> 1. Don't let the user turn it off from the command line (he can still
>>>> turn it off from the Python API).
>>>>
>>>> 2. Change it to --no-qrcode (API change)
>>>>
>>>> 3. Change the type to Bool (API change)
>>>>
>>>> Like you, I like #2 the best. Attached is an implementation.
>>>
>>> I like --no-qrcode as well.
>>>
>>> Should we also keep qrcode as 'no_option' to maintain API compatibility
>>> (but not CLI)?
>>
>> I don't think it is necessary. It only makes sense to specify --qrcode
>> in an interactive session.
>>
>
> Makes sense.
>
> ACK
>
> Not pushing yet to give time for NACK if anybody doesn't agree with the
> API change.

Hold on, what is happening here?

Aren't all clients since 4.0 sending the qrcode option to the server?
We absolutely can not break backwards compatibility with released versions.
We also should not break the CLI. Just make it a no-op option, and say 
it's deprecated in the doc.

-- 
Petr³