Re: [Freeipa-devel] [PATCH] 0056 Framework for admin/install tools, with ipa-ldap-updater
John Dennis wrote: On 07/20/2012 12:34 PM, Petr Viktorin wrote: On 07/20/2012 05:59 PM, John Dennis wrote: A fair amount of the code in the framework is doing this now, but the install code was never cleaned up. That was left for another day, I guess that day is here. Updated. I also added the necessary lint exception. I'm curious as to why it works that way, though. Normally, to put methods in a class, you would use a base class/mixin. Why this dynamic magic? Good question. I don't like dynamic magic either, it makes static code reading difficult and diminishes the value of static code analysis/checking. Jason who originally wrote the framework used dynamic magic a fair amount, including the initialization of the logging methods in the plugins. When I wrote the log manager I kept Jason's existing strategy (how many things do change at once?). In hindsight a mixin would have been a better solution, something we should probably fix down the road. Thinking more about this, composition would probably be cleaner than inheritance here (i.e. using self.logger.warn(...) instead of mixing the functionality into the class itself). Well, one day the time to fix it will come. Everything looks good, although there might be one minor thing that needs fixing. Shouldn't the logging setup be done first? That way any code that executes has the ability to emit a message. For example what if validate_options() wants to emit a message? That's a good question. If we set up logging before validating the options, we'll log all invocations, including those with misspelled option names and forgotten sudos. Do we want that? Sure, hard to say. Why don't we leave it the way it is now and down the road if it shows up as an issue we'll tweak it then. ACK. pushed to master ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0056 Framework for admin/install tools, with ipa-ldap-updater
On 07/19/2012 08:01 PM, John Dennis wrote: Overall I really like the approach, good work. I have not applied and run the patch, so my comments are based only on code reading. I have just a small things which might need changing. One of the ideas in the log manager is to easily support per class logging (borrowed from Java). This allows you to enable/disable or otherwise configure logging per logical code unit as opposed to a big global hammer (i.e. root logger). It also allows for the class name (logger name) to show up in the log output which can be really handy so you know which class is emitting a message (we don't currently turn this on but it's a trival one-line change to the log format spec.). Each significant class should initialize it's own logging, which is very easy to do with this line at the top of a class's __init__() log_mgr.get_logger(self, True) so instead of: self.logger.info() you would say: self.info() A fair amount of the code in the framework is doing this now, but the install code was never cleaned up. That was left for another day, I guess that day is here. Updated. I also added the necessary lint exception. I'm curious as to why it works that way, though. Normally, to put methods in a class, you would use a base class/mixin. Why this dynamic magic? Also each log method (i.e. info(), warn(), error(), etc) should pass it's format substitutions as parameters rather than preformatting the message before submitting it to the logger. This is a performance optimization because the logger delays building the message until it knows the message will actually be emitted and not discarded (common case with debug messages). So instead of: self.debug(the contents of this big dict is: %s % my_dict) You would write: self.debug(the contents of this big dict is: %s, my_dict) A small different in coding, but a lot less work at run time. Fixed, thanks for noticing. I'm not sure the following message handling is optimal +message = '\n'.join( +The hostname resolves to the localhost address (127.0.0.1/::1), +Please change your /etc/hosts file so that the hostname, +resolves to the ip address of your network interface., +, +Please fix your /etc/hosts file and restart the setup program, I'm not a big fan of individual lines of text in the source that are glued together or are emitted individually via a series of print statements. I'd rather see multi-line text as multi-line text using Python's multi-line string operator (e.g. ''' or ). That way you can see the text as it's meant to appear, but there is a bigger reason. You're right, fixed. To keep sane indentation I used textwrap.dedent. Shouldn't this message be internationalized? (e.g. wrapped in _()). When a message is internationalized it's even more important for multi-line text to appear in it entirety to give translators the greatest context and latitude for their translation rather than arbitrarily splitting it up into fragments according to English norms (fragments that might not even translate in another language). Also, a quick glance suggests a number of the messages need i18n treatment. None of the tools are internationalized. Perhaps they need to be, but it should be a separate ticket. Overall though, I really love the approach, great work! Thanks for the review :) -- Petr³ From cfa0d80b78bf9ea8651a0364703154c47eae9ff6 Mon Sep 17 00:00:00 2001 From: Petr Viktorin pvikt...@redhat.com Date: Fri, 20 Apr 2012 04:39:59 -0400 Subject: [PATCH] Framework for admin/install tools, with ipa-ldap-updater 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. Common tasks the tools do -- option parsing, validation, logging setup, error handling -- are represented as methods. Individual tools can extend, override or reuse the defaults as they see fit. The ipa-ldap-updater has two modes (normal and --upgrade) that don't share much functionality. They are represented by separate classes. Option parsing, and selecting which class to run, happens before they're instantiated. All code is moved to importable modules to aid future testing. The only thing that remains in the ipa-ldap-updater script is a two-line call to the library. First part of the work for: https://fedorahosted.org/freeipa/ticket/2652 --- install/tools/ipa-ldap-updater| 206 - ipapython/admintool.py| 229 + ipaserver/install/installutils.py | 92 ++--- ipaserver/install/ipa_ldap_updater.py | 189
Re: [Freeipa-devel] [PATCH] 0056 Framework for admin/install tools, with ipa-ldap-updater
A fair amount of the code in the framework is doing this now, but the install code was never cleaned up. That was left for another day, I guess that day is here. Updated. I also added the necessary lint exception. I'm curious as to why it works that way, though. Normally, to put methods in a class, you would use a base class/mixin. Why this dynamic magic? Good question. I don't like dynamic magic either, it makes static code reading difficult and diminishes the value of static code analysis/checking. Jason who originally wrote the framework used dynamic magic a fair amount, including the initialization of the logging methods in the plugins. When I wrote the log manager I kept Jason's existing strategy (how many things do change at once?). In hindsight a mixin would have been a better solution, something we should probably fix down the road. Everything looks good, although there might be one minor thing that needs fixing. Shouldn't the logging setup be done first? That way any code that executes has the ability to emit a message. For example what if validate_options() wants to emit a message? -- John Dennis jden...@redhat.com Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0056 Framework for admin/install tools, with ipa-ldap-updater
On 07/20/2012 05:59 PM, John Dennis wrote: A fair amount of the code in the framework is doing this now, but the install code was never cleaned up. That was left for another day, I guess that day is here. Updated. I also added the necessary lint exception. I'm curious as to why it works that way, though. Normally, to put methods in a class, you would use a base class/mixin. Why this dynamic magic? Good question. I don't like dynamic magic either, it makes static code reading difficult and diminishes the value of static code analysis/checking. Jason who originally wrote the framework used dynamic magic a fair amount, including the initialization of the logging methods in the plugins. When I wrote the log manager I kept Jason's existing strategy (how many things do change at once?). In hindsight a mixin would have been a better solution, something we should probably fix down the road. Thinking more about this, composition would probably be cleaner than inheritance here (i.e. using self.logger.warn(...) instead of mixing the functionality into the class itself). Well, one day the time to fix it will come. Everything looks good, although there might be one minor thing that needs fixing. Shouldn't the logging setup be done first? That way any code that executes has the ability to emit a message. For example what if validate_options() wants to emit a message? That's a good question. If we set up logging before validating the options, we'll log all invocations, including those with misspelled option names and forgotten sudos. Do we want that? -- Petr³ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0056 Framework for admin/install tools, with ipa-ldap-updater
On 07/20/2012 12:34 PM, Petr Viktorin wrote: On 07/20/2012 05:59 PM, John Dennis wrote: A fair amount of the code in the framework is doing this now, but the install code was never cleaned up. That was left for another day, I guess that day is here. Updated. I also added the necessary lint exception. I'm curious as to why it works that way, though. Normally, to put methods in a class, you would use a base class/mixin. Why this dynamic magic? Good question. I don't like dynamic magic either, it makes static code reading difficult and diminishes the value of static code analysis/checking. Jason who originally wrote the framework used dynamic magic a fair amount, including the initialization of the logging methods in the plugins. When I wrote the log manager I kept Jason's existing strategy (how many things do change at once?). In hindsight a mixin would have been a better solution, something we should probably fix down the road. Thinking more about this, composition would probably be cleaner than inheritance here (i.e. using self.logger.warn(...) instead of mixing the functionality into the class itself). Well, one day the time to fix it will come. Everything looks good, although there might be one minor thing that needs fixing. Shouldn't the logging setup be done first? That way any code that executes has the ability to emit a message. For example what if validate_options() wants to emit a message? That's a good question. If we set up logging before validating the options, we'll log all invocations, including those with misspelled option names and forgotten sudos. Do we want that? Sure, hard to say. Why don't we leave it the way it is now and down the road if it shows up as an issue we'll tweak it then. ACK. -- John Dennis jden...@redhat.com Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0056 Framework for admin/install tools, with ipa-ldap-updater
Overall I really like the approach, good work. I have not applied and run the patch, so my comments are based only on code reading. I have just a small things which might need changing. One of the ideas in the log manager is to easily support per class logging (borrowed from Java). This allows you to enable/disable or otherwise configure logging per logical code unit as opposed to a big global hammer (i.e. root logger). It also allows for the class name (logger name) to show up in the log output which can be really handy so you know which class is emitting a message (we don't currently turn this on but it's a trival one-line change to the log format spec.). Each significant class should initialize it's own logging, which is very easy to do with this line at the top of a class's __init__() log_mgr.get_logger(self, True) so instead of: self.logger.info() you would say: self.info() A fair amount of the code in the framework is doing this now, but the install code was never cleaned up. That was left for another day, I guess that day is here. Also each log method (i.e. info(), warn(), error(), etc) should pass it's format substitutions as parameters rather than preformatting the message before submitting it to the logger. This is a performance optimization because the logger delays building the message until it knows the message will actually be emitted and not discarded (common case with debug messages). So instead of: self.debug(the contents of this big dict is: %s % my_dict) You would write: self.debug(the contents of this big dict is: %s, my_dict) A small different in coding, but a lot less work at run time. I'm not sure the following message handling is optimal +message = '\n'.join( +The hostname resolves to the localhost address (127.0.0.1/::1), +Please change your /etc/hosts file so that the hostname, +resolves to the ip address of your network interface., +, +Please fix your /etc/hosts file and restart the setup program, I'm not a big fan of individual lines of text in the source that are glued together or are emitted individually via a series of print statements. I'd rather see multi-line text as multi-line text using Python's multi-line string operator (e.g. ''' or ). That way you can see the text as it's meant to appear, but there is a bigger reason. Shouldn't this message be internationalized? (e.g. wrapped in _()). When a message is internationalized it's even more important for multi-line text to appear in it entirety to give translators the greatest context and latitude for their translation rather than arbitrarily splitting it up into fragments according to English norms (fragments that might not even translate in another language). Also, a quick glance suggests a number of the messages need i18n treatment. Overall though, I really love the approach, great work! -- John Dennis jden...@redhat.com Looking to carve out IT costs? www.redhat.com/carveoutcosts/ ___ Freeipa-devel mailing list Freeipa-devel@redhat.com https://www.redhat.com/mailman/listinfo/freeipa-devel
Re: [Freeipa-devel] [PATCH] 0056 Framework for admin/install tools, with ipa-ldap-updater
On 07/17/2012 10:41 PM, Rob Crittenden wrote: Petr Viktorin wrote: On 06/29/2012 11:28 PM, Rob Crittenden wrote: 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
Re: [Freeipa-devel] [PATCH] 0056 Framework for admin/install tools, with ipa-ldap-updater
Petr Viktorin wrote: On 06/29/2012 11:28 PM, Rob Crittenden wrote: 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
Re: [Freeipa-devel] [PATCH] 0056 Framework for admin/install tools, with ipa-ldap-updater
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:
Re: [Freeipa-devel] [PATCH] 0056 Framework for admin/install tools, with ipa-ldap-updater
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
Re: [Freeipa-devel] [PATCH] 0056 Framework for admin/install tools, with ipa-ldap-updater
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
Re: [Freeipa-devel] [PATCH] 0056 Framework for admin/install tools, with ipa-ldap-updater
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 : ERRORUpdate complete, changes to be made, test mode ipa : ERRORNone 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 : DEBUGdn: uid=adtrust,cn=notfound,cn=etc,dc=greyoak,dc=com ipa : DEBUGadd: 'account' to objectClass, current value [] ipa : DEBUGadd: updated value [u'account'] ipa : DEBUG- ipa : DEBUGdn: uid=adtrust,cn=notfound,cn=etc,dc=greyoak,dc=com ipa : DEBUGobjectClass: ipa : DEBUG account ipa : DEBUGadd: 'adtrust' to uid, current value [] ipa : DEBUGadd: updated value [u'adtrust'] ipa : DEBUG- ipa : DEBUGdn: uid=adtrust,cn=notfound,cn=etc,dc=greyoak,dc=com ipa : DEBUGobjectClass: ipa : DEBUG account ipa : DEBUGuid: ipa : DEBUG adtrust ipa : DEBUG- ipa : DEBUGFinal value ipa : DEBUGdn: uid=adtrust,cn=notfound,cn=etc,dc=greyoak,dc=com ipa : DEBUGobjectClass: ipa : DEBUG account ipa : DEBUGuid: ipa : DEBUG adtrust ipa : INFO Parent DN of
Re: [Freeipa-devel] [PATCH] 0056 Framework for admin/install tools, with ipa-ldap-updater
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. -- Petr³ From 801f7c0f12f799a659a827daa569a8e3870a1eb6 Mon Sep 17 00:00:00 2001 From: Petr Viktorin pvikt...@redhat.com Date: Fri, 20 Apr 2012 04:39:59 -0400 Subject: [PATCH] Framework for admin/install tools, with ipa-ldap-updater 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. Common tasks the tools do -- option parsing, validation, logging setup, error handling -- are represented as methods. Individual tools can extend, override or reuse the defaults as they see fit. The ipa-ldap-updater has two modes (normal and --upgrade) that don't share much functionality. They are represented by separate classes. Option parsing, and selecting which class to run, happens before they're instantiated. All code is moved to importable modules to aid future testing. The only thing that remains in the ipa-ldap-updater script is a two-line call to the library. First part of the work for: https://fedorahosted.org/freeipa/ticket/2652 --- install/tools/ipa-ldap-updater| 198 ipapython/admintool.py| 230 + ipaserver/install/installutils.py | 91 ++--- ipaserver/install/ipa_ldap_updater.py | 184 ++ 4 files changed, 479 insertions(+), 224 deletions(-) rewrite install/tools/ipa-ldap-updater (85%) create mode 100644 ipapython/admintool.py create mode 100644 ipaserver/install/ipa_ldap_updater.py diff --git a/install/tools/ipa-ldap-updater b/install/tools/ipa-ldap-updater dissimilarity index 85% index 197b840b07867269340f56e32989820d8af59ae1..0fc5a5bc448d04eb9ce8562ee1feebbf8f0fe356 100755 --- a/install/tools/ipa-ldap-updater +++ b/install/tools/ipa-ldap-updater @@ -1,173 +1,25 @@ -#!/usr/bin/python -# Authors: Rob Crittenden rcrit...@redhat.com -# -# Copyright (C) 2008 Red Hat -# see file 'COPYING' for use and warranty information -# -# This program is free software; you can redistribute it and/or modify -# it under the terms of the GNU General Public License as published by -# the Free Software Foundation, either version 3 of the License, or -# (at your option) any later version. -# -# This program is distributed in the hope that it will be useful, -# but WITHOUT ANY WARRANTY; without even the implied warranty of -# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the -# GNU General Public License for more details. -# -# You should have received a copy of the GNU General Public License -# along with this program. If not, see http://www.gnu.org/licenses/. -# - -# Documentation can be found at http://freeipa.org/page/LdapUpdate - -# TODO -# save undo files? - -import os -import sys -try: -from ipapython.config import IPAOptionParser -from ipapython import ipautil, config -from ipaserver.install import installutils -from ipaserver.install.ldapupdate import LDAPUpdate, BadSyntax, UPDATES_DIR -from ipaserver.install.upgradeinstance import IPAUpgrade -from ipapython import sysrestore -import krbV -from
