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

[Martin Kosek]

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.

Martin