-
Notifications
You must be signed in to change notification settings - Fork 1
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/75 no mkdir when s3 #76
Conversation
…unnecessary and not permitted.
…unnecessary and not permitted.
Coverage reportClick to see where and how coverage changed
This report was generated by python-coverage-comment-action |
|
||
log = logging.getLogger(__name__) | ||
|
||
|
||
async def upload_to_storage(lei: str, submission_id: str, content: bytes, extension: str = "csv"): | ||
try: | ||
fs: AbstractFileSystem = filesystem(settings.upload_fs_protocol.value) | ||
fs.mkdirs(f"{settings.upload_fs_root}/{lei}", exist_ok=True) | ||
if settings.upload_fs_protocol == FsProtocol.FILE: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It looks like the LocalFileSystem impl takes a auto_mkdir boolean which would create the directory on open if it doesn't exist, that could be passed in kwargs to open I think. If so, this might be a good way to avoid this impl specific if (which loses the abstractness which is really nice to have!). The kwarg would get ignored by S3. I'm not positive, just reading the API, but might be worth a look?
If that doesn't work, this looks good. Just was hoping to keep it all abstract but alas!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
really good suggestion, alas it doesn't work... I get an unexpected keyword argument, so unfortunately the s3fs doesn't ignore it...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
BOOO. Oh well
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good
closes #75