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

Adds option for lazy connection to swift #97

Merged
merged 2 commits into from
Feb 15, 2018

Conversation

pomegranited
Copy link
Contributor

@pomegranited pomegranited commented Feb 14, 2018

Instead of connecting to SWIFT on storage init, this adds an option to allow the SWIFT connection to be created only when required.

Django takes great care to initialise Storage classes lazily, and so connecting on creation is generally a valid option. However there are some cases where this is difficult to do, e.g., when using FileField.storage, the Storage instance is instantiated on model import time, which can result in unintended SWIFT authentication/connections.

Reviewer

@codecov-io
Copy link

codecov-io commented Feb 14, 2018

Codecov Report

Merging #97 into master will increase coverage by 0.12%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master      #97      +/-   ##
==========================================
+ Coverage   97.77%   97.89%   +0.12%     
==========================================
  Files           1        1              
  Lines         225      238      +13     
==========================================
+ Hits          220      233      +13     
  Misses          5        5
Impacted Files Coverage Δ
swift/storage.py 97.89% <100%> (+0.12%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d6b52cf...83b3ba4. Read the comment docs.

@smarnach
Copy link

👍 Looking good to me. I've read through the code and tested that it still works for our use case (an Open edX instance).

I'm wondering whether there is any disadvantage to connecting lazily, or whether lazy connections should be the default, but it's probably safer to keep the current behaviour.

@pomegranited
Copy link
Contributor Author

Hi @dennisv ! Would you consider merging this PR? Please let me know if you need more information or tests added.

@dennisv dennisv merged commit dd9dc2f into dennisv:master Feb 15, 2018
@pomegranited pomegranited deleted the lazy_connect branch February 15, 2018 09:43
@pomegranited
Copy link
Contributor Author

Thank you @dennisv !

@dennisv
Copy link
Owner

dennisv commented Feb 15, 2018

It's also pushed to PyPI:
https://pypi.python.org/pypi/django-storage-swift/1.2.18

@pomegranited
Copy link
Contributor Author

So quick -- awesome!

pomegranited added a commit to open-craft/edx-platform that referenced this pull request Feb 15, 2018
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.

4 participants