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

Let media have a different bucket if desired #1

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

matias-pizarro
Copy link

This gives priority to 'MEDIA_'-prefixed settings keys so we can
define a media-specific bucket if necessary. The default key names
remain the same so this should be a drop-in replacement.

This gives priority to 'MEDIA_'-prefixed settings keys so we can
define a media-specific bucket if necessary. The default key names
remain the same so this should be a drop-in replacement.
@matias-pizarro
Copy link
Author

Hi @anush0247, can you have a look at my pull request please?

@matias-pizarro
Copy link
Author

Upstream is not responding so I will push a fork on pypi (zappa-file-widget-mediabucket), hopefully only temporarily

@matias-pizarro
Copy link
Author

The forked package can be found at https://pypi.python.org/pypi/zappa-file-widget-mediabucket

@anush0247
Copy link
Owner

@rebost did you check my commit review comments?

@matias-pizarro
Copy link
Author

@anush0247 I have looked everywhere I could think of and have not seen your review comments. Could you please provide a link?

s3_host_from_settings = getattr(settings, 'MEDIA_AWS_S3_HOST', 'AWS_S3_HOST')
s3_host = s3_host_from_settings or 's3-ap-southeast-1.amazonaws.com'
default_region = s3_host.split(".amazonaws.com")[0].split("s3-")[1]
aws_storage_bucket_name = getattr(settings, 'MEDIA_AWS_STORAGE_BUCKET_NAME', 'AWS_STORAGE_BUCKET_NAME')
Copy link
Owner

Choose a reason for hiding this comment

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

I appreciate your concept of having separate media bucket name. But consider
AWS_STORAGE_BUCKET_NAME this is going to be the default value if no MEDIA_AWS_STORAGE_BUCKET_NAME defined in the settings.
My Concern is to provide backward compatibility.

Copy link
Author

Choose a reason for hiding this comment

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

Excuse me if I do not understand your comment correctly: the proposed changes are meant to provide full backwards compatibility. Unless settings.MEDIA_xxx is provided the exact same settings.xxx your code uses is used for xxx. One example (the other two context keys behave in the exact same way):

  1. branch master:
    context['AWS_ACCESS_KEY_ID'] == settings.AWS_ACCESS_KEY_ID

  2. branch feature/allow_media_differentiation, with settings.MEDIA_AWS_ACCESS_KEY_ID not defined:
    context['AWS_ACCESS_KEY_ID'] == settings.AWS_ACCESS_KEY_ID

  3. branch feature/allow_media_differentiation, with settings.MEDIA_AWS_ACCESS_KEY_ID defined:
    context['AWS_ACCESS_KEY_ID'] == settings.MEDIA_AWS_ACCESS_KEY_ID

case 2) defaults back to the same values as case 1) so someone upgrading to the new version of zappa-file-widget and not modifying his settings would see no difference.

Copy link
Owner

Choose a reason for hiding this comment

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

ok @rebost

@matias-pizarro
Copy link
Author

matias-pizarro commented Jun 12, 2017

@anush0247 , please see my reply from May 6th

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