AWS settings for XBlocks to have filesystem-like storage - #5120
Conversation
|
FYI: Upgrade to django-pyfilesystem is trivial. Adds support for passing auth through settings (rather than environment variables), and fixes a bug with one-time URLs. |
|
@stephensanchez @swdanielli I know both of you are traveling. On the off-chance either of you happen to be on-line, if either could give a quick thumbs-up, it would speed things up on projects you both have an interest in. |
There was a problem hiding this comment.
if 'AWS_ACCESS_KEY_ID' in AUTH_TOKENS?
There was a problem hiding this comment.
In fact, given the changes in djpyfs, why not just always include the aws_access_key_id and aws_secret_access_key, possibly as None?
There was a problem hiding this comment.
Regarding #1: On sandbox machines, it is there, and it is set to None. This is the same as not being there.
Regarding #2: I could do that if you prefer. As a matter of style, I prefer to omit irrelevant settings. They add clutter everywhere (inspecting variables when debugging, longer config files, etc.). In this particular case, the lines "AWS_ACCESS_KEY_ID": null get replicated among many config files (this is on sandbox, vagrant, etc.).
Ideally, lines 213 and 214 would check command-line overrides, auth.json, environment variables, as well as possible some kind of platform-level defaults, so all settings at this level would be populated.
There was a problem hiding this comment.
Not sure what you mean by longer config files. It would make this settings file shorter (removing the if clause), and since it should use AUTH_TOKENS.get('AWS_ACCESS_KEY_ID') (note that None is the default default value for .get). It wouldn't force you to add values to auth_tokens.json, because it's still providing a default.
There was a problem hiding this comment.
"it is there, and it is set to None. This is the same as not being there": Is that actually true? djpyfs treats both conditions the same (because it uses None as the default value if the setting is missing.)
There was a problem hiding this comment.
It's a question of contract. The contract, as read by me, is "pass a string to set, don't pass a parameter if you want system defaults. System defaults currently come from environment variables. In the future, we might add ~/.boto or ~/.aws or similar."
The API happens to default to aws_access_key_id=None in the function call to detect if it was set.
Meh. Not important. Changed.
While we're at it, I also changed the default prefix to /xblock-storage/
b1409e7 to
eeb139a
Compare
Upgrades to latest pyfs to support auth not through environment variables
eeb139a to
31176ae
Compare
|
Also, added a tiny CSS change which prevented file upload link from properly showing up. pmitros/ProfileXBlock@1ede634...4aeaa24 (Since it looks like we're about to release) |
|
👍 |
1 similar comment
|
👍 |
AWS settings for XBlocks to have filesystem-like storage
There was a problem hiding this comment.
Not that it matters but just curious why lines 223 and 224 add keys to the dict rather than have these declared below line 221.
There was a problem hiding this comment.
Blah. Code evolution. It'd be clear from the git history, if we didn't rewrite. Should be in the dictionary. It got merged a while back, and not worth cleanup now.
There was a problem hiding this comment.
It would be nice to put in a small, obvious PR w/ the cleanup, just to
improve the hygene of the repo.
On Wed, Sep 10, 2014 at 12:02 PM, Piotr Mitros notifications@github.com
wrote:
In cms/envs/aws.py:
@@ -213,6 +213,16 @@
with open(CONFIG_ROOT / CONFIG_PREFIX + "auth.json") as auth_file:
AUTH_TOKENS = json.load(auth_file)+############### XBlock filesystem field config ##########
+if 'XBLOCK-FS-STORAGE-BUCKET' in ENV_TOKENS:
- DJFS = {
'type' : 's3fs','bucket' : ENV_TOKENS.get('XBLOCK-FS-STORAGE-BUCKET', None),'prefix' : ENV_TOKENS.get('XBLOCK-FS-STORAGE-PREFIX', '/xblock-storage/')- }
- DJFS['aws_access_key_id'] = AUTH_TOKENS.get('AWS_ACCESS_KEY_ID', None)
Blah. Code evolution. It'd be clear from the git history, if we didn't
rewrite. Should be in the dictionary. It got merged a while back, and not
worth cleanup now.—
Reply to this email directly or view it on GitHub
https://github.com/edx/edx-platform/pull/5120/files#r17370250.
There was a problem hiding this comment.
Roger willco.
On backlog. However, I'll wait for dev-ops to get the settings out on sandboxes+prod. Right now, testing is a bit of a pain.
|
👍 aside from one small question this seems reasonable to me. |
4 participants
Adds support for S3FS for deployment infrastructure. Tested on sandbox, and works. Waiting for tests. @cpennington @jarv