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

Support for ACL, File Content Type, Cache Max Age, and user Metadata added #171

Closed

Conversation

sumitsharansatsangi
Copy link
Contributor

Currently, The visibility and accessibility of the file was private. But now user can define the visibility and accessibility (default is private).

Currently, No any file content-type was assigned by the piccolo-api, but now it is automatically assigned by the piccolo-api.

Currently, There is no option for defining the file cache age, but now it can be defined.

Currently, there was no support for the user defined metadata for the file, but now it can be defined.

@codecov-commenter
Copy link

codecov-commenter commented Aug 16, 2022

Codecov Report

Merging #171 (6ae548c) into master (ef105f7) will increase coverage by 0.05%.
The diff coverage is 100.00%.

@@            Coverage Diff             @@
##           master     #171      +/-   ##
==========================================
+ Coverage   92.10%   92.16%   +0.05%     
==========================================
  Files          32       33       +1     
  Lines        1774     1787      +13     
==========================================
+ Hits         1634     1647      +13     
  Misses        140      140              
Impacted Files Coverage Δ
piccolo_api/media/base.py 96.38% <ø> (ø)
piccolo_api/media/content_type.py 100.00% <100.00%> (ø)
piccolo_api/media/s3.py 99.01% <100.00%> (+0.13%) ⬆️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@lgtm-com
Copy link

lgtm-com bot commented Aug 17, 2022

This pull request introduces 1 alert when merging 98ffdf5 into ef105f7 - view on LGTM.com

new alerts:

  • 1 for Unused import

@@ -15,12 +17,14 @@ def __init__(self, *routers: Router) -> None:
async def __call__(self, scope: Scope, receive: Receive, send: Send):
for router in self.routers:
try:
asgi = await router(scope, receive=receive, send=send)
response: t.Any | None = await router(
Copy link
Member

Choose a reason for hiding this comment

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

Just a tip. In future, you must be careful to use syntax that will satisfy the tests in Py3.7, Py3.8, P3.9, and Py3.10. I run your branch and everything should be fine, just remove the import typing as t from file because is unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I'll be careful. Done the changes. Hope everything will be good now. Please have a look and merge it.

Copy link
Member

Choose a reason for hiding this comment

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

Great. You have to wait @dantownsend for merging.

@sumitsharansatsangi
Copy link
Contributor Author

@dantownsend , Please merge this.

@dantownsend
Copy link
Member

I think it's almost there.

I left a couple of comments.

My only other concern is before we were assuming the bucket would be private, so all of the URLs are signed. But now it could be a public bucket, so there's no need to sign the URLs. In the future we could add something like a sign_urls parameter.

@dantownsend
Copy link
Member

If you like I'll make the last couple of changes, and will release it.

@sumitsharansatsangi
Copy link
Contributor Author

Please do the change what you feel necessary and recommends and then do release.

@dantownsend dantownsend mentioned this pull request Aug 18, 2022
@dantownsend
Copy link
Member

Merged in #177

@dantownsend dantownsend added this to In progress in Enhancements via automation Aug 18, 2022
@dantownsend dantownsend moved this from In progress to Done in Enhancements Aug 18, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants