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@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
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
_______________________________________________
Freeipa-devel mailing list
Freeipa-devel@redhat.com
https://www.redhat.com/mailman/listinfo/freeipa-devel

Reply via email to