Add rsync client for remote copy operations

[pevogam]

This commit adds support for a new rsync client for remote copy operations which is especially important now that the SCP protocol is being deprecated.

[ldoktor]

All of this is common to all three methods. How about splitting it into common part and then remote definition and share the common code that way?

I mean something like:

def rsync_from_remote(...):
    command = _get_rsync_cmd(...)
    command += _get_rsync_host(...)
    command += "{shlex.quote(local_path)}"

def rsync_between_remotes(...):
    command = _get_rsync_cmd(...)
    command += _get_rsync_host(...)
    command += _get_rsync_host(...)

note it's not a strong request, I just think the less code one writes the easier is it to maintain it....

[pevogam]

In the future we might end up dropping scp entirely and just leaving rsync rendering such refactoring futile at present so I would keep the changes as additive as possible right now. Let's see after some more use of the remote module if it would make more sense to rather simplify it and maximize reusability.

[ldoktor]

that seems reasonable

[ldoktor]

This is user-facing change as people might be relying on those. How about adding the generic exceptions and inherit the SCP ones out of them to keep compatibility for scp group of commands?

[pevogam]

I tried this first (separate rsync and scp hierarchies derived from the same base exceptions) but a lot of the refactored code which is nearly identical would have to be split into multiple versions for the only reason of raising a different exception type without much additional meaning (e.g. structure of the exception). So I ended up opting for using a simpler set of exceptions just like the "copy" instead of "SCP or rsync" in the above logging message.

[pevogam]

So the point rather in the rafactoring and not as much in breaking changes. As you correctly observe above though, we could also break but improve the argument order regarding the directory (recursive copy) value, so let's do that too to at least make all of this somewhat nicer.

[ldoktor]

Hello Plamen, I provided some suggestions but there is no real blocker. The renamed exceptions might bring some difficulties to users, but I'm not sure how many rely on this so I leave it on your consideration. The same with the argument order as even the scp methods exhaust all the system arguments before the directory one so it is consistent as is (while not that user-friendly :D).

All in all, let me know what do you think about the exceptions and if you say it's fine I'm ready to merge it as is. But don't say nobody warned you then ;-)

[pevogam]

Hi @ldoktor let's see how many people will rather thell me they warned me about changing the order of the directory argument which I did in a separate commit (as it is not directly related to the rsync extention). Other than that I hope I have addressed most if not all of your comments.

[ldoktor]

>>> remote.rsync_between_remotes("127.0.0.1", "localhost", 22, "", "", "medic", "medic", "/tmp/a", "/tmp/b")
Traceback (most recent call last):
  File "/home/medic/Work/Projekty/avocado/aexpect/aexpect/remote.py", line 520, in _remote_copy
    match, text = session.read_until_last_line_matches(
                  ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/medic/Work/Projekty/avocado/aexpect/aexpect/client.py", line 1002, in read_until_last_line_matches
    return self.read_until_output_matches(patterns,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/medic/Work/Projekty/avocado/aexpect/aexpect/client.py", line 940, in read_until_output_matches
    raise ExpectProcessTerminatedError(patterns, self.get_status(),
aexpect.exceptions.ExpectProcessTerminatedError: Process terminated while looking for patterns ['[Aa]re you sure', '[Pp]assword:\\s*$', 'lost connection']    (status: 1,    output: 'The source and destination cannot both be remote.\nrsync error: syntax or usage error (code 1) at main.c(1415) [Receiver=3.4.1]\n')

The above exception was the direct cause of the following exception:

Traceback (most recent call last):
  File "<stdin>", line 1, in <module>
  File "/home/medic/Work/Projekty/avocado/aexpect/aexpect/remote.py", line 857, in rsync_between_remotes
    return remote_copy(command, password_list,
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/home/medic/Work/Projekty/avocado/aexpect/aexpect/remote.py", line 586, in remote_copy
    _remote_copy(session, password_list, transfer_timeout, login_timeout, method)
  File "/home/medic/Work/Projekty/avocado/aexpect/aexpect/remote.py", line 555, in _remote_copy
    raise TransferFailedError(error.status, error.output) from error
aexpect.remote.TransferFailedError: SCP transfer failed    (status: 1,    output: 'The source and destination cannot both be remote.\nrsync error: syntax or usage error (code 1) at main.c(1415) [Receiver=3.4.1]\n')

So it looks like you'll have to deal with this by either relying on scp or ssh to one of those and initiate the rsync from there (which might not cover the scenario where remotes don't have direct connection, then you'd have to tunel it through...)

[pevogam]

Ah interesting, in fact I have never used any copying between remotes and simply assumed rsync supports this as much as scp. If you want we can simply remove the support for this functionality then. The scp between remotes will still remain of course. What do you think?

[ldoktor]

makes sense to me. Better to be explicit than trying to hack something that might or might not work most of the time...