On Tue, Dec 7, 2010 at 1:21 PM, Paul Berry <[email protected]> wrote:
> On Wed, Dec 1, 2010 at 8:46 AM, Peter Meier <[email protected]>wrote: > >> Hi all, >> >> recently I had to cleanup various old hosts and also wanted to >> cleanup their exported resources. >> I remembered that brice hacked once on something, but it looks >> like it never got into the core. >> >> So I took Brice's initial work, ported it over to the new 2.6.x >> style, fixed some bugs, added some other features and adapted >> and improved tests etc. >> >> So here you go: Another proposal to include a cleanup mechanism >> for nodes into puppet. So far it worked very nice for me. >> >> ~pete >> >> > Hi Peter-- > > Thanks for working on this long-standing feature request. Jesse Wolfe and > I reviewed the code, and there are a few issues that would need to be > addressed before we can merge the patches into the codebase. > > > Regarding patch 1 (Fix #1886 - Add node cleanup capability): > > - The documentation in bin/puppetclean won't actually work. We've changed > how documentation works for puppet apps since Brice first wrote the patch. > The documentation now belongs in the lib/puppet/util/command_line > directory. I'd recommend calling it lib/puppet/util/command_line/clean. > Note that you will also have to update lib/puppet/util/command_line.rb to > point to it. For an example of how this should be done for a non-legacy > command have a look at what we do for "filebucket". > > - The copyright notice in bin/puppetclean is out of date (2007) and still > refers to Reductive Labs. > > - We shouldn't be adding a bin/puppetclean file at all. Since this is a > new feature, there's no nead to make "puppetclean" an alias for "puppet > clean". > > - Why do we specify "should_not_parse_config" for this app? It seems like > the app should be respecting config settings. > > - The metaprogramming that is used to call clean_cert, clean_cached_facts, > etc. seems unnecessary. > > - If an error occurs during a call to clean_cert, clean_caced_facts, etc., > the app should abort. As written, it will output a backtrace and then go on > to call the next method. > > - clean_cached_node() is calling the indirector in a clumsy way. It would > be better to set the Node terminus to :yaml and then simply do > Puppet::Node.destroy(). > > > Regarding patch 3 (Add a destroy method for the various reports processor > types): > > - Adding a destroy() method to Puppet::Transaction::Report is confusing, > since this function doesn't destroy a single report instance, it destroys > all reports associated with a given node. > > - It also seems unreasonable that we should have to add a destroy() method > to all report types, even though it only does something in the "store" > report type. > > - How about this alternative: change the destroy() method in "store" to a > class method, remove the other report types' destroy() methods, and extract > a processors() method from Puppet::Transaction::Report::Processor#process, > which returns a list of all report modules. Then, modify > Puppet::Transaction::Report::Processor#destroy so that it loops over all the > modules returned by processors() and calls the destroy() class method on > each module that implements it. > > > Regarding patch 4 (Unlink only existing files): > > - This should be combined with patch 2, since it fixes a bug in patch 2. > > > Regarding patch 5 (Finish implementation of #1886) > > - This should be combined with patch 1, since it fixes bugs in patch 1. > > Whoops, sorry. I dropped a page out of my notes. A few more issues regarding patch 5: - Our other apps use "-d" as short for "--debug" and "-v" as short for "--verbose". Puppet clean should support this too. - It seems unnecessary to pass self.node to clean_cert, clean_cached_facts, etc. These methods should be able to access self.node directly. - As written, puppet clean will accept more than 1 command line argument, but it will ignore everything but the first. It should either clean all the specified nodes or give the user an error message. - Clean_cert looks like it will only work properly if the Puppet master is also the CA. If there is a remote CA, the revoke action will fail. We should make puppet clean work for both local CA and remote CA configurations. - Can you explain why it's necessary to add a no-op destroy method to active_record.rb? On the surface this seems like a mistake. Thanks, Peter. Paul -- You received this message because you are subscribed to the Google Groups "Puppet Developers" group. To post to this group, send email to [email protected]. To unsubscribe from this group, send email to [email protected]. For more options, visit this group at http://groups.google.com/group/puppet-dev?hl=en.
