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@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!
