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

[Petr Viktorin]

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.

-- 
Petr³
-------------- next part --------------
A non-text attachment was scrubbed...
Name: freeipa-pviktori-0056-03-Framework-for-admin-install-tools-with-ipa-ldap-upda.patch
Type: text/x-patch
Size: 30668 bytes
Desc: not available
URL: <http://listman.redhat.com/archives/freeipa-devel/attachments/20120625/a889e910/attachment.bin>