[Freeipa-devel] [PATCH] 0056 Framework for admin/install tools, with ipa-ldap-updater

[Petr Viktorin]

On 06/20/2012 06:15 PM, Rob Crittenden wrote:
> Petr Viktorin wrote:
>> On 06/04/2012 04:56 PM, Petr Viktorin wrote:
>>> Currently, FreeIPA's install/admin scripts are long pieces of code
>>> that aren't very reusable, importable, or testable.
>>> They have been extended over time with features such as logging and
>>> error handling, but since each tool was extended individually, there
>>> is much inconsistency and code duplication.
>>> This patch starts a framework which the admin tools can use, and
>>> converts ipa-ldap-updater to use the framework.
>>>
>>> In an earlier patch I found that improving a particular functionality in
>>> all the commands is not workable, so I want to tackle this one tool at a
>>> time.
>>> I'm starting with ipa-ldap-updater, because it's pretty small, doesn't
>>> use DNs (I don't want conflicts with John's work), and has the
>>> interesting --upgrade option.
>>>
>>>
>>> The framework does these tasks:
>>> - Parse options
>>> - Select tool to run (see below)
>>> - Validate options
>>> - Set up logging
>>> - Run the tool code
>>> - Handle any errors
>>> - Log success/failure
>>>
>>> The base class has some defaults for these that the tools can
>>> extend/override.
>>>
>>>
>>> To handle the case where one script does two different things
>>> (ipa-ldap-updater with/without --upgrade, or ipa-server-install
>>> with/without --uninstall), I want to split the tool in two classes
>>> rather than have repeated ifs in the code.
>>> This meant that option parsing (and initializing the parser) has to be
>>> done before creating an instance of the tool. I use a factory
>>> classmethod.
>>>
>>>
>>> I put the admintool base class in ipapython/ as it should be useful for
>>> ipa-client-install as well.
>>>
>>>
>>>
>>> First part of the work for:
>>> https://fedorahosted.org/freeipa/ticket/2652
>>>
>>>
>>
>> Attaching rebased patch.
>
> I gather you want people to be calling run_cli() in their admin tools.
> Should main() be made private then? I could see someone getting confused
> and using main instead, which would work, but then the return value
> might not do the right thing.
>
> Or maybe just drop run_cli and have main exit with sys.exit()?

I don't see why running a command as a Python function should be 
discouraged. In fact it could even help -- for example logging could 
only be set up once, so if we call, say, ipa-ldap-updater from 
ipa-server-install, all related logs would go to a single file.
A C-style main (taking a list of arguments and returning the exit 
status) is a good thing for modularity and testability.
The `run_cli` method is just a convenient shortcut for the usual usage, 
so the calling modules can be as small as possible.

If people get confused and call main instead of run_cli, they need to 
manually pass in sys.argv. I think this is enough of a warning that 
their assumptions aren't right.
To make it even clearer I've removed the possibility to pass None as 
argv to main() and have it auto-filled.

Some relevant reading:
http://www.artima.com/weblogs/viewpost.jsp?thread=4829 (old but still valid)
http://en.wikipedia.org/wiki/Main_function#Python

> It isn't correctly handling the case of an update not found:
>
> ipa : INFO Parsing file ad
> [Errno 2] No such file or directory: 'ad'
> ipa : INFO File
> "/usr/lib/python2.7/site-packages/ipapython/admintool.py", line 151, in
> execute
> self.run()
> File
> "/usr/lib/python2.7/site-packages/ipaserver/install/ipa_ldap_updater.py", line
> 180, in run
> modified = ld.update(self.files)
> File "/usr/lib/python2.7/site-packages/ipaserver/install/ldapupdate.py",
> line 828, in update
> sys.exit(1)
>
> ipa : INFO The ipa-ldap-updater command failed, exception: SystemExit: 1

I've added validation for missing files, and improved the error message 
ldapupdate raises (for cases the validation doesn't catch, like passing 
directories or unreadable files).
Ideally ldapupdate would not try to handle the error itself, but that 
code is used in more places that I don't want to break, so I'm leaving 
the extraneous print in.

> Running in test mode with the attached update doesn't seem to work
> either. There is nothing special about this file, just something I had
> lying around:
>
> ipa : INFO File
> "/usr/lib/python2.7/site-packages/ipapython/admintool.py", line 151, in
> execute
> self.run()
> File
> "/usr/lib/python2.7/site-packages/ipaserver/install/ipa_ldap_updater.py", line
> 184, in run
> 'Update complete, changes to be made, test mode', 2)
>
> ipa : INFO The ipa-ldap-updater command failed, exception: ScriptError:
> Update complete, changes to be made, test mode
> ipa : ERROR Update complete, changes to be made, test mode
>
> ipa : ERROR None

Fixed.

> The unit tests still pass which is good.
>
> With ipa-ldap-updater the return value is a bit strange. All the updates
> themselves can fail for one reason or another and the command can still
> consider this a success (it may fail because a feature is not enabled,
> for example). Still, the success message displayed at the end is a bit
> jarring when the updates themselves aren't applied. Here is a snippet
> when running ad.update live:
>
> ipa : INFO New entry: uid=adtrust,cn=notfound,cn=etc,dc=greyoak,dc=com
> ipa : DEBUG ---------------------------------------------
> ipa : DEBUG dn: uid=adtrust,cn=notfound,cn=etc,dc=greyoak,dc=com
> ipa : DEBUG add: 'account' to objectClass, current value []
> ipa : DEBUG add: updated value [u'account']
> ipa : DEBUG ---------------------------------------------
> ipa : DEBUG dn: uid=adtrust,cn=notfound,cn=etc,dc=greyoak,dc=com
> ipa : DEBUG objectClass:
> ipa : DEBUG account
> ipa : DEBUG add: 'adtrust' to uid, current value []
> ipa : DEBUG add: updated value [u'adtrust']
> ipa : DEBUG ---------------------------------------------
> ipa : DEBUG dn: uid=adtrust,cn=notfound,cn=etc,dc=greyoak,dc=com
> ipa : DEBUG objectClass:
> ipa : DEBUG account
> ipa : DEBUG uid:
> ipa : DEBUG adtrust
> ipa : DEBUG ---------------------------------------------
> ipa : DEBUG Final value
> ipa : DEBUG dn: uid=adtrust,cn=notfound,cn=etc,dc=greyoak,dc=com
> ipa : DEBUG objectClass:
> ipa : DEBUG account
> ipa : DEBUG uid:
> ipa : DEBUG adtrust
> ipa : INFO Parent DN of uid=adtrust,cn=notfound,cn=etc,dc=greyoak,dc=com
> may not exist, cannot create the entry
> ipa : INFO The ipa-ldap-updater command was successful
> [root at pinto freeipa]# echo $?
> 0
>
> This may be contrasting just because it is a contrived case. The command
> rval is separate from whether the updates all applied, so maybe this is ok.

The current ipa-ldap-updater also works this way, so this should go in a 
separate ticket.
I worry that changing the return value could make installations fail, 
for example.

> rob


Thanks for the review!

-- 
Petr³