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

[Martin Basti]

On 13/03/15 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.
>
> Martin
And ldap.add_entry, del_entry, ipa add/mod/del commands

-- 
Martin Basti