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

Allow passing arbitrary extra fields when saving a file #15

Closed
wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Nov 9, 2022

This is to support adding extra parameters when uploading files.

The specific use case for this is uploading to S3 with a canned acl.

@ghost
Copy link
Author

ghost commented Nov 9, 2022

Thanks for triggering the run, I'll fix that and see if I can get the checks to pass locally before pushing again.

@jowilf
Copy link
Owner

jowilf commented Nov 10, 2022

Hello @jefflieb-wavy,
Hope you are doing well.
Thank you for your contribution,
I'm little bit busy right now, but i'll review your code once i have some free time,
Please add an simple example of how it can be used.

Thank you

@jowilf jowilf closed this Nov 10, 2022
@jowilf jowilf reopened this Nov 10, 2022
@codecov
Copy link

codecov bot commented Nov 10, 2022

Codecov Report

Base: 100.00% // Head: 100.00% // No change to project coverage 👍

Coverage data is based on head (849204d) compared to base (5ee62a9).
Patch coverage: 100.00% of modified lines in pull request are covered.

Additional details and impacted files
@@            Coverage Diff            @@
##              main       #15   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files           24        24           
  Lines         1755      1756    +1     
=========================================
+ Hits          1755      1756    +1     
Impacted Files Coverage Δ
sqlalchemy_file/file.py 100.00% <100.00%> (ø)
sqlalchemy_file/storage.py 100.00% <100.00%> (ø)

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

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@rafnixg
Copy link

rafnixg commented Nov 25, 2022

Hello, When will this merge be available, or is there another way to add ACLs?

Copy link
Owner

@jowilf jowilf left a comment

Choose a reason for hiding this comment

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

Thank you for time and your contribution but I think we still need to override the default File class to use your solution, I'm sure we can make it easy

My proposition

  • Accept extra in field declaration as default for all associated files
class Attachment(Base):
    __tablename__ = "attachment"

    id = Column(Integer, autoincrement=True, primary_key=True)
    name = Column(String(50), unique=True)
    content = Column(FileField(extra={"acl": ..., "meta_data": ...}))
  • Accept extra in File to customize for each file
file = File(content="Hello World", filename="hello.txt", content_type="text/plain", extra={"acl": ..., "meta_data": ...})
  • Deprecate metadata usage (merge with extra['meta_data'])

wdyt ?

@rafnixg
Copy link

rafnixg commented Nov 26, 2022

Hello @jowilf this implantation seems good to me, the configuration from the fields seems clear and flexible.
Thanks for this package.

@jowilf
Copy link
Owner

jowilf commented Nov 26, 2022

Thank you all for your contribution
I'll close this PR now. We can continue here -> #18
Please @jefflieb-wavy, to avoid this in future start with a discussion or an issue before PR. Thank you

@jowilf jowilf closed this Nov 26, 2022
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.

2 participants