openssh_keypair: prevent error when path includes no path

[felixfontein]

SUMMARY

Prevents error when path includes no path, only a filename (i.e. references to a file in cwd).

ISSUE TYPE

• Bugfix Pull Request

COMPONENT NAME

openssh_keypair

[ansibot]

cc @Spredzy @lolcube
click here for bot help

[MarkusTeufelberger]

Should this stuff really work with relative paths?

[felixfontein]

Yes, why should it not?

[felixfontein]

(Main use case is local_action, I guess.)

[MarkusTeufelberger]

Well, the module would write private keys somewhere and while I would usually expect to have ansible modules execute in $HOME on a remote machine I'm not sure if the path where it will run is actually specified...?

[felixfontein]

With local_action (or hosts: localhost), it will write to the CWD. Supporting this allows to write a little playbook to create a keypair in the current directory. Might be a lot nicer than writing a shell script which does the same than this module :)

[gundalow]

Wonder if it would be worth updating the docs for path to make this clearer.

[Akasurde]

Add a note here stating that - where we failed to find the file.

The local/remote directory '%s' ...

[felixfontein]

That's already there in line 279. Currently, when base_dir is the empty string, it doesn't read very nice, though (The directory does not exist or the file is not a directory -- watch the double space).

[MarkusTeufelberger]

Updating the docs for path would change this for all modules that use this documentation snippet. Do modules generally accept relative and absolute paths?

Also, since the module is only added in the next version, I'm not sure if a changelog entry is needed, but it won't hurt for sure.

For the record, I don't feel very strongly either way on the relative path issue, but I would consider it bad style if I saw it in a playbook that I would expect to be idempotent (unless I've missed something in the docs, as I said I think the working directory of Ansible is not strongly defined and could change).

[felixfontein]

Updating the docs for path would change this for all modules that use this documentation snippet.

I wouldn't do that.

Do modules generally accept relative and absolute paths?

Well, all the ones I used / worked on so far don't do such checks on paths, so they do work fine.

Also, since the module is only added in the next version, I'm not sure if a changelog entry is needed, but it won't hurt for sure.

Ah, good point. I completely forgot about that. I'll remove the changelog fragment!

For the record, I don't feel very strongly either way on the relative path issue, but I would consider it bad style if I saw it in a playbook that I would expect to be idempotent (unless I've missed something in the docs, as I said I think the working directory of Ansible is not strongly defined and could change).

I don't think it is documented or advertised anywhere. It's still very useful in some edge cases, like testing or using Ansible instead of a complicated shell script. (In fact, I just tested that, you can put #!/usr/bin/ansible-playbook into the first line of a playbook and make it executable, and then just run it directly.)

I wouldn't really advertise this, but I also don't want that this is made not possible (without jumping through additional hoops by prefixing filenames with ./ etc.) by a check which ignores this case.

[MarkusTeufelberger]

An integration test would also be nice to have for this one, sounds like something that might/should be eventually refactored (if someone goes through the hassle of writing a python library that does the stuff ssh-keygen can do).

[felixfontein]

I don't think this part will be affected by a refactoring away from calling ssh-keygen; such a change would probably affect mostly the Keypair class, and maybe the main module logic in main(), but not the path existence check.

[MarkusTeufelberger]

shipit

[felixfontein]

@MarkusTeufelberger @gundalow @Akasurde thanks for your reviews!