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

Fri Jun 29 21:28:43 UTC 2012

Petr Viktorin wrote:
> On 06/25/2012 03:00 PM, Petr Viktorin wrote:
>> 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!
>>
>
> Once again, this time with the patch.

Almost there. When running in test mode and an update that would be 
applied should return 2.

rob