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)

defect

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)

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.
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')
Attached file requirements.txt (obsolete) —
Attached file dev_requirements.txt (obsolete) —
Attached patch bug1245621.diff (obsolete) — Splinter Review
Attached patch bug1245621_req.diff (obsolete) — Splinter Review
This is the final file that includes changes to req, dev_req and web.py
Attachment #8725067 - Attachment is obsolete: true
Attachment #8725068 - Attachment is obsolete: true
Attachment #8725075 - Attachment is obsolete: true
Attachment #8725076 - Attachment is obsolete: true
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-
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.
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.

Attachment

General

Creator:
Created:
Updated:
Size: