[Freeipa-devel] [PATCH 0208] Respect --test option in upgrade plugins

[Petr Spacek]

On 13.3.2015 10:18, Martin Kosek wrote:
> On 03/12/2015 05:10 PM, Rob Crittenden wrote:
>> Petr Spacek wrote:
>>> On 12.3.2015 16:23, Rob Crittenden wrote:
>>>> David Kupka wrote:
>>>>> On 03/06/2015 06:00 PM, Martin Basti wrote:
>>>>>> Upgrade plugins which modify LDAP data directly should not be executed
>>>>>> in --test mode.
>>>>>>
>>>>>> This patch is a workaround, to ensure update with --test option will not
>>>>>> modify any LDAP data.
>>>>>>
>>>>>> https://fedorahosted.org/freeipa/ticket/3448
>>>>>>
>>>>>> Patch attached.
>>>>>>
>>>>>>
>>>>>>
>>>>>
>>>>> Ideally we want to fix all plugins to dry-run the upgrade not just skip
>>>>> when there is '--test' option but it is a good first step.
>>>>> Works for me, ACK.
>>>>>
>>>>
>>>> I agree that this breaks the spirit of --test and think it should be
>>>> fixed before committing.
>>>
>>> Considering how often is the option is used ... I do not think that this
>>> requires 'proper' fix now. It was broken for *years* so this patch is a huge
>>> improvement and IMHO should be commited in current form. We can re-visit it
>>> later on, open a ticket :-)
>>>
>>
>> No. There is no rush for this, at least not for the promise of a future
>> fix that will never come.
> 
> I checked the code and to me, the proper fix looks like instrumenting
> ldap.update_entry calls in upgrade plugins with
> 
> if options.test:
>    log message
> else
>    do the update
> 
> right? I see just couple places that would need to be updated:
> 
> $ git grep -E "(ldap|conn).update_entry" ipaserver/install/plugins
> ipaserver/install/plugins/dns.py:        ldap.update_entry(container_entry)
> ipaserver/install/plugins/fix_replica_agreements.py:
> repl.conn.update_entry(replica)
> ipaserver/install/plugins/fix_replica_agreements.py:
> repl.conn.update_entry(replica)
> ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
> ipaserver/install/plugins/update_idranges.py: ldap.update_entry(entry)
> ipaserver/install/plugins/update_managed_permissions.py:
> ldap.update_entry(base_entry)
> ipaserver/install/plugins/update_managed_permissions.py: ldap.update_entry(entry)
> ipaserver/install/plugins/update_pacs.py:            ldap.update_entry(entry)
> ipaserver/install/plugins/update_referint.py:            ldap.update_entry(entry)
> ipaserver/install/plugins/update_services.py: ldap.update_entry(entry)
> ipaserver/install/plugins/update_uniqueness.py:
> ldap.update_entry(uid_uniqueness_plugin)
> 
> 
> So from my POV, very quick fix. In that case, I would also prefer a fix now
> than a ticket that would never be done.

I really dislike this approach because I consider it flawed by design. Plugin
author has to think about it all the time and do not forget to add if
otherwise ... too bad.

I can see two 'safer' ways to do that:
- LDAP transactions :-)
- 'mock_writes=True' option in LDAP backend which would print modlists instead
of applying them (and return success to the caller).

Both cases eliminate the need to scatter 'ifs' all over update plugins and do
not add risk of forgetting about one of them when adding/changing plugin code.

-- 
Petr^2 Spacek