[Freeipa-devel] [PATCH 0052] Only specify the ipatokenuniqueid default in the add operation

[Petr Viktorin]

On 05/22/2014 05:13 PM, Petr Vobornik wrote:
> On 22.5.2014 17:00, Nathaniel McCallum wrote:
>> On Thu, 2014-05-22 at 10:53 -0400, Nathaniel McCallum wrote:
>>> On Thu, 2014-05-22 at 16:45 +0200, Petr Viktorin wrote:
>>>> On 05/22/2014 04:12 PM, Nathaniel McCallum wrote:
>>>>> On Tue, 2014-05-13 at 12:55 -0400, Nathaniel McCallum wrote:
>>>>>> On Tue, 2014-05-13 at 16:47 +0200, Jan Cholasta wrote:
>>>>>>> On 12.5.2014 20:50, Nathaniel McCallum wrote:
>>>>>>>> On Mon, 2014-05-12 at 18:40 +0200, Misnyovszki Adam wrote:
>>>>>>>>> On Tue, 06 May 2014 11:46:14 -0400
>>>>>>>>> Nathaniel McCallum <npmccallum at redhat.com> wrote:
>>>>>>>>>
>>>>>>>>>> On Tue, 2014-05-06 at 11:38 -0400, Nathaniel McCallum wrote:
>>>>>>>>>>> On Tue, 2014-05-06 at 17:34 +0200, Petr Vobornik wrote:
>>>>>>>>>>>> On 6.5.2014 17:13, Nathaniel McCallum wrote:
>>>>>>>>>>>>> On Tue, 2014-05-06 at 17:04 +0200, Petr Vobornik wrote:
>>>>>>>>>>>>>> On 6.5.2014 16:51, Nathaniel McCallum wrote:
>>>>>>>>>>>>>>> Specifying the default in the LDAP Object causes the
>>>>>>>>>>>>>>> parameter to be specified for non-add operations. This is
>>>>>>>>>>>>>>> especially problematic when performing the modify operation
>>>>>>>>>>>>>>> as it causes the primary key to change for every
>>>>>>>>>>>>>>> modification.
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>> https://fedorahosted.org/freeipa/ticket/4227
>>>>>>>>>>>>>>>
>>>>>>>>>>>>>>
>>>>>>>>>>>>>> shouldn't removal of `autofill=True,` be enough?
>>>>>>>>>>>>>
>>>>>>>>>>>>> Removing autofill=True results in the default not being used
>>>>>>>>>>>>> for the otptoken-add operation. That may be a different bug
>>>>>>>>>>>>> (I'm not sure what the expectation of autofill is).
>>>>>>>>>>>>>
>>>>>>>>>>>>> Nathaniel
>>>>>>>>>>>>>
>>>>>>>>>>>>
>>>>>>>>>>>> Seems to work form me with:
>>>>>>>>>>>>
>>>>>>>>>>>> diff --git a/ipalib/plugins/otptoken.py
>>>>>>>>>>>> b/ipalib/plugins/otptoken.py index f68ea7d..623f1f1 100644
>>>>>>>>>>>> --- a/ipalib/plugins/otptoken.py
>>>>>>>>>>>> +++ b/ipalib/plugins/otptoken.py
>>>>>>>>>>>> @@ -121,9 +121,7 @@ class otptoken(LDAPObject):
>>>>>>>>>>>>                  cli_name='id',
>>>>>>>>>>>>                  label=_('Unique ID'),
>>>>>>>>>>>>                  default_from=lambda: unicode(uuid.uuid4()),
>>>>>>>>>>>> -            autofill=True,
>>>>>>>>>>>>                  primary_key=True,
>>>>>>>>>>>> -            flags=('optional_create'),
>>>>>>>>>>>>              ),
>>>>>>>>>>>>              StrEnum('type?',
>>>>>>>>>>>>                  label=_('Type'),
>>>>>>>>>>>
>>>>>>>>>>> Doing this causes the ipa otptoken-add command to prompt for the
>>>>>>>>>>> Unique ID. This may be the desired behavior, but it is not
>>>>>>>>>>> how it
>>>>>>>>>>> worked previously (no prompt).
>>>>>>>>>>
>>>>>>>>>> Here is an alternate patch for this second approach. I have no
>>>>>>>>>> strong
>>>>>>>>>> opinion on the correct behavior here.
>>>>>>>>>>
>>>>>>>>>> Nathaniel
>>>>>>>>>
>>>>>>>>> IMO you should update API.txt with ./makeapi
>>>>>>>>
>>>>>>>> Running ./makeapi results in no changes to API.txt.
>>>>>>>
>>>>>>> This is not right, there *are* changes in the API and build fails
>>>>>>> for me
>>>>>>> becase API.txt is not updated.
>>>>>>
>>>>>> I think maybe I ran it from the wrong branch. Fixed.
>>>>>
>>>>> I still need a review of this. It is pretty trivial.
>>>>>
>>>>> Nathaniel
>>>>
>>>> This still prompts for the unique ID on add:
>>>>
>>>> $ ipa otptoken-add
>>>> Unique ID [25cb3aa9-db19-40f8-acf4-33ef65ca863c]:
>>>>
>>>> I don't think that's the intended behavior.
>>>
>>> Hence the alternate patch (0052a). If we don't want to prompt, we'll
>>> need to use the first patch (0052). I have no strong opinion on the
>>> correct behavior and I am fine with merging either patch.
>>
>> Attached is the non-alternate (0052) with the api updated.
>>
>> Nathaniel
>
> IMO 52a is better if used by hand and it keeps code cleaner.

I don't think that should influence the design of the CLI that much.

> It might
> not be ideal though if used from a script because of nonexistent
> --unattended/-u option which would disable prompt (set
> env.interactive=False ?).

Right.


ACK to the non-prompting version (0052.1).


-- 
Petr³