On 13.3.2015 10:42, Alexander Bokovoy wrote: > On Fri, 13 Mar 2015, Petr Spacek wrote: >> 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. > I like idea about mock_writes=True. However, I think we still need to make > sure plugin writers rely on options.test value to see that we aren't > going to write the data. The reason for it is that we might get into > configurations where plugins would be doing updates based on earlier > performed tasks. If task configuration is not going to be written, its > status will never be correct and plugin would get an error.
That is exactly why I mentioned LDAP transactions. There is no other way how to test complex plugins which actually read own writes (except mocking the whole LDAP interface somewhere). -- Petr^2 Spacek -- Manage your subscription for the Freeipa-devel mailing list: https://www.redhat.com/mailman/listinfo/freeipa-devel Contribute to FreeIPA: http://www.freeipa.org/page/Contribute/Code
