use a stricter comparison so knife ssh only fails if --exit-on-error

[sarkis]

Description

This is required due to config[:on_error] being set here:

chef/lib/chef/knife/ssh.rb

Line 134 in 93fe1ac

config[:on_error] ||= :skip

. This conditional will allow ssh_commands that return non-zero status to be executed on entire pool of servers returned by the knife query when --exit-on-error is NOT specified.

Issues Resolved

#6581

Check List

• New functionality includes tests
• All tests pass
• RELEASE_NOTES.md, has been updated if required (not required for bugfixes, required for API changes)
• All commits have been signed-off for the Developer Certificate of Origin. See https://github.com/chef/chef/blob/master/CONTRIBUTING.md#developer-certification-of-origin-dco

[coderanger]

That whole option seems like something is very wrong. It's declared as a boolean type but in some places we expect a symbol. And the proc on it doesn't actually check the key value which means it doesn't actually matter if you set the option or not.

[sarkis]

@coderanger thanks for taking a look -
I think the way it's meant to be used where --exit-on-error is set (true) or not (false) - we should keep this as a boolean. I simplified this a bit - not sure the proc is necessary as you mentioned in the previous comment. Let me know what you think of this approach when you have a moment.

I did test and make sure this new way of doing it is working right.. the one thing I didn't test is if it encounters an ssh connection error - trying to get that tested now.

[sarkis]

I was able to successfully test all edge cases I could think of here - including testing ssh connection issues to one of the servers returned in the knife ssh query.

[lamont-granquist]

were we trying too hard to be backcompat at some point and only half converted it from boolean to a multi-valued symbol?

[stevendanna]

@lamont-granquist The other way around. It started as multi-value config that was really only used when calling this plugin from other plugins like this:

https://github.com/chef/chef/blob/master/lib/chef/knife/bootstrap.rb#L451

I'm in favor of moving it to a boolean if we don't think anyone else is using it. If we do move it to a boolean, lets fixup the usage in bootstrap above as well.

[sarkis]

Since this was set to :raise in bootstrap - i wanted to leave the same behavior which after moving to boolean would be the same as set to true. Hopefully, I didn't misinterpret this ... as far as I can tell that covers all instances of ssh.confg[:on_error] in this repo - let me know if a PR may be required elsewhere...

[mal]

Constantly running into this and having to remember to add || true to my commands 🙁 Tested out this patch and can confirm it fixes the basic knife ssh use case. Thanks for the patch, it'd be really good to see it in the next stable 👍

[thommay]

[thommay]

@sarkis thanks for your contribution!

[lock]

This thread has been automatically locked because it has not had recent activity. Please open a new issue for related bugs and link to relevant comments in this thread.