Define _sshpass_available (fixes #46)

[moqmar]

See #46

That method was implemented in https://github.com/chifflier/ansible-lxc-ssh/pull/1/files#diff-d5708b826a77f4da31d0b14b8c7c491973a414658f496a9f2b687745c3b9a2c1R58-R75. A part of the PR was incorporated into this fork in 4e569a7, but the _sshpass_available method seems to be missing. Adding it together with the SSHPASS_AVAILABLE variable fixes the issue.

[andreasscherbaum]

@moqmar Looks like the lint test doesn't like your syntax. Can you fix that, please?

[andreasscherbaum]

@moqmar We merged the other outstanding PRs. See #38. Can you please check if this PR is still needed, and if yes, resolve the conflicts? Thanks!

[moqmar]

Can you please check if this PR is still needed

https://github.com/andreasscherbaum/ansible-lxc-ssh/blob/master/lxc_ssh.py#L666 (wow, line 666) still tries to call the function which doesn't exist. I have rebased & tried to fix the linter errors, let's see if actions deems it alright. 🙃

[andreasscherbaum]

Hmm, probably, likely no one tests with passwords because this is anti-automation ...

[andreasscherbaum]

@moqmar Any chance this can be unit tested if it's working? Maybe by either removing the tool after the successful run, or not installing it first and then run another test?

[moqmar]

Puuh, it was intended as a quick fix from my side. I see two possibilities which are both out of scope IMO:

• test the _sshpass_available method with python directly: I'd move the "sshpass" into a sshpass_command parameter & call it once with "sshpass", then set SSHPASS_AVAILABLE back to None and once again with "sshpass-that-definitely-doesn't-exist"
  • for that we'd need to create a place for actual Python unit tests though, which is a whole different issue
• test if SSH password authentication actually works correctly using the existing unit testing strategy
  • that would test way more than just this PR - it wouldn't be just this function that's tested, but rather the whole block where it's used: https://github.com/andreasscherbaum/ansible-lxc-ssh/blob/32854291ec1fb27c29a6d85d9b2bb5da568e9dcb/lxc_ssh.py#L691-L714
  • I think that this is generally a good idea, but I don't understand exactly how the unit tests work here and am just a user of the plugin - my main concern is whether it's usable or not. If this was a new feature PR, I'd agree to write unit tests for it, but this one fixes an actual syntax error (object has no attribute) in an existing feature using existing code.

[andreasscherbaum]

I get your point about the fix, and to avoid any such bugs and mistakes in the feature it seems to be a good idea to cover this by tests. A future change might break this functionality again, and apparently not many users are using passwords. Also given that passwords are used, it might be a good idea in general to test if using a password works.

My test idea in this comment consists of two tests:

Version 1:

1. Run the test with password which is expected to succeed
2. Uninstall sshpass
3. Run the test again, expect it to fail (which passes the test)

Version 2:

1. Ensure sshpass is not installed, or disable it
2. Run the test, expect it to fail
3. Install/enable sshpass
4. Run the test again, with password, and expect it to succeed

Version 1 seems to be easier, as it runs a successful test first, and can already spot any problems on this front.

[andreasscherbaum]

@moqmar Sorry, since you don't have any commits in this repo, GitHub asks me to approve every new commit in your PR. I will see that I do this as soon as possible.

[moqmar]

I'm still trying to make the tests work as intended, I'll let you know when they work - I'm running the actions on my fork for now.

[moqmar]

Alright, finally got around to implementing the tests - sorry, I somehow didn't saw the GitHub Actions so I thought the tests were purely the Ansible playbook, which would have made that much more complicated. :see_no_evil:

I've now tested it and it should be working - just got an issue with GitHub Actions right now where it can't find the LXC container IP - can you try to run it here maybe?

An older commit passed everything (https://github.com/moqmar/ansible-lxc-ssh/actions/runs/2207203094), but now it only gets to "Fail if no network available" (https://github.com/moqmar/ansible-lxc-ssh/actions/runs/2207414730), even though I haven't changed anything before that - it even broke on master for some reason... :/

[andreasscherbaum]

The lint is still failing, but let's see about that once everything else is working.

[moqmar]

Yup, lint wasn't anything related to this PR so I didn't change that. And that network issue is really weird :/

Edit: maybe you can try if you get the same issue on master as well now?

[moqmar]

Oof... Running dhclient manually fixes this, but no idea where it came from in the first place. Finally works now \o/

[andreasscherbaum]

Looks good, it seems. I will do a detailed review later when I have time. Just wanted to make sure your tests run timely.

[andreasscherbaum]

@moqmar Just found one small issue or open question: the Test ssh connection into root user with password authentication test throws an Load key "/dev/null": invalid format error. That doesn't seem right.

[moqmar]

That is intentional - when not specifying -i, SSH defaults to a couple of default paths which includes the existing working key. So, specifying it with an invalid key file forces password authentication. PreferredAuthentications would be another way to do that, but I wanted to keep it as close as possible to just not having a key for the server.