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

fix(sync-mode): use the same source path schemas for all action types #5363

Merged
merged 5 commits into from Nov 14, 2023

Conversation

vvagaytsev
Copy link
Collaborator

@vvagaytsev vvagaytsev commented Nov 9, 2023

What this PR does / why we need it:
This unifies the way we validate the sync mode's source paths for all action types.

Which issue(s) this PR fixes:

Fixes #4982
Fixes #5031

Special notes for your reviewer:

I tested and verified the changes on the Windows machine. I tried both relative and absolute Windows paths as a sync-mode source path.

I also added 2 test cases for Windows in 40ec1ea.
Verified the failure of those tests manually and in CI.

See individual commits for details.

@vvagaytsev vvagaytsev changed the title fix(sync-mode): use the same sourcePath schemas for all action types fix(sync-mode): use the same source path schemas for all action types Nov 9, 2023
@vvagaytsev vvagaytsev changed the title fix(sync-mode): use the same source path schemas for all action types fix(sync-mode): use the same sourcePath schemas for all action types Nov 9, 2023
@vvagaytsev vvagaytsev changed the title fix(sync-mode): use the same sourcePath schemas for all action types fix(sync-mode): use the same source path schemas for all action types Nov 9, 2023
@vvagaytsev vvagaytsev force-pushed the fix/sync-mode-source-apth-windows branch 6 times, most recently from a6c4ced to 40ec1ea Compare November 10, 2023 13:22
@vvagaytsev vvagaytsev requested review from stefreak and a team November 10, 2023 13:54
@vvagaytsev vvagaytsev marked this pull request as ready for review November 10, 2023 13:54
.description(
deline`
POSIX-style or Windows-style local path of the directory to sync to the target.
Can be either absolute or relative to the source directory where the Deploy action is defined.
Copy link
Member

@stefreak stefreak Nov 10, 2023

Choose a reason for hiding this comment

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

Will a Windows-style relative path automatically be transformed to a Unix-path when you use the action on Linux or MacOS?

Copy link
Member

Choose a reason for hiding this comment

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

If not both styles of writing the path work on all platforms, we should note that here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good catch, thanks! I'll check if there are any advanced options to ensure that conversion. I hope it's doable.
Otherwise, we can implement it and apply it in all similar places.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It looks like there is no way to do automatic conversion with joi. I've rewritten the docs in 59ca3ff. Now it says that the path should be in a platform-specific format.

Copy link
Member

@stefreak stefreak Nov 13, 2023

Choose a reason for hiding this comment

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

I'm not sure if platform-specific is a good way to express this when Unix-paths will work under Windows.

Please correct me if I'm wrong, but I think the following statements are true:

  1. I am assuming here that windows-style paths won't work with Mutagen under Linux (Have we tested that though? If this assumption is false we are over-worrying here)
  2. The Unix-style paths with a forward slash (/) will work on Windows computers (I guess? Otherwise you can't use sync mode with coworkers on other platforms?)
  3. Windows-style paths (with a backslash) won't be portable to Unix-like-systems like Linux or MacOS (Unless mutagen somehow will convert the paths?)
  4. Unix-style absolute paths won't be portable to Windows systems, and Windows-like absolute paths with a drive letter won't be portable to Unix-systems, obviously

My suggestion:

        Path to a local local directory to be synchronized with the target.
        
        Can be either absolute or relative to the source directory where the Deploy action is defined.
        
        We recommend sticking with relative paths here, and using forward slashes (`/`) as a delimiter, as Windows-style paths with back slashes (`\`) and absolute paths will work on some platforms, but they are not portable and will not work for users on other platforms.

I have not tested the portability of using absolute paths and backslashes – have you?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

@stefreak right. Windows paths are not portable to Linux and Unix-like systems. Absolute POSIX paths are also not portable to Windows because of the drive letter format. Those can be used under WSL.

Relative POSIX-paths work well on Windows too. So that's the recommended setup. Thanks for the suggestion.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Updated it in 13fd82c.

@vvagaytsev vvagaytsev marked this pull request as draft November 10, 2023 18:53
auto-merge was automatically disabled November 10, 2023 18:53

Pull request was converted to draft

@vvagaytsev vvagaytsev force-pushed the fix/sync-mode-source-apth-windows branch from 40ec1ea to c7c016a Compare November 13, 2023 11:17
@vvagaytsev vvagaytsev marked this pull request as ready for review November 13, 2023 12:08
@vvagaytsev vvagaytsev force-pushed the fix/sync-mode-source-apth-windows branch from 59ca3ff to 13fd82c Compare November 14, 2023 09:56
@vvagaytsev vvagaytsev added this pull request to the merge queue Nov 14, 2023
Merged via the queue into main with commit 71b3781 Nov 14, 2023
47 checks passed
@vvagaytsev vvagaytsev deleted the fix/sync-mode-source-apth-windows branch November 14, 2023 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
3 participants