On Tue, Jan 18, 2011 at 10:44 AM, Nigel Kersten <[email protected]>wrote:
> On Tue, Jan 18, 2011 at 10:41 AM, Nick Lewis <[email protected]> wrote: > > I get an error as soon as I try to run this: > > /Users/nicklewis/puppet/puppet/lib/puppet/defaults.rb:104: undefined > local > > variable or method `diff' for Puppet:Module (NameError) > > The Ruby interpreter is interpreting :show-diff as :show - diff. A > possible > > fix would be to use :"show-diff", if this is what's really intended. > > However, I don't have enough context from the ticket or commit message to > > understand the reason for this change. We have no options containing > dashes, > > and many containing underscores, so I question what this change is making > > show_diff more consistent with. The only place we have dashes is in the > > command-line form of specifying boolean options, in which case we use > > --[no-]OPTION. In the actual option names, we are inconsistent between > > underscores (show_diff, config_version) and no separator at all > > (usecacheonfailure, downcasefacts). We do have an item on our technical > debt > > list for standardizing to using hyphens in our option names (at least on > the > > command line), but that will require a more general patch. > > --detailed-exit-codes doesn't count? > > I jumped the gun and assumed we'd already had to solve this due to > other options containing hyphens. > > I should clarify that by "options" I only mean options that can be set in puppet.conf, specified in defaults.rb. For every option in defaults.rb, we add a corresponding command-line option to the option parser, using the --[no-]OPTION syntax. Applications also add some of their own application-specific command-line-only options (such as --debug, --verbose, --detailed-exitcodes, --test), and do appear to use dashes. I don't see any good reason we don't use dashes in defaults.rb, other than the fact that we use symbols for our option names, which lend themselves more easily to using underscores as seen in this patch. We could probably start using strings and dashes fairly easily. Or we could convert underscores to dashes in the process of creating the associated command-line option, which requires fewer changes inside Puppet since the option names themselves haven't changed, but also creates a disconnect between the option name you use in puppet.conf and the option name you use on the command line. Standardizing (and hyphenating) options is on our technical debt list, but there are quite a few other items higher up the list. I'll file a ticket, so we can properly prioritize it. Nick > > > > Nick > > On Sun, Jan 16, 2011 at 4:28 PM, James Turnbull <[email protected] > > > > wrote: > >> > >> Signed-off-by: James Turnbull <[email protected]> > >> --- > >> lib/puppet/application/agent.rb | 4 ++-- > >> lib/puppet/application/apply.rb | 2 +- > >> lib/puppet/defaults.rb | 2 +- > >> lib/puppet/type/file/content.rb | 2 +- > >> spec/unit/application/apply_spec.rb | 4 ++-- > >> spec/unit/type/file/content_spec.rb | 4 ++-- > >> 6 files changed, 9 insertions(+), 9 deletions(-) > >> > >> diff --git a/lib/puppet/application/agent.rb > >> b/lib/puppet/application/agent.rb > >> index 2b75505..291e68e 100644 > >> --- a/lib/puppet/application/agent.rb > >> +++ b/lib/puppet/application/agent.rb > >> @@ -137,7 +137,7 @@ class Puppet::Application::Agent < > Puppet::Application > >> Puppet.settings.handlearg("--ignorecache") > >> Puppet.settings.handlearg("--no-usecacheonfailure") > >> Puppet.settings.handlearg("--no-splay") > >> - Puppet.settings.handlearg("--show_diff") > >> + Puppet.settings.handlearg("--show-diff") > >> Puppet.settings.handlearg("--no-daemonize") > >> options[:verbose] = true > >> Puppet[:onetime] = true > >> @@ -202,7 +202,7 @@ class Puppet::Application::Agent < > Puppet::Application > >> exit(Puppet.settings.print_configs ? 0 : 1) if > >> Puppet.settings.print_configs? > >> > >> # If noop is set, then also enable diffs > >> - Puppet[:show_diff] = true if Puppet[:noop] > >> + Puppet[:show-diff] = true if Puppet[:noop] > >> > >> args[:Server] = Puppet[:server] > >> if options[:fqdn] > >> diff --git a/lib/puppet/application/apply.rb > >> b/lib/puppet/application/apply.rb > >> index 33a70ce..af1d08c 100644 > >> --- a/lib/puppet/application/apply.rb > >> +++ b/lib/puppet/application/apply.rb > >> @@ -137,7 +137,7 @@ class Puppet::Application::Apply < > Puppet::Application > >> exit(Puppet.settings.print_configs ? 0 : 1) if > >> Puppet.settings.print_configs? > >> > >> # If noop is set, then also enable diffs > >> - Puppet[:show_diff] = true if Puppet[:noop] > >> + Puppet[:show-diff] = true if Puppet[:noop] > >> > >> Puppet::Util::Log.newdestination(:console) unless options[:logset] > >> client = nil > >> diff --git a/lib/puppet/defaults.rb b/lib/puppet/defaults.rb > >> index 4521a59..ad82205 100644 > >> --- a/lib/puppet/defaults.rb > >> +++ b/lib/puppet/defaults.rb > >> @@ -101,7 +101,7 @@ module Puppet > >> }, > >> :diff_args => ["-u", "Which arguments to pass to the diff command > when > >> printing differences between files."], > >> :diff => ["diff", "Which diff command to use when printing > differences > >> between files."], > >> - :show_diff => [false, "Whether to print a contextual diff when > files > >> are being replaced. The diff > >> + :show-diff => [false, "Whether to print a contextual diff when > files > >> are being replaced. The diff > >> is printed on stdout, so this option is meaningless unless you are > >> running Puppet interactively. > >> This feature currently requires the `diff/lcs` Ruby library."], > >> :daemonize => { :default => true, > >> diff --git a/lib/puppet/type/file/content.rb > >> b/lib/puppet/type/file/content.rb > >> index b8f30a9..2620bab 100755 > >> --- a/lib/puppet/type/file/content.rb > >> +++ b/lib/puppet/type/file/content.rb > >> @@ -100,7 +100,7 @@ module Puppet > >> > >> result = super > >> > >> - if ! result and Puppet[:show_diff] > >> + if ! result and Puppet[:show-diff] > >> write_temporarily do |path| > >> print diff(@resource[:path], path) > >> end > >> diff --git a/spec/unit/application/apply_spec.rb > >> b/spec/unit/application/apply_spec.rb > >> index 4e17442..c70e857 100755 > >> --- a/spec/unit/application/apply_spec.rb > >> +++ b/spec/unit/application/apply_spec.rb > >> @@ -61,12 +61,12 @@ describe Puppet::Application::Apply do > >> @apply.options.stubs(:[]).with(any_parameters) > >> end > >> > >> - it "should set show_diff on --noop" do > >> + it "should set show-diff on --noop" do > >> Puppet.stubs(:[]=) > >> Puppet.stubs(:[]).with(:config) > >> Puppet.stubs(:[]).with(:noop).returns(true) > >> > >> - Puppet.expects(:[]=).with(:show_diff, true) > >> + Puppet.expects(:[]=).with(:show-diff, true) > >> > >> @apply.setup > >> end > >> diff --git a/spec/unit/type/file/content_spec.rb > >> b/spec/unit/type/file/content_spec.rb > >> index cde643f..b48d57e 100755 > >> --- a/spec/unit/type/file/content_spec.rb > >> +++ b/spec/unit/type/file/content_spec.rb > >> @@ -169,9 +169,9 @@ describe content do > >> @content.must be_insync("{md5}" + Digest::MD5.hexdigest("some > >> content")) > >> end > >> > >> - describe "and Puppet[:show_diff] is set" do > >> + describe "and Puppet[:show-diff] is set" do > >> before do > >> - Puppet[:show_diff] = true > >> + Puppet[:show-diff] = true > >> end > >> > >> it "should display a diff if the current contents are different > >> from the desired content" do > >> -- > >> 1.7.3.4 > >> > >> -- > >> 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]<puppet-dev%[email protected]> > . > >> For more options, visit this group at > >> http://groups.google.com/group/puppet-dev?hl=en. > >> > > > > -- > > 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]<puppet-dev%[email protected]> > . > > For more options, visit this group at > > http://groups.google.com/group/puppet-dev?hl=en. > > > > -- > 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]<puppet-dev%[email protected]> > . > For more options, visit this group at > http://groups.google.com/group/puppet-dev?hl=en. > > -- 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.
