[Freeipa-devel] [freeipa PR#797][opened] ipa-replica-conncheck: handle ssh not installed

[flo-renaud]

URL: https://github.com/freeipa/freeipa/pull/797
Author: flo-renaud
Title: #797: ipa-replica-conncheck: handle ssh not installed
Action: opened

PR body:
"""
When ipa-replica-conncheck is run but ssh is not installed, the tool exits
with a stack trace. Properly handle the error by raising an Exception.

https://pagure.io/freeipa/issue/6935
"""

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/797/head:pr797
git checkout pr797

[martbab]

URL: https://github.com/freeipa/freeipa/pull/797
Title: #797: ipa-replica-conncheck: handle ssh not installed

martbab commented:
"""
I have a small inline comment. I also miss any processing of the raised exception in the caller. If we want to skip SSH check when ssh is not installed (and we may argue about that), the called needs to catch RuntimeError from SshExec and log at warning/error level.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/797#issuecomment-302628027

[flo-renaud]

URL: https://github.com/freeipa/freeipa/pull/797
Author: flo-renaud
Title: #797: ipa-replica-conncheck: handle ssh not installed
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/797/head:pr797
git checkout pr797

[flo-renaud]

URL: https://github.com/freeipa/freeipa/pull/797
Title: #797: ipa-replica-conncheck: handle ssh not installed

flo-renaud commented:
"""
Hi @martbab
thank you for the review, I agree with your comments. My plan was to raise the RuntimeException so that ipa-replica-conncheck exits on error when ssh is not installed (same behavior as if ssh exits on error). The output would be:
Retrying using SSH...
ERROR: Remote master check failed with the following error message:
Could not SSH to remote host, ssh not installed
and exit code would be 1.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/797#issuecomment-302694825

[martbab]

URL: https://github.com/freeipa/freeipa/pull/797
Title: #797: ipa-replica-conncheck: handle ssh not installed

martbab commented:
"""
I was quite not sure what to do when SSH is not installer because the intent of the original code (at least from what I inferred) was that the absence of SSHd should trigger a warning message and skip SSH checks. Your commit changes the behavior so that this state is a hard error.

Shouldn't we first keep the original behavior?
"""

See the full comment at https://github.com/freeipa/freeipa/pull/797#issuecomment-302743786

[flo-renaud]

URL: https://github.com/freeipa/freeipa/pull/797
Author: flo-renaud
Title: #797: ipa-replica-conncheck: handle ssh not installed
Action: synchronized

To pull the PR as Git branch:
git remote add ghfreeipa https://github.com/freeipa/freeipa
git fetch ghfreeipa pull/797/head:pr797
git checkout pr797

[flo-renaud]

URL: https://github.com/freeipa/freeipa/pull/797
Title: #797: ipa-replica-conncheck: handle ssh not installed

flo-renaud commented:
"""
Hi @martbab
I initially thought that the code never worked and always triggered an exception, but you are right, it used to skip the check when ssh client is not installed. I updated the PR to revert to the original behavior.
"""

See the full comment at https://github.com/freeipa/freeipa/pull/797#issuecomment-303075145