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

Cloud media files #141

Merged
merged 36 commits into from
Aug 19, 2022
Merged

Cloud media files #141

merged 36 commits into from
Aug 19, 2022

Conversation

dandavies99
Copy link
Contributor

This PR contains most of the changes to move to using Azure storage to serve media files. Overall it should now be possible to download raw data files that have been uploaded by users (and any other media files).

  • There are new settings to configure the django-storage[azure] backend
  • There are functions to generate sas tokens that are valid for 1 hour and for only the blob (file) in question
  • There is a view which uses the sas-generating function, this requires the user to have permission to view that ExperimentDataFile object.
  • Existing tests have been extended to check the above works for the biologic and maccor examples.

The parsing procedure is now a little different: The file is copied to a temporary local folder, all the parsing routines are carried out, then that temporary file is destroyed. This could be handled better (see #139) but I would prefer to tackle that separately: There are lots of places where "file_path" is used to read the file and it's already started to get a little messy. See file-obj-parsers branch where I've made a start.

Overall, I think we should check these changes solve the problem in #61 for the VM before moving to updating the parsing engines.

@dandavies99 dandavies99 changed the title Cloud static files Cloud media files Aug 4, 2022
@dandavies99 dandavies99 requested a review from dalonsoa August 4, 2022 13:24
Copy link
Collaborator

@dalonsoa dalonsoa left a comment

Choose a reason for hiding this comment

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

It looks good. Using directly azure tools for Django seems pretty useful. Have left a few questions/comments, so I will not approve - or request changes - for now.

from azure.storage.blob import BlobServiceClient, generate_blob_sas
from storages.backends.azure_storage import AzureStorage

from liionsden.settings import settings
Copy link
Collaborator

Choose a reason for hiding this comment

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

These settings are the standard settings, not the production settings which are in production.py. I'm not sure if it matters, but just in case I better mention it.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeh one question I had, which I forgot to mention, was whether we want to only use the azure storage in the production environment. In principle it sounds sensible, but I'm not sure if it is as simple as just changing settings.py until we sort out the parsing engines - they currently kind of rely on the blob storage being used to retrieve the file.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it makes sense to serve files from Azure only when Liionsden is running in Azure - or possibly the VM. But for local use and development purposes, I don't think we should enforce the need to Azure. Yes, the parsers will need updating.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well it will have to be definitely when running on the VM (as we are still using as a development version) because the original issue this solves is that we can't serve media files from there currently.

I agree about local development though so I suggest that we test the implementation in this PR on the VM (once I've addressed the other points) first. Then once we know it fixes the original issue, we can sort out the parsers and make it a production (i.e. VM or Azure) only setting.

@dandavies99 dandavies99 requested a review from dalonsoa August 9, 2022 10:45
@dandavies99
Copy link
Contributor Author

Hi @dalonsoa - I think I've addressed all the points other than setting this as a production-only setting. As I said, I'd rather do this as a separate step to test that this approach definitely works on the VM. After that, the next PR will be refactoring parsers and allowing the storage backend to be selected based on the environment. Happy to hear any other suggestions though!

Copy link
Collaborator

@dalonsoa dalonsoa left a comment

Choose a reason for hiding this comment

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

All looks good to me. Let's see if it works well, too!

@dandavies99 dandavies99 merged commit ffd8a90 into develop Aug 19, 2022
@dandavies99 dandavies99 deleted the cloud-static-files branch August 25, 2022 08:56
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.

3 participants