On Mon, Jul 25, 2011 at 13:25, Matt Robinson <[email protected]> wrote: > 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.
I disagree: it would be immediately visible that the comment existed, because it sits right there, while the commit message is only visible if someone does a `git blame` *and* looks at the full commit message for that line. Which is several steps of work away. It might not be attended to, but it is hardly the same degree of visibility. > 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. ...but they are not outdated, at least until someone changes the code they immediately reference, right? People might not update the comments, but aside from that they remain accurate: they describe why the current code was written the way it was. If the rest of the system changes, and violates the assumptions the code, and the related comment, refer to then the comment should still be useful: it shows why this was done, and cues someone in that it dates from *before* the assumption changed, not after. Without that, I have to inspect the commit message to find out that your code was because of the run_mode thing, not because of some other ordering dependency, or because you thought it was an optimization, or ... whatever. I can't read your mind, and the code itself tells me nothing about your intentions. > 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. I just meant the top level Puppet required; you would need to delay loading the filebucket either way, because of the ordering problem. :) Daniel Not that I said that. -- ⎋ Puppet Labs Developer – http://puppetlabs.com ♲ Made with 100 percent post-consumer electrons -- 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.
