Fix rsync operations for source directories containing a relative pat... - #1353

caller() was removed. get_file_path() calls caller() it if there is no second argument. See https://github.com/RexOps/Rex/blob/master/lib/Rex/Helper/Path.pm#L60

caller() was removed. get_file_path() calls caller() it if there is no second argument. See https://github.com/RexOps/Rex/blob/master/lib/Rex/Helper/Path.pm#L60

Yep, I just updated my comment once I realized I read the diff backwards :) Thanks for the explanation!

Oh, one more thing I noticed. So with this patch, the full path to the dir is now found. In other words, if the source dir is some_dir/*, the source dir will become absolute with something like /home/me/some_dir/*.

However, if you don't have a wildcard character and just have some_dir, the source dir is not fully qualified, even without this patch.

I think, ideally, directories without a wildcard should get fully qualified as well. To achieve, this, you can comment out this line: https://github.com/RexOps/Rex/blob/master/lib/Rex/Helper/Path.pm#L108. With that line commented out, the get_file_path will then return a full qualified path to the file (or dir).

But things still seem to work without the source dir being fully qualified so maybe it's better not to mess with it.

At any rate, it feels like whoever wrote Rsync to use get_file_path used it as a quick hack. Someone should probably take a really close look at how Rsync resolves relative paths.

I don't think we can fully untangle the various rsync questions without having tests that describe the expected behavior. I started write some tests locally (and so far, I had to add support for local rsync runs to be able to test).

I also closed #1106 as duplicate of #1075. This should take care of cross-linking the related issues/PRs.

I think, ideally, directories without a wildcard should get fully qualified as well.

That might work, yes.

To achieve, this, you can comment out this line: https://github.com/RexOps/Rex/blob/master/lib/Rex/Helper/Path.pm#L108. With that line commented out, the get_file_path will then return a full qualified path to the file (or dir).

Without reading the code, it feels a bit risky to change the path resolution behavior for all consumers both in the core and in every other existing code outside of it. Perhaps some kind of "please give me the absolute path flag" has less disrupting potential.

But things still seem to work without the source dir being fully qualified so maybe it's better not to mess with it.

Yes, ideally it would be preferred to address the bug directly, instead of redesigning the overall behavior.

At any rate, it feels like whoever wrote Rsync to use get_file_path used it as a quick hack. Someone should probably take a really close look at how Rsync resolves relative paths.

I'd say the motivation was more like "let the Rsync module use the exact same path resolution logic like any other function" (something like look for candidates in the current dir, the module path, or in any of path_map locations, also look for files with extension matching the environment name (prod, dev, etc.), and so on). In that sense, it would be quite desirable to keep the behavior the same.

Right, it definitely needs tests. There should be one set of tests just to determine if it is generating the desired command. That way, you don't need to have rsync installed to actually run the tests.

To test the rest of the command, you'd have to set up a fake server that sent back different response strings. I don't think that's really necessary for now as that doesn't seem to be broken.

I would like to figure out how to handle running rsync sudo per #1298. There is also #1315

Also, the rsync command automatically inserts the -d and -l options. I haven't filed a bug report on this but it really probably shouldn't be doing that. Or if it is, that should at least be documented.

Overall, I think the modules needs a little mini overhaul because it has some fairly big problems.

Right, it definitely needs tests. There should be one set of tests just to determine if it is generating the desired command.
That way, you don't need to have rsync installed to actually run the tests.

Could be done, though so far I'm more interested in testing the behavior ("does it actually sync the files as expected"), instead of "can it generate an expected rsync command". That way, I expect many of the tests would be easier to reuse for Rex::Commands::Sync as well.

To test the rest of the command, you'd have to set up a fake server that sent back different response strings. I don't think that's really necessary for now as that doesn't seem to be broken.

I feel mocking an overkill, while it's definitely possible. Unless there's an existing solution, I'm not sure it's worth to invent and maintain our own rsync mocking server inside rex core.

Currently, I chose to add a t/sync dir with a bunch of files and a nested dir under it, and ask the test to sync it to a temp directory, then compare the contents of source and the target.

I would like to figure out how to handle running rsync sudo per #1298. There is also #1315

Yep, they are rsync related, but offtopic here.

Also, the rsync command automatically inserts the -d and -l options. I haven't filed a bug report on this but it really probably shouldn't be doing that. Or if it is, that should at least be documented.

Yeah, not sure if bug or feature request, but definitely a different issue :)

Overall, I think the modules needs a little mini overhaul because it has some fairly big problems.

Rsync is also the only one module that is not running on the managed host. In the long run, it might worth cutting the whole module out into a separate distribution.

With all that, we drifted quite far away of the original topic of this PR though and I'd strongly prefer not mixing too many topics in a single thread.

How about opening an feature request about writing the (r)sync tests first, and only after it's there, then figure out what's broken and how to fix?

As commented on #1075, the underlying issue is something else, and I don't consider removing caller() as the proper fix for those.

Tests are also a different topic.