[PE-17491] Do not fail on install when a restart exit code is returned #200
Conversation
wilson208
changed the title
| @@ -44,11 +44,13 @@ def modify_features(features, action) | |||
| if not_nil_and_not_empty? features | |||
| debug "#{action.capitalize}ing features '#{features.join(',')}'" | |||
| cmd_args, obfuscated_strings = build_cmd_args(features, action) | |||
| acceptable_exit_codes = [0] | |||
| acceptable_exit_codes << 3010 if action == 'install' | |||
There was a problem hiding this comment.
Also applicable to uninstall
lib/puppet/provider/sqlserver.rb
Outdated
|
|
||
| obfuscate_strings.each {|str| msg.gsub!(str, '**HIDDEN VALUE**') } unless obfuscate_strings.nil? | ||
|
|
||
| raise Puppet::Error, msg | ||
| end | ||
|
|
||
| warn("Execution of '#{command}' returned exit code 3010 - restart required") if res.exitstatus == 3010 |
There was a problem hiding this comment.
Would it make more sense to move the warning to the install/uninstall code and use the result exitcode?
There was a problem hiding this comment.
Will get that done.
| execute(command.compact) | ||
| rescue Puppet::ExecutionFailure => error | ||
| def try_execute(command, msg = nil, obfuscate_strings = nil, acceptable_exit_codes = [0]) | ||
| res = execute(command.compact, failonfail: false) |
There was a problem hiding this comment.
This has changed behaviour. Previously this was in a begin rescue block
There was a problem hiding this comment.
I know this has changed behaviour, however the error which is raised and surfaced in the rescue block does not contain the exit code to be able to handle this there.
There was a problem hiding this comment.
@glennsarti are you happy with this solution? I have tried to replicate the error message which is raised by the execute function when a non-zero exit code is returned, see here: [link].(https://github.com/puppetlabs/puppet/blob/master/lib/puppet/util/execution.rb#L235)
I know this is a change in how the try_execute method functions, however the behaviour to the user is exactly the same and the error messages surfaced are exactly the same, except for allowing non-zero exit codes.
There was a problem hiding this comment.
Without the begin rescue block, the obfuscate strings later on never gets called. This will regress the Password leaking issue
There was a problem hiding this comment.
Oh wait...I see you have failonfail set to false
|
I know this PR needs work still, was pushing at the end of my day just to show what I have done. I have one question, should the acceptable_exit_codes be something that is set by a parameter in the type/provider, defaulting to 0 (current behaviour)? |
|
Hrmmm not sure. I'm leaning to no. This is the only case so far where the exit code has been issue. Technically there is also 1641 (Reboot has occurred) to worry about. |
wilson208
changed the title
|
@glennsarti I have made changes based on your feedback and got travis and appveyor green. I have this running through SQL server adhoc pipeline at the moment also. |
| res = try_execute(cmd_args, "Unable to #{action} features (#{features.join(', ')})", nil, [0, 1641, 3010]) | ||
|
|
||
| warn("#{action} of features (#{features.join(', ')} returned exit code 3010 - reboot required") if res.exitstatus == 3010 | ||
| warn("#{action} of features (#{features.join(', ')} returned exit code 1641 - reboot occurred") if res.exitstatus == 1641 |
There was a problem hiding this comment.
1641 is more reboot scheduled
|
Also missing spec tests for a mocked 3010 and 1641 response. |
When exit code 3010 is returned from setup.exe upon install the puppet run failed even though the install succeeded, the 3010 exit code just signifies that some changes are pending and a restart is required. This change allows the puppet run to succeed when the install returns 3010 exit code and simply raises a warning.
|
@glennsarti have updated this to have correct wording of reboot initiated for 1641 (from here) and added spec tests which exercise all of the warnings and ensure the warnings are not generated when 0 exit code is produced. |
|
@wilson208 Ready to rock! |
When exit code 3010 is returned from setup.exe upon install the puppet run failed even though the install succeeded, the 3010 exit code just signifies that some changes are pending and a restart is required.
This change allows the puppet run to succeed when the install returns 3010 exit code and simply raises a warning.