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

Bug 1562731 - upgrade to Python-3 #246

Merged
merged 6 commits into from
Nov 14, 2019

Conversation

djmitche
Copy link
Contributor

The only laggard dependency was flask_secure_headers, which appears unmaintained. It's just setting headers, though, so not too complicated. I vendored tarek's version from twaldear/flask-secure-headers#9. He had finished the meat of it (really just removing some useless try/except's and fixing whitespace) and was working on the tests.

The tests pass locally. I bet they'll pass in CI. But I have less confidence this will work right-off in a deployment. We should probably figure out a plan to deploy "safely":

  • for the webapp, rollback is pretty easy and we can just QA "manually"
  • for the backend queue-monitoring bit, we can deploy it in a read-only fashion and see what it logs, thereby avoiding the risk of some subtle bug causing it to delete everyone's queues or something crazy.

Anyway, I'd like to get @escapewindow to have a look at this first, then we can work on landing it.

Copy link

@escapewindow escapewindow left a comment

Choose a reason for hiding this comment

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

Stamping. If you need deeper flask review, that may not be me.

(I do see diffs if I clone Tarek's master into flask_secure_headers. The 3 patches after the vendor seem sane.)

@djmitche
Copy link
Contributor Author

Nothing flask-y changed here, so I think it's OK. I'll check the travis failures.

@ccooper
Copy link
Contributor

ccooper commented Nov 13, 2019

I checked the travis failures for you. :)

@djmitche
Copy link
Contributor Author

This has been rolled back in production. #251 appears unrelated (it reproduces with the patch rolled back) but #252 is definitely related.

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