On Mon, Jul 25, 2011 at 1:10 PM, Daniel Pittman <[email protected]> wrote: > On Mon, Jul 25, 2011 at 12:57, Matt Robinson <[email protected]> wrote: >> Requiring puppet before the run_mode has been set by the application >> causes the default run_mode of 'user' to be set instead of what the >> application wants. This leads to the incorrect default settings to be >> used, which lead to inspect not being able to properly retrieve file >> metadata from a fileserver. >> >> Reviewed-by: Max Martin <[email protected]> >> Signed-off-by: Matt Robinson <[email protected]> >> --- >> lib/puppet/application/inspect.rb | 7 +++++-- >> spec/unit/application/inspect_spec.rb | 5 +++++ >> 2 files changed, 10 insertions(+), 2 deletions(-) >> >> diff --git a/lib/puppet/application/inspect.rb >> b/lib/puppet/application/inspect.rb >> index ce32c66..2260fee 100644 >> --- a/lib/puppet/application/inspect.rb >> +++ b/lib/puppet/application/inspect.rb >> @@ -1,6 +1,4 @@ >> -require 'puppet' >> require 'puppet/application' >> -require 'puppet/file_bucket/dipper' >> >> class Puppet::Application::Inspect < Puppet::Application >> >> @@ -97,6 +95,11 @@ Licensed under the GNU General Public License version 2 >> Puppet::Resource::Catalog.terminus_class = :yaml >> end >> >> + def preinit >> + require 'puppet' >> + require 'puppet/file_bucket/dipper' >> + end > > I would be much more comfortable if the information in the commit > message were replicated here. Otherwise the next person to come > along, aside from you and I, and try to work out what the heck is > going on here will have to rediscover this coalition of fragile code > themselves.
I thought about how to communicate the run mode problem, and there doesn't seem to me to be an obvious place to do it. I think a comment in the preinit is as likely to be overlooked as a commit message. The fact that requiring puppet has so many side effects (setting run mode in this case to a default instead of what the app wants) is something that we need to work to reduce, but I don't like just peppering a bunch of comments that are sure to be outdated in the code. The biggest place it's good to know about this run_mode issue is when writing new applications, and just having the application setup to not have this problems seems as good a solution as any in case someone copies the code. > That said, does this code actually need to require puppet itself? It > should be able to just assume it... If you don't require puppet the filebucket dipper require after it fails. The filebucket's requirement dependencies should probably be fixed long term, but there's a lot of requirement dependency problems with requires and I didn't feel like diving down that rathole for this bug fix. -- 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.
