In T408, it was proposed to have ssh_privkey in the config file and --ssh-privkey as a cmdline argument. That could be used both for --ssh and --libvirt connections. Do you think your currently proposed approach is better?

An idea - will we need to have port configurable as well? Should we change the syntax to user@machine:port (where :port might be optional) right away?

This should be processed immediately after calling parser.parse_args() . I suppose we will have more checks like these soon, so there should be a dedicated function for it, which will accept parser and args as input parameters and will call parser.error() if args are not valid.

I'd leave this up to the code actually using the privkey to raise IOError if it is not there. Only if the code silently ignores the missing file and continues, then we should have an explicit check (a good idea for a functional test).

This should probably be

# TODO: remove this section once T408 is implemented

Please add a docstring.

I'd rather quit with an error if user specifies user@machine:3a5 instead of silently reverting to port 22.

Shouldn't this be called before process_args()? First check whether the provided values are correct, then process them.

It wouldn't silently revert to port 22, it would raise ValueError.

Well, process_args() converts arg from Namespace object to dict, which is more testable (and all our tests provides args in dict). It could be before it, but there has to be explicit var(args).

I noticed only now, but could you please move the whole Runner class upwards before the top-level functions definitions? Right now we have 2 class definitions, then a couple of functions definitions, then another class, then another couple of functions. It makes the file harder to navigate. Let's have all classes at top, and then all the functions.

You're right, I got confused by it, sorry. Which means a comment explaining what you're catching and why would help people like me understanding it. Or we can make the code a bit more obvious:

self.user, machine = self.arg_data['ssh'].split('@')
if ':' in machine:
    self.machine, port = machine.split(':')
    self.port = int(port)
else:
    self.machine = machine
    self.port = 22

Hmm, I'm not even sure why we use vars(args) in process_args(). It's quite usual to work with argparse objects like args.ssh and I don't think there would be any problem in process_args() with it. It seems we don't even want to modify the existing args but return a new, processed set.

When talking about check_args(), would there be any issue with writing the test cases like this?

class Empty: pass
...
class TestRunner:
    def setup(self, monkeypatch):
        ...
        self.obj = Empty()

    def test_ssh_bad_usermachine(self, monkeypatch):
        obj.local = False
        obj.ssh = 'user#machine'

        stub_error = Dingus()
        stub_parser = Dingus()
        stub_parser.error = stub_error
        runner.check_args(stub_parser, obj)

        assert len(stub_error.calls()) == 1

You can of course also use vars(args) in check_args(), but I'm still not sure why we need it.