Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

More rsync fixes #360

Merged
merged 16 commits into from
Nov 1, 2024
Merged

More rsync fixes #360

merged 16 commits into from
Nov 1, 2024

Conversation

tieneupin
Copy link
Contributor

@tieneupin tieneupin commented Oct 9, 2024

A follow-up to the rsync issue raised in #318 .
Replaced the individual file warnings in murfey.client.rsync with a summary message comparing the files to be transferred and the files registered as successfully transferred. This should help pinpoint whether it's a file path mismatch issue, or another condition further upstream that is not behaving as expected.

Additionally, the MSYS2 environment is set up differently from Cygwin, so Python pathlib.Path defaults to a WindowsPath object instead of a PosixPath as in Cygwin. This PR also introduces additional conditions to ensure file paths are passed to the rsync daemon in the correct Unix format.

@tieneupin tieneupin added the client Relates to the client component label Oct 9, 2024
@tieneupin tieneupin self-assigned this Oct 9, 2024
Copy link

codecov bot commented Oct 9, 2024

Codecov Report

Attention: Patch coverage is 10.71429% with 50 lines in your changes missing coverage. Please review.

Project coverage is 28.19%. Comparing base (ba1c83c) to head (81d8661).
Report is 2 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main     #360      +/-   ##
==========================================
- Coverage   28.26%   28.19%   -0.08%     
==========================================
  Files          71       71              
  Lines        9661     9686      +25     
  Branches     1301     1308       +7     
==========================================
  Hits         2731     2731              
- Misses       6836     6861      +25     
  Partials       94       94              

d-j-hatton
d-j-hatton previously approved these changes Oct 11, 2024
@tieneupin tieneupin dismissed d-j-hatton’s stale review October 29, 2024 14:37

Contents of PR have changed, so will request review again when it's done

@tieneupin tieneupin marked this pull request as draft October 29, 2024 14:38
@tieneupin tieneupin changed the title Replaced individual file warnings in 'rsync' with a summary of the batch More rsync fixes Oct 29, 2024
src/murfey/client/rsync.py Fixed Show fixed Hide fixed
@tieneupin
Copy link
Contributor Author

tieneupin commented Oct 29, 2024

By preprending bash -c to the subprocess command, this resolves the issue.

Test script I wrote to evaluate how to construct the rsync subprocess:
rsync_test.zip

rsync subprocess ran successfully on:

  • MSYS2 terminal
  • MSYS2 run via CMD
  • Cygwin terminal

@tieneupin tieneupin marked this pull request as ready for review October 29, 2024 17:48
@tieneupin
Copy link
Contributor Author

tieneupin commented Nov 1, 2024

This branch was very old, so I encountered some issues when merging from 'main', but it should be all good now.

This PR should be approved and pushed to main together with, but after, PR #388 to fix MSYS2 functionality.

Copy link
Contributor

@stephen-riggs stephen-riggs left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One bug and one query, otherwise ready to merge

src/murfey/client/tui/screens.py Outdated Show resolved Hide resolved
src/murfey/util/__init__.py Show resolved Hide resolved
@tieneupin tieneupin merged commit 3be8e09 into main Nov 1, 2024
17 checks passed
@tieneupin tieneupin deleted the rsync-fix branch November 1, 2024 16:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
client Relates to the client component
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants