-
Notifications
You must be signed in to change notification settings - Fork 330
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
[IN-294] Django-storages assets for providers #8371
[IN-294] Django-storages assets for providers #8371
Conversation
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.
LGTM: did a quick zoom demo and discussed approach. Some concerns about local development, could use some details about that on the PR.
Other comments very, very minor (spacing/indent/naming)
api/base/settings/defaults.py
Outdated
|
||
DEFAULT_FILE_STORAGE = 'storages.backends.gcloud.GoogleCloudStorage' | ||
GS_BUCKET_NAME = os.environ.get('GS_BUCKET_NAME', 'cos-osf-stage-osf-cdn') | ||
# GOOGLE_APPLICATION_CREDENTIALS env var must be set |
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.
Discussed over zoom. Local dev might be interesting or require everyone to hook up staging key, some details written in the PR description might be nice.
|
||
class ProviderAssetFile(BaseModel): | ||
name = models.CharField(max_length=63) | ||
file = models.FileField(upload_to='assets') |
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.
Stupid minor; I personally don't like file
as a name since it technically is a builtin... but we use it all over the place so whatev (https://docs.python.org/2/library/functions.html#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.
I had the same mixed feeling about it, but I think it's only bad practice when assigning locals to builtins, as local assignment will override them but attrs/fields/methods will not. Some examples (in addition to yours) would be that any TypedModel
has a type
field, or that Queryset
objects have filter
and all
methods.
¯\_(ツ)_/¯
{% block content %} | ||
<div class="row"> | ||
<div class="col-md-12"> | ||
{% if perms.osf.delete_asset_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.
might be an extra space after {%
|
||
{% load static %} | ||
{% block top_includes %} | ||
<link rel="stylesheet" type="text/css" href="/static/css/institutions.css" /> |
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.
inconsistent indent
29bcd78
to
6f0ff31
Compare
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.
Looked over code but didn't check out locally. I don't see any blockers.
1542109
to
ee3a50b
Compare
ee3a50b
to
5c6b08b
Compare
Purpose
Implement something better than dog-food'd files committed to a git repo for branded assets.
Changes
django-storages
used for uploading static files to Google Cloud StorageAPI endpoints for assetsRelationship added to providers in the the API (so ember can reference and fetch these by name)(These two to be done in [IN-302])
QA Notes
These assets won't be visible in the ember UI until collections goes out, but test that the admin module can be used to create/edit/delete them.
Side Effects
None expected
Deployment Notes
A
GOOGLE_APPLICATION_CREDENTIALS
environment variable must be set. See lookit's helm chart for referenceTicket
[IN-294]