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

[Rob Crittenden]

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()?

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

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

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.

rob
-------------- next part --------------
dn: uid=adtrust,cn=sysaccounts,cn=etc,$SUFFIX
add: objectClass: account
add: objectClass: simplesecurityobject
add: uid: adtrust

dn: uid=adtrust,cn=notfound,cn=etc,$SUFFIX
add: objectClass: account
add: uid: adtrust