[PATCH/puppet 1/1] Feature #2400 : add display changes and add them in YAML to the report

1 view
Skip to first unread message

Stéphan Gorget

unread,
Jul 10, 2009, 3:10:00 AM7/10/09
to puppe...@googlegroups.com
Signed-off-by: Stéphan Gorget <gor...@ocre.cea.fr>
---
lib/puppet/defaults.rb | 4 ++-
lib/puppet/resource/catalog.rb | 2 +-
lib/puppet/transaction.rb | 7 ++++++
lib/puppet/transaction/report.rb | 39 +++++++++++++++++++++++++++++++++++++-
4 files changed, 49 insertions(+), 3 deletions(-)

diff --git a/lib/puppet/defaults.rb b/lib/puppet/defaults.rb
index b2de823..88162ae 100644
--- a/lib/puppet/defaults.rb
+++ b/lib/puppet/defaults.rb
@@ -628,7 +628,9 @@ module Puppet
being evaluated. This allows you to interactively see exactly
what is being done."],
:summarize => [false,
- "Whether to print a transaction summary."
+ "Whether to print a transaction summary."],
+ :changes => [false,
+ "Whether to print the transaction changes."
]
)

diff --git a/lib/puppet/resource/catalog.rb b/lib/puppet/resource/catalog.rb
index 42e92f4..4cd3ba1 100644
--- a/lib/puppet/resource/catalog.rb
+++ b/lib/puppet/resource/catalog.rb
@@ -151,7 +151,7 @@ class Puppet::Resource::Catalog < Puppet::SimpleGraph

yield transaction if block_given?

- transaction.send_report if host_config and (Puppet[:report] or Puppet[:summarize])
+ transaction.send_report if host_config and (Puppet[:report] or Puppet[:summarize] or Puppet[:changes])

return transaction
ensure
diff --git a/lib/puppet/transaction.rb b/lib/puppet/transaction.rb
index f09ca80..06c3c73 100644
--- a/lib/puppet/transaction.rb
+++ b/lib/puppet/transaction.rb
@@ -512,6 +512,13 @@ class Transaction
report.graph()
end

+ report.changes = report.sumup(@events)
+ if Puppet[:changes]
+ unless report.changes.empty?
+ puts report.displaychanges
+ end
+ end
+
if Puppet[:summarize]
puts report.summary
end
diff --git a/lib/puppet/transaction/report.rb b/lib/puppet/transaction/report.rb
index e5b8650..77b8aec 100644
--- a/lib/puppet/transaction/report.rb
+++ b/lib/puppet/transaction/report.rb
@@ -10,7 +10,7 @@ class Puppet::Transaction::Report

indirects :report, :terminus_class => :processor

- attr_accessor :logs, :metrics, :time, :host
+ attr_accessor :logs, :metrics, :time, :host, :changes

# This is necessary since Marshall doesn't know how to
# dump hash with default proc (see below @records)
@@ -58,6 +58,29 @@ class Puppet::Transaction::Report
@records[metric] << object
end

+ # Provide a summary of the changes
+ def displaychanges
+ ret = ""
+
+ # display the different changes for each type
+ changes.keys.sort.each { |t|
+ ret += "%s:\n" % t
+ changes[t].keys.sort.each { |e|
+ # Count the number of occurences for each value in the changes[t][e]
+ count = changes[t][e].inject(Hash.new(0)) {|h,i| h[i] += 1; h }
+ ret += count.to_a.collect { |c, count|
+ if count > 1
+ " %15s %s (%s)" % [c + ":", e, count]
+ else
+ " %15s %s" % [c + ":", e]
+ end
+ }.join("\n")
+ ret += "\n"
+ }
+ }
+ return ret
+ end
+
# Provide a summary of this report.
def summary
ret = ""
@@ -83,5 +106,19 @@ class Puppet::Transaction::Report
end
return ret
end
+
+ # Copy events in a structure that can be return with the report
+ def sumup(events)
+ report= {}
+ events.each { |e|
+ type = e.source.class.name.to_s.capitalize
+ title = e.source.title.to_s
+ change = e.name.to_s
+ report[type] = {} if !report[type].is_a?(Hash)
+ report[type][title] = [] if !report[type][title].is_a?(Array)
+ report[type][title].push(change)
+ }
+ report
+ end
end

--
1.6.2.2

James Turnbull

unread,
Jul 10, 2009, 3:16:26 AM7/10/09
to puppe...@googlegroups.com
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

Stéphan Gorget wrote:
> Signed-off-by: Stéphan Gorget <gor...@ocre.cea.fr>
> ---
> lib/puppet/defaults.rb | 4 ++-
> lib/puppet/resource/catalog.rb | 2 +-
> lib/puppet/transaction.rb | 7 ++++++
> lib/puppet/transaction/report.rb | 39 +++++++++++++++++++++++++++++++++++++-
> 4 files changed, 49 insertions(+), 3 deletions(-)


Tests?

Regards

James Turnbull

- --
Author of:
* Pro Linux Systems Administration
(http://tinyurl.com/linuxadmin)
* Pulling Strings with Puppet
(http://tinyurl.com/pupbook)
* Pro Nagios 2.0
(http://tinyurl.com/pronagios)
* Hardening Linux
(http://tinyurl.com/hardeninglinux)
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.7 (Darwin)
Comment: Using GnuPG with Mozilla - http://enigmail.mozdev.org

iD8DBQFKVurK9hTGvAxC30ARAswdAKCdj0VaDmo7c+Oj4OkczQOXT+QCrACfbZG6
GkPfO+FGziIde8hHB/QSg+A=
=cgIF
-----END PGP SIGNATURE-----

Stephan Gorget

unread,
Jul 10, 2009, 3:29:32 AM7/10/09
to puppe...@googlegroups.com
James Turnbull a écrit :
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
>
> Stéphan Gorget wrote:
>> Signed-off-by: Stéphan Gorget <gor...@ocre.cea.fr>
>> ---
>> lib/puppet/defaults.rb | 4 ++-
>> lib/puppet/resource/catalog.rb | 2 +-
>> lib/puppet/transaction.rb | 7 ++++++
>> lib/puppet/transaction/report.rb | 39 +++++++++++++++++++++++++++++++++++++-
>> 4 files changed, 49 insertions(+), 3 deletions(-)
>
>
> Tests?
It seems that Luke is working on something similar based on what he
wrote (http://projects.reductivelabs.com/issues/2401#note-1), and I just
wanted to show what I have done so far. But if what I've done is going
to be included I will add tests.

Luke Kanies

unread,
Jul 10, 2009, 12:22:11 PM7/10/09
to puppe...@googlegroups.com
I like the basic idea here, and as you discerned, I'm working on
something similar.

There are a couple of straightforward issues (e.g., I can't seem to
find the code that actually adds the changes to the transaction, and
the output would be better in an easily-machine readable form like
yaml or json), but the biggest problem is that we want this
information in the reports, but it has to be done in a way that keeps
references to all of the resources from leaking into the reports.

Change objects have references to parameters, which have references to
resources and catalogs and all that, so adding them directly to
reports and then serializing those reports will result in your reports
being, um, gigantic.

My plan instead has always been to have the changes create Events
(which they already do) and have those events contain all of the
important information from changes but as strings instead of
references (which they don't).

Then you just have the transactions collect all of the events, and
serializing those events to send to the server or print on the console
is very easy because the plain data should print well as yaml, json,
etc.

What do you think?
--
I think that's how Chicago got started. A bunch of people in New York
said, 'Gee, I'm enjoying the crime and the poverty, but it just isn't
cold enough. Let's go west.' --Richard Jeni
---------------------------------------------------------------------
Luke Kanies | http://reductivelabs.com | http://madstop.com

Aurelien....@cea.fr

unread,
Jul 13, 2009, 4:14:59 PM7/13/09
to puppe...@googlegroups.com
Hello

What you're proposing looks very closed to the proposed patch (mainly for the same reasons).
My only concerns are:
- What informations do you plan to add to the transaction reports?
- What the output of 'puppetd --changes' will look like?

As far as those two points are ok with us, we are fine with any patch.

Do you have some bit of code for those 2 features?

Thanks

Aurelien

-------- Message d'origine--------
De: puppe...@googlegroups.com de la part de Luke Kanies
Date: ven. 10/07/2009 18:22
À: puppe...@googlegroups.com
Objet : [Puppet-dev] Re: [PATCH/puppet 1/1] Feature #2400 : add display changes and add them in YAML to the report
winmail.dat

Luke Kanies

unread,
Jul 13, 2009, 11:46:45 PM7/13/09
to puppe...@googlegroups.com
On Jul 13, 2009, at 1:14 PM, <Aurelien....@CEA.FR> <Aurelien....@CEA.FR
> wrote:

> Hello
>
> What you're proposing looks very closed to the proposed patch
> (mainly for the same reasons).
> My only concerns are:
> - What informations do you plan to add to the transaction reports?

Instances of an expanded Event class.

Ethan Rowe should be emailing a proposed design to the -dev list this
week.

>
> - What the output of 'puppetd --changes' will look like?

Those Event instances, in some kind of serialized form. Probably
defaulting to yaml but tunable via the default_serialization_format or
whatever that parameter is.

>
> As far as those two points are ok with us, we are fine with any patch.
>
> Do you have some bit of code for those 2 features?

Not yet, because we're going to spend some time refactoring the
related Event code, since it's messy and hasn't been touched in ages.
> <winmail.dat>


--
The major difference between a thing that might go wrong and a thing
that cannot possibly go wrong is that when a thing that cannot possibly
goes wrong goes wrong it usually turns out to be impossible to get at
or repair. -- Douglas Adams, Mostly Harmless

Aurelien....@cea.fr

unread,
Jul 14, 2009, 3:13:46 PM7/14/09
to puppe...@googlegroups.com


>> - What the output of 'puppetd --changes' will look like?
>
>Those Event instances, in some kind of serialized form. Probably
>defaulting to yaml but tunable via the default_serialization_format or
>whatever that parameter is.

Puppetd was designed to be used as a daemon and does it quite well.
But it lacks a good interface for day to day use, interactively, by admins, in CLI mode, not in daemon.
Having a easily human readable way to known what Puppet has done is important.

So Puppet must have a day to display and sum up the event log without dumping a serialization format.
We need something like:

Package:
installed: httpd, httpd-devel, ntp
removed: iptables6
Service:
started: httpd
File:
...


Please, no dump :)
You would have been displeased if 'ls -l' had returned its output in YAML ;)


Thank

Aurelien

winmail.dat

Luke Kanies

unread,
Jul 14, 2009, 5:24:17 PM7/14/09
to puppe...@googlegroups.com


Yeah, that's a good point. We've been talking about ways to make make
the basic interaction a bit better, but you're right that providing a
non-dump format is a good idea. How do you think we should handle
switching between the machine-readable and human-readable formats?

--
Love is a snowmobile racing across the tundra and then suddenly it
flips over, pinning you underneath. At night, the ice weasels come.
--Matt Groening

James Turnbull

unread,
Jul 14, 2009, 6:02:59 PM7/14/09
to puppe...@googlegroups.com
Luke Kanies wrote:
> Yeah, that's a good point. We've been talking about ways to make make
> the basic interaction a bit better, but you're right that providing a
> non-dump format is a good idea. How do you think we should handle
> switching between the machine-readable and human-readable formats?
>

Can't we do both? Have a --changes flag that does both and a
--changes-serial and --changes-text that outputs each variation? Add a
--serialisation-format option etc.

Regards

James Turnbull

signature.asc

Nigel Kersten

unread,
Jul 14, 2009, 6:05:59 PM7/14/09
to puppe...@googlegroups.com
On Tue, Jul 14, 2009 at 3:02 PM, James Turnbull<ja...@lovedthanlost.net> wrote:
> Luke Kanies wrote:
>> Yeah, that's a good point.  We've been talking about ways to make make
>> the basic interaction a bit better, but you're right that providing a
>> non-dump format is a good idea.  How do you think we should handle
>> switching between the machine-readable and human-readable formats?
>>
>
> Can't we do both?  Have a --changes flag that does both and a
> --changes-serial and --changes-text that outputs each variation? Add a
> --serialisation-format option etc.

++

It's going to be a structured format internally anyway.

I can see wanting both invocations regularly. One for end users, one
for automated data scraping and reporting.

>
> Regards
>
> James Turnbull
>
> --
> Author of:
> * Pro Linux Systems Administration
> (http://tinyurl.com/linuxadmin)
> * Pulling Strings with Puppet
> (http://tinyurl.com/pupbook)
> * Pro Nagios 2.0
> (http://tinyurl.com/pronagios)
> * Hardening Linux
> (http://tinyurl.com/hardeninglinux)
>
>

--
Nigel Kersten
nig...@google.com
System Administrator
Google, Inc.

Luke Kanies

unread,
Jul 14, 2009, 6:36:37 PM7/14/09
to puppe...@googlegroups.com
On Jul 14, 2009, at 3:05 PM, Nigel Kersten wrote:

>
> On Tue, Jul 14, 2009 at 3:02 PM, James Turnbull<ja...@lovedthanlost.net
> > wrote:
>> Luke Kanies wrote:
>>> Yeah, that's a good point. We've been talking about ways to make
>>> make
>>> the basic interaction a bit better, but you're right that
>>> providing a
>>> non-dump format is a good idea. How do you think we should handle
>>> switching between the machine-readable and human-readable formats?
>>>
>>
>> Can't we do both? Have a --changes flag that does both and a
>> --changes-serial and --changes-text that outputs each variation?
>> Add a
>> --serialisation-format option etc.
>
> ++
>
> It's going to be a structured format internally anyway.
>
> I can see wanting both invocations regularly. One for end users, one
> for automated data scraping and reporting.


Right, the question is, what's a good interface? I don't particularly
like supporting three settings for this one thing...

--
Real freedom lies in wildness, not in civilization.
-- Charles Lindbergh

Aurelien....@cea.fr

unread,
Jul 15, 2009, 10:29:06 AM7/15/09
to puppe...@googlegroups.com

Presently, puppetd can output 2 kinds of informations:
- first, a trace-like output with all the 'notice:', info and warning/err messages.
This is much more like a trace, usefull for understanding unexpected behaviour, but not really usefull for admins when they just want to know what was done and the errors.
- second, a human readible output, like --summarize. Really nice for admins, but usefull for automatic stuff.

We have two use cases for them:
- output for humans
- output for scripts

Whatever the output is, we must know whether the puppetd output will be parsed by some scripts or a human.
So, rather than having several flags for --changes, we must have a global switch for the whole output.

So:
-> Decide which information interest you:
--verbose/--trace/--debug/--summarize/--changes, etc...
-> Decide in which format you want them
default: human readable, --output[=yaml,jason,...]


Example:

Admin use:
# puppetd -o --no-daemonize --changes

Automatic use:
# puppetd -o --no-daemonize --changes --output=yaml | ./my-script --e-mail=...


By the way:
--no-daemonize is really boring to enter, please add a short option :)
--changes should also a short-cut also :)

Moreover:
To control trace verbosity, admin should play with 5 flags to decide what they want display:
--logdest console (only way I found to just display err/warn/notice)
--verbose (display err/warn/notice/info)
--trace (...)
--debug (...)
--evaltrace (...)
Maybe they should be groupped around something more classical like:
--level={err,warn,notice,info,debug,...}
with err=0 and debug=5
With logdest doing just what its name tell, putting log somewhere with no side effect.


So, this was some of my though about admin CLI use of puppetd.
- Control trace verbosity
- Display sumup of what happened
- Choose in which format you want them displayed.


Aurelien

-------- Message d'origine--------
De: puppe...@googlegroups.com de la part de Luke Kanies
Date: mer. 15/07/2009 00:36
À: puppe...@googlegroups.com
Objet : Re: RE : RE : [Puppet-dev] Re: [PATCH/puppet 1/1] Feature #2400 : add display changes and add them in YAML to the report
winmail.dat

David Schmitt

unread,
Jul 15, 2009, 10:40:56 AM7/15/09
to puppe...@googlegroups.com
Aurelien....@CEA.FR wrote:
> Presently, puppetd can output 2 kinds of informations:
> - first, a trace-like output with all the 'notice:', info and warning/err messages.
> This is much more like a trace, usefull for understanding unexpected behaviour, but not really usefull for admins when they just want to know what was done and the errors.
> - second, a human readible output, like --summarize. Really nice for admins, but usefull for automatic stuff.
>
> We have two use cases for them:
> - output for humans
> - output for scripts
>
> Whatever the output is, we must know whether the puppetd output will be parsed by some scripts or a human.
> So, rather than having several flags for --changes, we must have a global switch for the whole output.
>
> So:
> -> Decide which information interest you:
> --verbose/--trace/--debug/--summarize/--changes, etc...
> -> Decide in which format you want them
> default: human readable, --output[=yaml,jason,...]
>
>
> Example:
>
> Admin use:
> # puppetd -o --no-daemonize --changes
>
> Automatic use:
> # puppetd -o --no-daemonize --changes --output=yaml | ./my-script --e-mail=...
> Moreover:
> To control trace verbosity, admin should play with 5 flags to decide what they want display:
> --logdest console (only way I found to just display err/warn/notice)
> --verbose (display err/warn/notice/info)
> --trace (...)
> --debug (...)
> --evaltrace (...)
> Maybe they should be groupped around something more classical like:
> --level={err,warn,notice,info,debug,...}
> with err=0 and debug=5
> With logdest doing just what its name tell, putting log somewhere with no side effect.
>
> So, this was some of my though about admin CLI use of puppetd.
> - Control trace verbosity
> - Display sumup of what happened
> - Choose in which format you want them displayed.
>

+1

>
> By the way:
> --no-daemonize is really boring to enter, please add a short option :)
> --changes should also a short-cut also :)

There should probably be proper defaults when using --test (which
already contains --no-daemonize).

Regards, DavidS

Luke Kanies

unread,
Jul 15, 2009, 9:00:09 PM7/15/09
to puppe...@googlegroups.com
-D theoretically works, but I just tried and it didn't actually work. :/

>
> --changes should also a short-cut also :)

Settings theoretically support a 'short' option but it appears to be
broken, so we could specify that -C would be the short option here, if
we fix the short options.

>
> Moreover:
> To control trace verbosity, admin should play with 5 flags to decide
> what they want display:
> --logdest console (only way I found to just display err/warn/notice)
> --verbose (display err/warn/notice/info)
> --trace (...)
> --debug (...)
> --evaltrace (...)
> Maybe they should be groupped around something more classical like:
> --level={err,warn,notice,info,debug,...}
> with err=0 and debug=5
> With logdest doing just what its name tell, putting log somewhere
> with no side effect.
>
>
> So, this was some of my though about admin CLI use of puppetd.
> - Control trace verbosity
> - Display sumup of what happened
> - Choose in which format you want them displayed.

I agree with all of your points.

Any interest in helping us to fix these?
> <winmail.dat>


--
Beware of all enterprises that require new clothes.
-- Henry David Thoreau

Aurelien....@cea.fr

unread,
Jul 16, 2009, 11:14:24 AM7/16/09
to puppe...@googlegroups.com

Hello

I'm interest by helping fixing this but I'm not sure I will have time to dig into Puppet code.
Moreover, it seems puppet arguments are a bit tricky because lot of arguments are not handled in the main script, but just added to Puppet[:xxx] and test at any place in source code, their difficult to track, and mainly to modify (I do not want to be the guy you will change all the 'if Puppet[:trace]' with 'if current loglevel is greater or equal to trace' :p)

Regarding to short opts, they seems to work, it just seems than in 0.24.8, in puppetd, -D flag is never tested.

What do you think of:
1 - fix -D
2 - use Puppet::Utils::Log.level for all trace output
3 - Add --loglevel|-L option (possible value are err,warn,notice,info,debug,trace,evaltrace)
4 - (difficult part) fix the :trace :evaltrace use in puppet code... :/

Other things should wait for Event code rewritting.




Aurelien

-------- Message d'origine--------
De: puppe...@googlegroups.com de la part de Luke Kanies

Date: jeu. 16/07/2009 03:00
À: puppe...@googlegroups.com
Objet : Re: RE : RE : RE : [Puppet-dev] Re: [PATCH/puppet 1/1] Feature #2400 : add display changes and add them in YAML to the report

James Turnbull

unread,
Jul 16, 2009, 6:16:11 PM7/16/09
to puppe...@googlegroups.com
Luke Kanies wrote:
>>
>> By the way:
>> --no-daemonize is really boring to enter, please add a short option :)
>
> -D theoretically works, but I just tried and it didn't actually work. :/

Actually looking at the code I don't see a short option for no-daemonize
in the new Application class.

signature.asc

Luke Kanies

unread,
Jul 16, 2009, 6:18:00 PM7/16/09
to puppe...@googlegroups.com
On Jul 16, 2009, at 8:14 AM, <Aurelien....@CEA.FR> wrote:

> Hello
>
> I'm interest by helping fixing this but I'm not sure I will have
> time to dig into Puppet code.
> Moreover, it seems puppet arguments are a bit tricky because lot of
> arguments are not handled in the main script, but just added to
> Puppet[:xxx] and test at any place in source code, their difficult
> to track, and mainly to modify (I do not want to be the guy you will
> change all the 'if Puppet[:trace]' with 'if current loglevel is
> greater or equal to trace' :p)
>

Really, we just need a common method that handles all of the trace
logic. At this point, it probably makes the most sense to monkey-
patch Exception, and then replace all of the instances of
Puppet[:trace] usage with a call to this new method.

I kinda wish it made sense to have some hook on exception creation,
but do we really want a stack trace for every single exception, or
just at certain points?

I think I missed something, though; I don't really understand your
point about trace vs. loglevel. Are you saying we get rid of --trace
and just always print them if --debug is enabled or something?
>
> Regarding to short opts, they seems to work, it just seems than in
> 0.24.8, in puppetd, -D flag is never tested.
>
> What do you think of:
> 1 - fix -D
>

Definitely a good idea, but... we shouldn't need to test for -D
specifically, it should set Puppet[:daemonize] like any other setting.
>
> 2 - use Puppet::Utils::Log.level for all trace output
> 3 - Add --loglevel|-L option (possible value are
> err,warn,notice,info,debug,trace,evaltrace)
>

Can you explain this more completely?

>
> 4 - (difficult part) fix the :trace :evaltrace use in puppet
> code... :/
>

What behaviour are you wanting here?
--
A conservative is a man who believes that nothing should be done for
the first time. --Alfred E. Wiggam

Luke Kanies

unread,
Jul 16, 2009, 6:19:47 PM7/16/09
to puppe...@googlegroups.com
On Jul 16, 2009, at 3:16 PM, James Turnbull wrote:

> Luke Kanies wrote:
>>>
>>> By the way:
>>> --no-daemonize is really boring to enter, please add a short
>>> option :)
>>
>> -D theoretically works, but I just tried and it didn't actually
>> work. :/
>
> Actually looking at the code I don't see a short option for no-
> daemonize
> in the new Application class.

It's in defaults.rb:

:daemonize => { :default => true,
:desc => "Send the process into the background. This is
the default.",
:short => "D"
},

The applications don't define this setting, they just test for it in
Puppet[:daemonize], which should correctly be set by our getopt parser
interpreting -D.

--
I think that all good, right thinking people in this country are sick
and tired of being told that all good, right thinking people in this
country are fed up with being told that all good, right thinking people
in this country are fed up with being sick and tired. I'm certainly
not, and I'm sick and tired of being told that I am.
-- Monty Python

James Turnbull

unread,
Jul 16, 2009, 6:23:00 PM7/16/09
to puppe...@googlegroups.com
Luke Kanies wrote:
>
> The applications don't define this setting, they just test for it in
> Puppet[:daemonize], which should correctly be set by our getopt parser
> interpreting -D.
>

But the getopt parser in application.rb only seems to handle long options.

# used to declare code that handle an option
def option(*options, &block)
long = options.find { |opt| opt =~ /^--/
}.gsub(/^--(?:\[no-\])?([^ =]+).*$/, '\1' ).gsub('-','_')
fname = "handle_#{long}"
if (block_given?)
meta_def(symbolize(fname), &block)
else
meta_def(symbolize(fname)) do |value|
self.options["#{long}".to_sym] = value
end
end
@opt_parser.on(*options) do |value|
self.send(symbolize(fname), value)
end
end

Unless I am misreading this?

signature.asc

Luke Kanies

unread,
Jul 16, 2009, 6:25:10 PM7/16/09
to puppe...@googlegroups.com

Ah, I see. That's what needs to be fixed, then.

--
A government that robs Peter to pay Paul can always depend on the
support of Paul. -- George Bernard Shaw

Brice Figureau

unread,
Jul 17, 2009, 3:07:07 AM7/17/09
to puppe...@googlegroups.com

I don't think this part of the code is at fault (I didn't test anything
yet).
I think it is when we convert the Pupppet::Settings defaults to
OptionParser.
I'll fix this ASAP.
Thanks,
--
Brice Figureau
My Blog: http://www.masterzen.fr/

David Schmitt

unread,
Jul 17, 2009, 6:33:00 AM7/17/09
to puppe...@googlegroups.com
Luke Kanies wrote:
> On Jul 16, 2009, at 8:14 AM, <Aurelien....@CEA.FR> wrote:
>
>> Hello
>>
>> I'm interest by helping fixing this but I'm not sure I will have
>> time to dig into Puppet code.
>> Moreover, it seems puppet arguments are a bit tricky because lot of
>> arguments are not handled in the main script, but just added to
>> Puppet[:xxx] and test at any place in source code, their difficult
>> to track, and mainly to modify (I do not want to be the guy you will
>> change all the 'if Puppet[:trace]' with 'if current loglevel is
>> greater or equal to trace' :p)
>>
>
> Really, we just need a common method that handles all of the trace
> logic. At this point, it probably makes the most sense to monkey-
> patch Exception, and then replace all of the instances of
> Puppet[:trace] usage with a call to this new method.
>
> I kinda wish it made sense to have some hook on exception creation,
> but do we really want a stack trace for every single exception, or
> just at certain points?
>
> I think I missed something, though; I don't really understand your
> point about trace vs. loglevel. Are you saying we get rid of --trace
> and just always print them if --debug is enabled or something?

That's what I've read from Aurelien's mails too. But I think exception
traces should always be printed if they signal no known error (in which
case a proper error message is more helpful than the stack trace).

Implementing that should be as easy as setting Puppet[:trace] always to
true and (not|only) removing the ifs.


Regards, DavidS

Aurelien....@cea.fr

unread,
Jul 17, 2009, 11:28:52 AM7/17/09
to puppe...@googlegroups.com

.Regarding --trace I was just saying that, from my point of view, trace is some debugging information, more precise than just debug, and so, must be an upper level of logging verbosity.

loglevel could have values starting from:
0-nothing
1-err
2-warn (dislays err and warn messages)
3-notice (err + warn + notice)
4-info (above + info)
5-debug (above + debug)
6-trace (above + trace)
(and maybe more...)

Something classic in logging system like
http://www.ruby-doc.org/core/classes/Logger.html

.About -D, my point was to have a short options for --no-daemonize, if -D means daemonize, this is not really usefull because that the standard behaviour of puppetd (what I would expect from something whose named ends with a letter 'd').
So 'puppetd' == 'puppetd -D' :)

If you use puppetd as a daemon, short options is not so important because you do not starts daemon by hand every days, start scripts will do it for you.
But, if you use puppetd as a CLI command (what I do every times), short options is really usefull. And working with puppetd as a CLI tools, means, at least: "puppetd -o --no-daemonize"
That's just a simple letter alias for --no-daemonize :)


Hoping it is clearer now :)



Aurelien


-------- Message d'origine--------
De: puppe...@googlegroups.com de la part de Luke Kanies

Date: ven. 17/07/2009 00:18
À: puppe...@googlegroups.com
Objet : Re: RE : RE : RE : RE : [Puppet-dev] Re: [PATCH/puppet 1/1] Feature #2400 : add display changes and add them in YAML to the report

Brice Figureau

unread,
Jul 17, 2009, 2:11:49 PM7/17/09
to puppe...@googlegroups.com
Hi,

On 16/07/09 3:00, Luke Kanies wrote:
> On Jul 15, 2009, at 7:29 AM, <Aurelien....@CEA.FR> wrote:
>> By the way:
>> --no-daemonize is really boring to enter, please add a short option :)
>
> -D theoretically works, but I just tried and it didn't actually work. :/

I just tried on 0.25 and traced all the way and -D works fine. But used
with --test on puppetd, then --test is winning because it is processed
afterward. I bet it's what you tested (ie puppetd --test -D), or is it
that you wanted it to mean "--no-daemonize" ?

So I looked to 0.24.8, and it has exactly the same behavior, so I did
the right thing when I applied the application stuff :-)

Now the question is: should we change this behaviour and have --test
processed at the moment it appears on the command line (so it can be
overriden by other parameters)?

[snipped last part of discussion]

Aurelien....@cea.fr

unread,
Jul 17, 2009, 5:08:07 PM7/17/09
to puppe...@googlegroups.com

See my previous e-mails, indeed I mean --no-daemonize.
'puppetd -D' does the same as 'puppetd' because it is the default, not really usefull :) even if not useless.


-------- Message d'origine--------
De: puppe...@googlegroups.com de la part de Brice Figureau
Date: ven. 17/07/2009 20:11
À: puppe...@googlegroups.com
Objet : [Puppet-dev] Re: -D not working (was Feature #2400 : add display changes and add them in YAML to the report)
winmail.dat

Luke Kanies

unread,
Jul 17, 2009, 5:48:21 PM7/17/09
to puppe...@googlegroups.com
On Jul 17, 2009, at 11:11 AM, Brice Figureau wrote:

>
> Hi,
>
> On 16/07/09 3:00, Luke Kanies wrote:
>> On Jul 15, 2009, at 7:29 AM, <Aurelien....@CEA.FR> wrote:
>>> By the way:
>>> --no-daemonize is really boring to enter, please add a short
>>> option :)
>>
>> -D theoretically works, but I just tried and it didn't actually
>> work. :/
>
> I just tried on 0.25 and traced all the way and -D works fine. But
> used
> with --test on puppetd, then --test is winning because it is processed
> afterward. I bet it's what you tested (ie puppetd --test -D), or is it
> that you wanted it to mean "--no-daemonize" ?

Yeah, I realized that -D == --daemonize, which is not actually what we
want. Duh. My bad.

>
> So I looked to 0.24.8, and it has exactly the same behavior, so I did
> the right thing when I applied the application stuff :-)
>
> Now the question is: should we change this behaviour and have --test
> processed at the moment it appears on the command line (so it can be
> overriden by other parameters)?


I think the main thing was not having a short form of --no-daemonize,
and I don't think the current system will provide an easy way to add a
short alias for disabling booleans.

--
Criminal: A person with predatory instincts who has not sufficient
capital to form a corporation. --Howard Scott

Brice Figureau

unread,
Jul 17, 2009, 5:59:56 PM7/17/09
to puppe...@googlegroups.com

Yes, that won't be easy, mainly because I don't know how to represent a
no-short option (ie +D instead of -D, that feel, hu, awkward).
I'm wondering what's the posix way of handling this, if there's one...
I assigned you #2420, feel free to reject or bounce it back to me if you
want me to try to fix it.
Also, note that -D is the only short option we have for the whole
defaults settings (I mean outside of apps). Getting rid of it will,
somehow _generalize_ the options :-)

Luke Kanies

unread,
Jul 17, 2009, 6:19:52 PM7/17/09
to puppe...@googlegroups.com

--no-D? :)

>
> I'm wondering what's the posix way of handling this, if there's one...
> I assigned you #2420, feel free to reject or bounce it back to me if
> you
> want me to try to fix it.

Will do.

>
> Also, note that -D is the only short option we have for the whole
> defaults settings (I mean outside of apps). Getting rid of it will,
> somehow _generalize_ the options :-)

Weird. We really just need to use them a lot more, rather than
getting rid of the one we have.

--
The whole secret of life is to be interested in one thing profoundly
and in a thousand things well. -- Horace Walpole

Aurelien Degremont

unread,
Jul 23, 2009, 2:21:07 AM7/23/09
to puppe...@googlegroups.com
Luke Kanies a écrit :

>> I'm wondering what's the posix way of handling this, if there's one...

I do not think there is a POSIX way for that.
At least there is a GNU way.

In fact, the behaviour --foo and --no-foo <=> --foo=no it's not
something we can call "standard", this is more puppetish.

So up to us to define a Puppet way to attribute short options for them.
One proposition could be

Assign a letter, lowercase means 'yes', uppercase means 'no'
By example:

--daemonize|-d
--no-daemonize|-D

(but this example conflicts with --debug|-d)

>> Also, note that -D is the only short option we have for the whole
>> defaults settings (I mean outside of apps). Getting rid of it will,
>> somehow _generalize_ the options :-)
> Weird. We really just need to use them a lot more, rather than
> getting rid of the one we have.

If this is the only option using this trick, as it behaves like the
default option, I quite sure near nobody use it.
So, it is possible to redefined how this is handled.


Another possibility, is to define special args, in 'puppetd' and
'puppet' scripts, on a case by case basis.

--
Aurelien Degremont
CEA

Luke Kanies

unread,
Jul 23, 2009, 2:56:55 AM7/23/09
to puppe...@googlegroups.com
On Jul 22, 2009, at 11:21 PM, Aurelien Degremont wrote:

>
> Luke Kanies a écrit :
>>> I'm wondering what's the posix way of handling this, if there's
>>> one...
>
> I do not think there is a POSIX way for that.
> At least there is a GNU way.
>
> In fact, the behaviour --foo and --no-foo <=> --foo=no it's not
> something we can call "standard", this is more puppetish.

I've seen this somewhere before, I'm pretty sure, but I don't know
where.

>
> So up to us to define a Puppet way to attribute short options for
> them.
> One proposition could be
>
> Assign a letter, lowercase means 'yes', uppercase means 'no'
> By example:
>
> --daemonize|-d
> --no-daemonize|-D
>
> (but this example conflicts with --debug|-d)

Yeah. And even then, we need a way to specify short inverse options,
which our system doesn't currently provide.

>
>>> Also, note that -D is the only short option we have for the whole
>>> defaults settings (I mean outside of apps). Getting rid of it will,
>>> somehow _generalize_ the options :-)
>> Weird. We really just need to use them a lot more, rather than
>> getting rid of the one we have.
>
> If this is the only option using this trick, as it behaves like the
> default option, I quite sure near nobody use it.
> So, it is possible to redefined how this is handled.

True. How would you like to change it?

>
> Another possibility, is to define special args, in 'puppetd' and
> 'puppet' scripts, on a case by case basis.

Yeah. That's probably the best option in the short term.

--
It is absurd to divide people into good and bad. People are either
charming or tedious. -- Oscar Wilde

Reply all
Reply to author
Forward
0 new messages