Skip to content

utils/fs: checking files ownership in 'move' (#4348) #4832

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 5 commits into from
Nov 12, 2020
Merged

utils/fs: checking files ownership in 'move' (#4348) #4832

merged 5 commits into from
Nov 12, 2020

Conversation

@danielfleischer
Copy link

@danielfleischer danielfleischer commented Nov 3, 2020 •
edited
Loading

Files are checked for ownership by trying to chmod them before moving them around; if fail, return a verbose exception.
Previous behavior: moving files to a temp folder and then do the check; when fail, files seemed to be missing.

Should fix #4348 and maybe fix #2992.

Thank you for the contribution - we'll try to review it as soon as possible. 🙏

pared
pared reviewed Nov 4, 2020
View reviewed changes
pared
pared approved these changes Nov 6, 2020
View reviewed changes
Copy link
Contributor

@pared pared left a comment

Choose a reason for hiding this comment

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

Looks great! One minor thing

pared
pared suggested changes Nov 6, 2020
View reviewed changes
@danielf-amobee
Specific error handling
5579767 
If error related to access/permission, throw the newly created
FileOwnershipError exception, otherwise throw the OSError as is.
@pared
Copy link
Contributor

pared commented Nov 9, 2020

@danielfleischer looks good, the tests test_add_symlink_file seems to be failing on windows due to this change.

@danielf-amobee
Using safer 'unlink'
314f49e 
Using the function _unlink which includes error handling.
@danielfleischer
Copy link
Author

Made a small fix; used _unlink instead, it's safer. Now all Windows tests pass.

@pared pared requested review from efiop, pmrowla and skshetry November 11, 2020 13:03
@pared pared merged commit 8e73826 into iterative:master Nov 12, 2020
@pared
Copy link
Contributor

pared commented Nov 12, 2020

@danielfleischer Thank you very much!

This was referenced Nov 12, 2020
Closed
Closed
@efiop efiop added the enhancement Enhances DVC label Nov 18, 2020
@efiop efiop mentioned this pull request Jan 13, 2021
Closed
efiop pushed a commit that referenced this pull request Jan 24, 2021
Revert "utils/fs: checking files ownership in 'move' (#4348) (#4832)"
475a146 
Fixes #5255

We need a proper solution for #4832, will introduce later.

This reverts commit 8e73826.
@efiop efiop mentioned this pull request Jan 24, 2021
Merged
2 tasks
efiop added a commit that referenced this pull request Jan 24, 2021
@efiop
Revert "utils/fs: checking files ownership in 'move' (#4348) (#4832)" (…
c36ae73 
…#5321)

Fixes #5255

We need a proper solution for #4832, will introduce later.

This reverts commit 8e73826.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@skshetry skshetry skshetry approved these changes

@pmrowla pmrowla Awaiting requested review from pmrowla

@efiop efiop Awaiting requested review from efiop

+1 more reviewer

@pared pared pared approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

enhancement Enhances DVC

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Parquet file owned by other user gets deleted upon dvc add better message when trying to link from files that belong to another user

5 participants

@danielfleischer @pared @skshetry @efiop @danielf-amobee