Removed update_from_dict function from ldapupdate

[stlaz]

update_from_dict was basically dead code as it's used nowhere in
the project.

https://fedorahosted.org/freeipa/ticket/6311

[abbra]

NACK. Please instead fix update_from_dict() to follow _run_updates() expectations.

update_from_dict() is a handy function for externally provided FreeIPA modules. They will need to implement the same functionality if they would want to do dynamic updates themselves. Thus, the function is better to stay to avoid duplication and instead be fixed to properly call _run_updates().

[MartinBasti]

Shouldn't external plugins use update files or update plugins as IPA does?

We don't have any guaranteed internal API for anything, we don't have any document about external plugins, we don't have prepared any API for 3rd party plugins. We just have your POC 3rd party plugin.

Unless there is no design document with serious investigation and agreed workflow how to work with 3rd party plugins, provided stable well tested API for 3rd party plugins (not just internal API that may and will change), then "is a handy" is not valid argument for me.

I don't remember that we did freeze our internal API, so 3rd party plugins will fail with any change there.

[abbra]

Update plugins are higher level of abstraction. They use ipaserver.install.ldapupdate.LDAPUpdate which provides both .update() and .update_from_dict() methods. Update plugins can produce dictionaries. With the change in this pull request they will have to always write down dynamic update content to files first and then run LDAPUpdate.update() with those files. Or re-implement .update_from_dict().

That's why I gave a NACK -- consider this coming from the work I'm doing right now to create documentation for external plugins. It is silly to remove function only to introduce it back.

[stlaz]

Well, I did fix the test, then. I can imagine the function being pretty handy as a library function although it'd better be used in the future.
What infuriates me is the fact that the test might have never worked (well, at least year and a half, but my guess is never) and nobody really cared.

[rcritten]

For the record this test used to pass. Don't blame the test when the code it is testing was changed.

[HonzaCholasta]

@abbra:

Update plugins are higher level of abstraction. They use ipaserver.install.ldapupdate.LDAPUpdate which provides both .update() and .update_from_dict() methods. Update plugins can produce dictionaries. With the change in this pull request they will have to always write down dynamic update content to files first and then run LDAPUpdate.update() with those files. Or re-implement .update_from_dict().

This is not true, update plugins are supposed to return the dictionaries from their execute method. See any of the update plugins in ipaserver/install/plugins for how it's done.

[rcritten]

The sort of pie-in-the-sky thinking about 3rd party plugins that Jason and I talked about eons ago was users would provide their own update files as part of their installer to be dropped somewhere (perhaps into the IPA-provided directory) and be processed like any other. I'm not sure I'd want a 3rd party plugin poking at ldapupdate internals.

[abbra]

This is not true, update plugins are supposed to return the dictionaries from their execute method. See any of the update plugins in ipaserver/install/plugins for how it's done.

most of plugins there use direct 'ldap.update()' calls and return empty lists of dictionaries. Perhaps, 'update_uniqueness.py' and 'rename_managed.py' could serve as examples where a list of dictionaries is returned to do the update. But point is taken -- I think this proves we can remove the method as the original patch proposed.

[stlaz]

Removed the method and its respective test again.

[abbra]

LGTM. Thanks.

[MartinBasti]

Fixed upstream
master:
https://fedorahosted.org/freeipa/changeset/330a3ca93101bcec82ec5d3add14586871864bdd
ipa-4-4:
https://fedorahosted.org/freeipa/changeset/fd9434cab32e1581706d5c2925c774f9afe125b9
ipa-4-3:
https://fedorahosted.org/freeipa/changeset/126c7c6932bba62342eefdc910877df1075e4a70