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

[MRG+1] Relocate telnetconsole to extensions/ #1524

Merged
merged 1 commit into from Oct 30, 2015

Conversation

@Digenis
Copy link
Member

@Digenis Digenis commented Oct 5, 2015

Fixes #1520

@kmike
Copy link
Member

@kmike kmike commented Oct 5, 2015

Thanks @Digenis!

We need to think about backwards compatiility: say, people disabled Telnet extension using EXTENSIONS option (as shown in the example in generated settings.py) - it shouldn't become enabled after the relocation. It is better to have a test for it. Adding it here may be enough, but I'm not sure. //cc @curita

@curita
Copy link
Member

@curita curita commented Oct 5, 2015

Adding it to DEPRECATION_RULES in scrapy/scrapy/utils/deprecate.py should be enough. It's already tested, so there's no need to add a separate test for telnet's deprecation. I've been meaning to write a wiki entry for this kind of relocations/deprecations, probably I should get started 😄

@Digenis: Grepping 'telnet' in the repo shows that there are pending scrapy.telnet references in docs/topics/settings.rst, docs/topics/telnetconsole.rst and scrapy/templates/project/module/settings.py.tmpl, could you fix those too?

@Digenis Digenis force-pushed the Digenis:relocate_telnet_console branch from f5ca2d6 to d523c75 Oct 5, 2015
@kmike kmike changed the title Relocate telnetconsole to extensions/ [MRG+1] Relocate telnetconsole to extensions/ Oct 5, 2015
@kmike kmike added this to the Scrapy 1.1 milestone Oct 6, 2015
@Digenis
Copy link
Member Author

@Digenis Digenis commented Oct 9, 2015

Thank you both.

Something to consider before merging.
https://github.com/scrapy/scrapy/wiki/PY3:-Twisted-Dependencies#twistedconch

twisted.conch

Needs porting. Another alternative solution would be moving the telnet module into a contrib module. It's useful for debugging, but it's not a core part of Scrapy.

@nyov
Copy link
Contributor

@nyov nyov commented Oct 9, 2015

@Digenis
Copy link
Member Author

@Digenis Digenis commented Oct 9, 2015

This is out of the scope of this PR
but I feel for what nyov wants to suggest
and I say I also would rather have it disabled by default
as I already do in all of my projects.

@nyov
Copy link
Contributor

@nyov nyov commented Oct 9, 2015

Why would it be out of scope? It's a change from an "essential" component into an extension, as part of "downgrading" it into an extension, I feel it could also be disabled by default.
Especially because of the missing py3 port. Not?

@kmike
Copy link
Member

@kmike kmike commented Oct 9, 2015

Telnet extension is conditionally disabled when twisted.conch is not importable, so it is disabled in Python 3. I haven't thought of this move as of "downgrading", only as of a cleanup, but I see what you're heading to.

The problem with having telnet disabled by default is that if it is disabled and you need it (e.g. to debug an issue with a long-running spider without stopping it) you can't enable it without stopping the spider, which kind of defeats the purpose of telnet extension.

I think this extension is started/stopped in a wrong place: it should be a 'crawler process extension', not spider/crawler extension; it shouldn't be started for every spider if there are multiple spiders in a process. Because of this reason I'm +0 to disable it by default; it feels wrong. But we don't have 'crawler process extensions' in Scrapy, so it is a moot point.

@Digenis
Copy link
Member Author

@Digenis Digenis commented Oct 9, 2015

Telnet extension is conditionally disabled

More like: Telnet extension is conditionally enabled
(sorry, couldn't help it 😜)

@nyov
Copy link
Contributor

@nyov nyov commented Oct 9, 2015

Okay I agree with that reasoning, when you need it and forgot to enable it.

But it's more burden for the scripting/embedding approach, where one wants the minimal only.
As the minimum I have this boilerplate now:

from scrapy.settings import Settings
settings = Settings()
# or
#from scrapy.utils.project import get_project_settings
#settings = get_project_settings()
settings.setdict({
    'EXTENSIONS': {
        'scrapy.telnet.TelnetConsole': None,
    },  
    'DOWNLOAD_HANDLERS': {'s3': None},
})
process = CrawlerProcess(settings)

Not needing to pull in Settings here at all would make for smaller scripts.

@kmike
Copy link
Member

@kmike kmike commented Oct 9, 2015

@nyov yeah, I'm also disabling telnet console with CrawlerProcess. Btw, Settings is not necessary because CrawlerProcess accepts dicts, and s3 handler shouldn't cause problems now (it used to cause 1s startup delay).

@nyov
Copy link
Contributor

@nyov nyov commented Oct 10, 2015

Ah, right, nice I can skimp on an import there (and removing the s3 workaround after 1.1).

I'd still favor the minimalist approach, to disable telnet by default though. I'd hazard the guess that most people aren't using it, maybe not even quite aware of it.
(I remember having a good laugh after reading about someone remotely killing a scraper they found crawling their site, with that. Now it binds to localhost at least. But that means it's less for remote-admin and mostly used as a debugging tool. On that basis it makes more sense to enable on a project basis.)
When running scrapy the usual way (with a project env) it's pulling in configs left and right. There should be enough options still to have telnet enabled globally or on a user basis as a personal default, I think. That's my 2cents, and I'll stop bugging y'all now :)

@curita
Copy link
Member

@curita curita commented Oct 30, 2015

I'll be merging this since I don't see how we'll end the discussion of disabling telnet by default anytime soon 😉

I think another possibility could be to have telnet disabled in the default settings, but have it enabled in the settings.py template when you create a project. That way new projects created by scrapy startproject would have it enabled, and in all other cases it'd be disabled by default.

Anyway, I'll open up a new ticket to continue the discussion there.

curita added a commit that referenced this pull request Oct 30, 2015
[MRG+1] Relocate telnetconsole to extensions/
@curita curita merged commit 72eeead into scrapy:master Oct 30, 2015
0 of 2 checks passed
0 of 2 checks passed
codecov/patch CI failed: coverage not measured fully.
Details
continuous-integration/travis-ci/pr The Travis CI build failed
Details
@Digenis Digenis deleted the Digenis:relocate_telnet_console branch Jan 21, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

4 participants
You can’t perform that action at this time.