Skip to content
This repository has been archived by the owner on Aug 22, 2022. It is now read-only.

Configure PROFILE_IMAGE_BACKEND when using AWS s3 #370

Merged
merged 6 commits into from Dec 19, 2018

Conversation

GustavoKatel
Copy link
Contributor

@GustavoKatel GustavoKatel commented Dec 5, 2018

See SE-559

Turns out that to upload profile images to aws, the profile images backend has to be configured similar to the default file storage. This PR adds the ansible variable to the configuration_storage_settings entry in OCIM.

Ref: https://github.com/edx/configuration/blob/master/playbooks/roles/edxapp/defaults/main.yml#L688

Sandbox URL:
Sandbox with the current change in the extra settings: https://gustavo-profile-image-test.stage.opencraft.hosting/

Testing instructions:

  1. Set up the Ocim devstack with this branch.
  2. Deploy an instance with default configurations and using s3.
  3. Create a new AppServer and check on its configurations if EDXAPP_PROFILE_IMAGE_BACKEND is set and matches:
EDXAPP_PROFILE_IMAGE_BACKEND:
  class: storages.backends.s3boto.S3BotoStorage
  options:
    headers:
      Cache-Control: max-age-"{{ EDXAPP_PROFILE_IMAGE_MAX_AGE }}"
    location: '{{ EDXAPP_AWS_LOCATION }}/profile-images'
  1. After the provision, set a profile picture for one of its user and check if the url points to aws s3.

Reviewers

@@ -48,6 +48,13 @@ def swift_container_names(self):
"""
return [self.swift_container_name]

@property
def s3_custom_domain(self):
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you move this to /mixins/storage.py#S3BucketInstanceMixin? There are a few other related properties there.

@@ -61,13 +68,29 @@ def _get_s3_settings(self):
# using shared s3 buckets
"EDXAPP_AWS_LOCATION": self.swift_container_name,
"EDXAPP_DEFAULT_FILE_STORAGE": 'storages.backends.s3boto.S3BotoStorage',

# Set up S3 image backend
# https://github.com/edx/configuration/blob/master/playbooks/roles/edxapp/defaults/main.yml#L688
Copy link
Contributor

Choose a reason for hiding this comment

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

This reference will get out of date so we can leave it out. :)

"location": '{}/{}'.format(self.swift_container_name, 'profile-images'),
"custom_domain": self.s3_custom_domain,
"access_key": self.s3_access_key,
"secret_key": self.s3_secret_access_key,
Copy link
Contributor

Choose a reason for hiding this comment

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

Hmm. Since django-storages defaults to the global ones can we get away with not passing the ones which are already set? I think only location and headers are unique here.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

You're right! Thanks for the tip :)

from instance.tests.base import TestCase
from instance.tests.models.factories.openedx_appserver import make_test_appserver
from instance.tests.models.factories.openedx_instance import OpenEdXInstanceFactory
from instance.tests.models.factories.openedx_appserver import \
Copy link
Contributor

Choose a reason for hiding this comment

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

edX uses 120 as line length so you may want to set that in your IDE and undo these formatting changes.

Signed-off-by: Gustavo Sampaio <gbritosampaio@gmail.com>
@GustavoKatel GustavoKatel merged commit fdb80a6 into master Dec 19, 2018
@GustavoKatel GustavoKatel deleted the gustavokatel/profile-image-s3-backend branch December 19, 2018 16:50
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants