Closed
Bug 1245621
Opened 8 years ago
Closed 5 years ago
[PulseGuardian] Flask-SSLify should not be required if running locally
Categories
(Webtools :: Pulse, defect, P4)
Webtools
Pulse
Tracking
(firefox47 affected)
RESOLVED
WONTFIX
Tracking | Status | |
---|---|---|
firefox47 | --- | affected |
People
(Reporter: mcote, Assigned: rutuja.r.surve, Mentored)
Details
(Whiteboard: try: from flask_sslify import SSLify except ImportError: #log.debug('flask-sslify failed to import', exc_info=True) logging.warning('flask-sslify failed to import'))
User Story
This is a mentored Pulse bug. For general information on Pulse, see https://wiki.mozilla.org/Auto-tools/Projects/Pulse, which includes a section on Contributing.
Attachments
(1 file, 4 obsolete files)
1.37 KB,
patch
|
mcote
:
review-
|
Details | Diff | Splinter Review |
The Flask-SSLify package is only used in the production environment on Heroku (when the "DYNO" environment variable is set). It depends on pyopenssl, which requires some OS packages, so it would be a lot more convenient for contributors if it was optional. This should be pretty simple. You would just need a try/except ImportError statement around the flask-sslify import and log a warning. Same thing in the part that actually calls SSLify(); catch the error and log something, then exit. To test this, just make sure that you don't have Flask-SSLify installed in your virtualenv (nor on your system) and make sure the app launches, the warning is logged, and the app continues to work fine. An even better test is to remove the pyopenssl package *and*, if you can, the libopenssl-dev apt package from your system. This will identify if there are other parts of the code that also need SSL support; we can fix those too.
Reporter | ||
Updated•8 years ago
|
Mentor: mcote
User Story: (updated)
Whiteboard: try: from flask_sslify import SSLify except ImportError: #log.debug('flask-sslify failed to import', exc_info=True) logging.warning('flask-sslify failed to import')
This is the final file that includes changes to req, dev_req and web.py
Reporter | ||
Updated•8 years ago
|
Attachment #8725067 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8725068 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8725075 -
Attachment is obsolete: true
Reporter | ||
Updated•8 years ago
|
Attachment #8725076 -
Attachment is obsolete: true
Reporter | ||
Comment 6•8 years ago
|
||
Comment on attachment 8725080 [details] [diff] [review] bug1245621_req.diff Review of attachment 8725080 [details] [diff] [review]: ----------------------------------------------------------------- I have a few little comments below. Can you also modify README.md to include information about dev_requirements.txt and requirements.txt? ::: pulseguardian/web.py @@ +18,4 @@ > redirect, > request, > jsonify) > +#from flask_sslify import SSLify Please remove this line rather than commenting it out. @@ +22,5 @@ > + > +try: > + from flask_sslify import SSLify > +except ImportError: > + logging.warning('flask-sslify failed to import') This is good, but it might be confusing to a new user. They might wonder what it means exactly. Can you change this text to "flask_sslify failed to import. This is okay for local environments but will cause a failure in production." ::: requirements.txt @@ +2,2 @@ > Flask-SSLify==0.1.5 > + Please remove this extra blank line.
Attachment #8725080 -
Flags: review-
Comment 7•7 years ago
|
||
I don't think this is necessary any more. pyopenssl is only required on old Python versions, and if someone is using a version that old, then they should update.
Updated•5 years ago
|
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → WONTFIX
You need to log in
before you can comment on or make changes to this bug.
Description
•