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 permissions of files created with dump_to_path #164

Merged
merged 1 commit into from
Aug 23, 2021

Conversation

OriHoch
Copy link
Contributor

@OriHoch OriHoch commented Aug 23, 2021

Reproduction Steps

  • Create a file using standard Python: open("test.txt", "w").write("Hello World!")
  • Run a flow with dump_to_path: DF.Flow(({'a': 1},{'a': 2}), DF.dump_to_path('test')).process()

Expected

  • Default OS file permissions (from umask):
$ ls -l test.txt
-rw-rw-r-- 1 ori ori 12 Aug 23 19:03 test.txt
$ ls -l test/
-rw-rw-r-- 1 ori ori 816 Aug 23 19:03 datapackage.json
-rw-rw-r-- 1 ori ori   9 Aug 23 19:03 res_1.csv

Actual

  • dump_to_path files have permissions to owner only:
$ ls -l test/
-rw------- 1 ori ori 816 Aug 23 19:06 datapackage.json
-rw------- 1 ori ori   9 Aug 23 19:06 res_1.csv

Explanation

Files created with FileDumper are first created as temporary files which have permissions only for file owner, and are are later copied over to the final location. They are copied with the limited temporary file permissions which is unexpected and limited. This PR fixes it by setting their permissions to the default OS permissions as set by umask.

See more details about this problem and the solution here: https://stackoverflow.com/questions/7150826/how-can-i-get-the-default-file-permissions-in-python

@coveralls
Copy link

coveralls commented Aug 23, 2021

Pull Request Test Coverage Report for Build 552

  • 8 of 8 (100.0%) changed or added relevant lines in 1 file are covered.
  • No unchanged relevant lines lost coverage.
  • Overall coverage increased (+0.04%) to 84.895%

Totals Coverage Status
Change from base Build 551: 0.04%
Covered Lines: 2175
Relevant Lines: 2562

💛 - Coveralls

Copy link
Member

@akariv akariv left a comment

Choose a reason for hiding this comment

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

This is awesome @OriHoch , thanks!
I've bumped my head on this problem some time ago but didn't get to any conclusion.

@akariv akariv merged commit 9e77054 into datahq:master Aug 23, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants