Hi Paul and Jesse,
> 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.
thanks for looking at my patch. It took me some time to revise your
comments, but I should be able to work tomorrow or at least this week on
the missing parts.
In general I agree with them. But some questions remain, especially as
there is some limited knowledge of puppet internals from my side:
> - The metaprogramming that is used to call clean_cert, clean_cached_facts,
> etc. seems unnecessary.
I usually do that if I have just a list of methods I need to call
sequentially. But I can do the list of methods if you prefer.
> - 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.
You're right, also regarding handling multiple nodes we have to deal
with that a bit differently.
> - 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().
I'm still struggling a bit with the idea and the internals of the
indirector and googling "puppet indirector" didn't reveal the magic
hitchhiker's guide to puppet's indirector. Is there any? (I will scan
the -dev Archive after I sent that mail)
So as far as I understood your proposal this would look like:
# clean cached node +host+
def clean_cached_node(host)
Puppet::Node.terminus = :yaml
Puppet::Node.destroy(Puppet::Node.indirection.request(:destroy, host))
Puppet.info "%s's cached node removed" % host
end
But well the part with the request looks still a bit clumsy and I
suspect that there is an easier way to do it?
> - 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.
Will try to implement your alternative.
> 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.
Yeah, in general I left Brice's patches as they were to give him still
the credits in the history for his initial work. But I think the mess is
too big and I will simply squash them all together. Or how should I do it?
> - Our other apps use "-d" as short for "--debug" and "-v" as short for
> "--verbose". Puppet clean should support this too.
As proposed by Brice, I have a patch that refactors --debug and
--verbose of all apps into Puppet::Application. It will be a separate
(initial) patch for the upcoming fixed patch series for #1886. Do you
think that this needs a separate ticket?
> - It seems unnecessary to pass self.node to clean_cert,
clean_cached_facts,
> etc. These methods should be able to access self.node directly.
If we will do the next paragraph, it will make sense.
> - 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.
I will implement the possibility to clean multiple hosts.
> - 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.
Good point, I didn't think of that possibility as I don't have a remote
CA, but I agree. So I will only invoke revoke if we are a full CA.
> - 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.
Uhh, because it failed otherwise? :P Well this was one of the various
parts I didn't fully understand about the indirector. But while I was
looking now more into it, I think we should actually implement a destroy
method in Puppet::Indirector::ActiveRecord which we delegate to the AR
model.
The issue was that some part during the clean up of the storeconfigs
section tried to call such a method. But I don't remember anymore
exactly which one, as could hardly orientate myself in the indirector. I
will have a look at it when I'm testing again the cleaned up clean app
and will look for a more appropriate solution then.
~pete
--
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.
