By default don't import 3rd-party Python modules, only stdlib ones. … #857

gjcarneiro · 2016-09-13T17:43:12Z

…Fixes #854.

This change is

dcramer · 2016-09-14T16:13:12Z

This will break a backwards compat change (where we let you configure the transport with the DSN), but as long as we mentioned that in the CHANGES I'm totally ok with it.

We also should simply remove the comments and just kill the lines (we're using version control after all).

gjcarneiro · 2016-09-14T17:56:22Z

So I fixed the tests and cleaned up the code. One issue that I had was that I have to access Client._registry, and we shouldn't be required to access attributes marked private. So I propose to rename, Client._registry to Client.registry, what do you think?

dcramer · 2016-09-15T05:29:06Z

Thanks!

I'm fine with renaming it to .registry as well.

dcramer

(still getting used to the code review process)

dcramer · 2016-09-15T05:29:57Z

CHANGES

+  from raven.transport.threaded_requests import ThreadedRequestsHTTPTransport
+  from raven.base import Client
+
+  for transport in [EventletHTTPTransport,


of note we dont actually need to do this since you can pass transport to the Client() itself. It was purely for backwards compatibility for a period of time.

But the problem is that a typical Django setup just adds raven.contrib.django.raven_compat to the list of INSTALLED_APPS. If they also have a weird DSN configured, they would need to register the transports in the registry for it to work, as the Django Sentry app creates its own Client() and I honestly have no idea how to customize creation of this Client()...

You can actually just pass it into SENTRY_CONFIG via 'transport' (same as all of the other config options).

Ah, OK then. I guess I can update the documentation to reflect this. Seems cleaner the way you suggest.

dcramer · 2016-09-15T05:30:11Z

conftest.py

+
+
+
+for transport in [EventletHTTPTransport,


assuming things break without this?

Sadly yes. The reason is that all tests build clients based on DSN, so a few test cases fail. And I guess using the DSN and registry is part of the things we test, so we can't remove it?

_________________________________________________________________________________________________ ThreadedTransportTest.test_does_send __________________________________________________________________________________________________ InvalidDsn: Unsupported Sentry DSN scheme: threaded+requests+http (threaded+requests+http://some_username:some_password@localhost:8143/1) __________________________________________________________________________________________ ThreadedTransportTest.test_shutdown_waits_for_send ___________________________________________________________________________________________ InvalidDsn: Unsupported Sentry DSN scheme: threaded+requests+http (threaded+requests+http://some_username:some_password@localhost:8143/1) _________________________________________________________________________________________________ RequestsTransportTest.test_does_send __________________________________________________________________________________________________ InvalidDsn: Unsupported Sentry DSN scheme: requests+http (requests+http://some_username:some_password@localhost:8143/1) ________________________________________________________________________________ TornadoTransportTests.test__sending_successfully_calls_success_callback ________________________________________________________________________________ InvalidDsn: Unsupported Sentry DSN scheme: tornado+http (tornado+http://uver:pass@localhost:46754/1) __________________________________________________________________________________ TornadoTransportTests.test__sending_with_error_calls_error_callback __________________________________________________________________________________ InvalidDsn: Unsupported Sentry DSN scheme: tornado+http (tornado+http://uver:pass@localhost:46754/1) ____________________________________________________________________________________________________ TornadoTransportTests.test_send ____________________________________________________________________________________________________ InvalidDsn: Unsupported Sentry DSN scheme: tornado+https (tornado+https://user:pass@host:1234/1?timeout=1&verify_ssl=1&ca_certs=/some/path/somefile)

Yeah we'd have to update these tests to pass it in directly, but I'd be ok with that in order to remove the legacy compatibility.

OK. Will probably have to wait a few days before I commit more changes, as I'll be travelling today (to PyCon UK) and I don't have a laptop.

dcramer

I think I left comments but GitHub is broken.

gjcarneiro · 2016-09-23T18:04:49Z

Please check if the latest commits address your comments.

dcramer · 2016-09-23T18:55:25Z

Looks good we just need to update the tornado tests to remove the tornado+ prefix I think?

gjcarneiro · 2016-09-28T11:38:20Z

Hm, some Tornado test failing on Python 2.6, but I don't know why :(
Also I am unable to reproduce on my Ubuntu 16.04 box because of this bug: https://bugs.debian.org/cgi-bin/bugreport.cgi?bug=754248
Can we just drop Python 2.6 support?

mitsuhiko · 2016-10-18T09:41:19Z

2.6 is very much alive, we can't kill it yet.

gjcarneiro · 2017-05-22T16:44:35Z

It's all very fine to say "we must support Python2.6", but you have to realise it is incredibly hard, today, to do anything with Python2.6. After much insistence, I managed to get a docker image with python2.6 and pip, but even then I didn't manage to run the tests. The latest error is:

Collecting billiard<3.6.0,>=3.5.0.2 (from celery>=2.5->raven)
  Downloading billiard-3.5.0.2.tar.gz (157kB)
    100% |################################| 163kB 5.7MB/s 
    Complete output from command python setup.py egg_info:
    Traceback (most recent call last):
      File "<string>", line 1, in <module>
      File "/tmp/pip-build-QHA8oe/billiard/setup.py", line 90, in <module>
        raise ValueError('Versions of Python before 2.7 are not supported')
    ValueError: Versions of Python before 2.7 are not supported
    
    ----------------------------------------
Command "python setup.py egg_info" failed with error code 1 in /tmp/pip-build-QHA8oe/billiard/
make: *** [bootstrap] Error 1
root@45ac8d430fc8:/code#

Please, please, reconsider Python 2.6, it is dragging raven-python down!

Maybe rebrand raven-python into a new major release and state in the release notes that whoever needs Python 2.6 support should continue using the old raven version?

Oh, drat, I see now you had a major release recently :(

There must be something we can do, this is ridiculous. How are we supposed to fix tests if can't even run them?

By default don't import 3rd-party Python modules, only stdlib ones. F…

096245d

…ixes getsentry#854.

Gustavo Carneiro added 3 commits September 14, 2016 17:35

Add note about non-stdlib transports no longer being imported by default

7a6a6a7

Remove commented out code

f339226

Fix tests

55b8dad

dcramer reviewed Sep 15, 2016

View reviewed changes

Gustavo Carneiro added 2 commits September 23, 2016 19:02

Don't enable compatibility registration of all transports in tests.

6550d8b

In CHANGES, recommend to use the transport kwarg of raven.Client()

049b98b

gjcarneiro and others added 2 commits September 23, 2016 22:40

tornado test can now use http:// as dsn instead of tornado+http://

11c58cd

Fix gevent transport unit test

6125748

Merge remote-tracking branch 'origin/master' into dont_import_the_world

5151958

ashwoods added this to the 6.2.0 milestone Jul 17, 2017

ashwoods added the Improvement label Jul 17, 2017

ashwoods modified the milestones: 6.2.0, 6.3.0 Sep 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

By default don't import 3rd-party Python modules, only stdlib ones. … #857

By default don't import 3rd-party Python modules, only stdlib ones. … #857

gjcarneiro commented Sep 13, 2016 •

edited by dcramer

Loading

dcramer commented Sep 14, 2016

gjcarneiro commented Sep 14, 2016

dcramer commented Sep 15, 2016 •

edited

Loading

dcramer left a comment

dcramer Sep 15, 2016

gjcarneiro Sep 15, 2016

dcramer Sep 15, 2016

gjcarneiro Sep 15, 2016

dcramer Sep 15, 2016

gjcarneiro Sep 15, 2016

dcramer Sep 15, 2016

gjcarneiro Sep 15, 2016

dcramer left a comment

gjcarneiro commented Sep 23, 2016

dcramer commented Sep 23, 2016

gjcarneiro commented Sep 28, 2016 •

edited

Loading

mitsuhiko commented Oct 18, 2016

gjcarneiro commented May 22, 2017

By default don't import 3rd-party Python modules, only stdlib ones. … #857

Are you sure you want to change the base?

By default don't import 3rd-party Python modules, only stdlib ones. … #857

Conversation

gjcarneiro commented Sep 13, 2016 • edited by dcramer Loading

dcramer commented Sep 14, 2016

gjcarneiro commented Sep 14, 2016

dcramer commented Sep 15, 2016 • edited Loading

dcramer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcramer left a comment

Choose a reason for hiding this comment

gjcarneiro commented Sep 23, 2016

dcramer commented Sep 23, 2016

gjcarneiro commented Sep 28, 2016 • edited Loading

mitsuhiko commented Oct 18, 2016

gjcarneiro commented May 22, 2017

gjcarneiro commented Sep 13, 2016 •

edited by dcramer

Loading

dcramer commented Sep 15, 2016 •

edited

Loading

gjcarneiro commented Sep 28, 2016 •

edited

Loading