(#19805) Report failures to restart in exit code if requested#1541
(#19805) Report failures to restart in exit code if requested#1541lkundrak wants to merge 1 commit intopuppetlabs:masterfrom
Conversation
Documentation to --detailed-exitcodes states that mask 0x4 is applied in case
"there were failures during the transaction." Currently only failures to change
resource state are reported, failures to restart the resource are still ignored.
Only change (code=2) to Exec['/bin/true'] is considered here:
$ puppet apply --detailed-exitcodes -e 'service { "duckfaced": restart => "/bin/false", ensure => running, status => "/bin/true" } exec { "/bin/true": notify => Service["duckfaced"] }'
$ echo $?
2
Needless to say, information about failed restarts is usually as important to
the user as the status changes, since they usually mean that desired
configuration is not in effect.
This change makes Puppet consider information about failed restarts when
setting the failure bit.
Another possibility to avoid loosing the information would be to introduce
another bit in the exit code mask, but that would change semantics with respect
to existing documentation and thus probably be more confusing.
|
CLA Signed by lkundrak on 2012-08-11 21:00:00 -0700 |
There was a problem hiding this comment.
Since this line needs to be wrapped due to length, it would probably be more readable to do this with an if/elsif block.
|
Thank you for this contribution! This is definitely a change that would be good to have, I have a small comment that I've attached inline in the commit. In addition this doesn't have tests; it would be good to have some test coverage for this change. I've attached a basic primer to writing tests below. TestsWhy do we need tests?Unit tests for behavior changes serve a number of benefits. First, unit tests help explain the intended behavior. Some changes can have some very subtle effects that aren't immediately clear, and unit tests help explain changes by showing the intended effect. Second, by testing the expected behavior unit tests help catch regressions. If the original change is rolled back somehow the tests will fail, which lets us know about regressions sooner and gives us assurances that additional changes don't break existing behavior. Getting startedIf you're not familiar with rspec or writing unit tests, looking at existing tests is one of the best ways to get started. In this case you'll want to look at https://github.com/puppetlabs/puppet/blob/master/spec/unit/transaction/report_spec.rb#L126 . If you would like a more comprehensive overview of rspec, documentation is available at https://www.relishapp.com/rspec/. Getting helpIf you would like help with this pull request, #puppet-dev on irc.freenode.net is an excellent resource. There are a number of experienced developers available hours in that channel, and I'm generally available from 9:00 - 17:00 GMT -7, and my IRC handle is finch. |
|
Merged into master as d5454f6. This should be released in 3.2.0. Thanks again for the contribution! -Adrien |
3 participants
Documentation to --detailed-exitcodes states that mask 0x4 is applied in case
"there were failures during the transaction." Currently only failures to change
resource state are reported, failures to restart the resource are still ignored.
Only change (code=2) to Exec['/bin/true'] is considered here:
$ puppet apply --detailed-exitcodes -e 'service { "duckfaced": restart => "/bin/false", ensure => running, status => "/bin/true" } exec { "/bin/true": notify => Service["duckfaced"] }'
$ echo $?
2
Needless to say, information about failed restarts is usually as important to
the user as the status changes, since they usually mean that desired
configuration is not in effect.
This change makes Puppet consider information about failed restarts when
setting the failure bit.
Another possibility to avoid loosing the information would be to introduce
another bit in the exit code mask, but that would change semantics with respect
to existing documentation and thus probably be more confusing.