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

Prevent middleware error on older Django versions (<1.8) #603

Merged
merged 8 commits into from Apr 9, 2019

Conversation

wsabransky
Copy link
Contributor

No description provided.

@wsabransky wsabransky requested review from c24t, reyang, songy23 and a team as code owners April 4, 2019 21:23
@reyang
Copy link
Contributor

reyang commented Apr 4, 2019

@zoidbergwill FYI

Copy link
Contributor

@reyang reyang left a comment

Choose a reason for hiding this comment

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

LGTM, probably code coverage would complain during CI.

@reyang
Copy link
Contributor

reyang commented Apr 4, 2019

@wsabransky
Copy link
Contributor Author

@reyang Yep, I've tested this on 1.6.5 and it runs.

@wsabransky
Copy link
Contributor Author

Hmm, it seems like unit tests are failing for reasons potentially not related to this PR. Maybe due to this being moved into its own package?

@reyang
Copy link
Contributor

reyang commented Apr 4, 2019

@wsabransky Great! Probably we can change the required Django version in setup.py. What would be your suggestion on the lowest version?

@reyang
Copy link
Contributor

reyang commented Apr 4, 2019

The CI failure was introduced by the PostgreSQL lib update.
I have a fix in #602.

@wsabransky
Copy link
Contributor Author

@reyang I don't know enough about Django before 1.6.x to say with confidence that it would work. But 1.6.x is definitely safe.

@reyang
Copy link
Contributor

reyang commented Apr 4, 2019

@wsabransky Great to know. Thank you!

@reyang
Copy link
Contributor

reyang commented Apr 5, 2019

@wsabransky I have the CI fixed, would you rebase to the latest master and try?

@wsabransky
Copy link
Contributor Author

@reyang done

@zoidyzoidzoid
Copy link

zoidyzoidzoid commented Apr 5, 2019

@reyang Thanks for the ping, when we add support for testing multiple Django versions, we can test all the currently maintained versions then probably, which is great.

@c24t
Copy link
Member

c24t commented Apr 5, 2019

FWIW we've been setting the minimum django version according to django's support timeline even though the opencensus middleware is technically compatible with older versions. We do the same thing for flask.

Since we only depend on django to instrument it (and aren't e.g. running a django service ourselves), most of the usual security concerns don't apply. On the other hand, it makes our lives easier if we always set the minimum required versions of our dependencies such that it's impossible to accidentally install an old unsupported version.

@wsabransky
Copy link
Contributor Author

@c24t Makes sense. I've reverted the supported minimum version change. Anything else you'd like to see to get this merged?

@c24t
Copy link
Member

c24t commented Apr 8, 2019

Looks like lint and coverage checks are still failing.

@c24t c24t merged commit b1075ce into census-instrumentation:master Apr 9, 2019
@wsabransky
Copy link
Contributor Author

@c24t Any chance we could get this merged into a minor version update for the django extension?

@c24t
Copy link
Member

c24t commented Apr 30, 2019

@wsabransky in case you haven't seen it already: this was released in https://github.com/census-instrumentation/opencensus-python/releases/tag/v0.5.0.

@reyang reyang mentioned this pull request May 3, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants