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. -- 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.
