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

"Run command" module #1366

Closed
wants to merge 2 commits into from
Closed

Conversation

ralienpp
Copy link

@ralienpp ralienpp commented Aug 19, 2018

This adds a new module to Sopel's core, making it able to run a command received via IRC and return its status code and output, or PID (if ran in the background).

There are no dependencies, this is based entirely on the features provided by Python's standard library.

Examples of use:

  • .run touch /tmp/test.txt runs touch /tmp/test.txt and displays the status code and output like Status: 0, Stdout: <<EMPTY
  • .runbackground sleep 30 runs sleep 30 in the background and reports the PID of the new process via IRC as Started process XXX
  • .run cat /var/log/data.txt | grep matches - this example illustrates how multiple commands can be piped.

@dgw
Copy link
Member

dgw commented Aug 20, 2018

Hmm… I'm not sure this is something that should ship with Sopel. Any module in the core set will be enabled by default (unless the configuration defaults are modified), and this could have real consequences for bot owners who give admin rights to someone they shouldn't—especially since new users tend to run Sopel as their own system user.

What would you say to packaging this as a third-party module installable from PyPI? One way or another the reloading issue with those is going to be fixed in the next Sopel release, and it's pretty simple to make a module package. I'm just really leery of modules that allow running arbitrary stuff… even ipython should probably be split into an external module, since it's really targeted at module developers.

@dgw
Copy link
Member

dgw commented Sep 12, 2018

Closing this, as it's not likely to be merged unless I read a really compelling argument.

@dgw dgw closed this Sep 12, 2018
@ralienpp
Copy link
Author

ralienpp commented Oct 4, 2018

Hi, I didn't react initially because I agreed with your rationale, this thing is indeed a sharp tool that inexperienced users can cut themselves with. But on a second thought, I believe that the following arguments in favour of including it are valid:

  • the module relies exclusively on stdlib and has no third party dependencies
  • it should be disabled by default (using the exclude feature of the config syntax), thus inexperienced users won't "cut themselves"
  • the final point is that, depending on the statistics about who actually uses sopel and for what purpose, it could be the case that advanced users are implementing their own version of this anyway.

@dgw
Copy link
Member

dgw commented Oct 4, 2018

Requiring no additional dependencies is a basic soft-requirement for all core modules, honestly, unless they're doing something sufficiently complicated. (That said, even modules like reddit would have probably ended up as third-party modules if I had been maintainer at the time.)

The only way to get relevant user statistics is from self-reporting, since Sopel has no built-in way of gathering metrics (and won't as long as I have a say). One could argue that someone who would run an IRC bot is already likely to basically know what they're doing… but plenty of new Sopel users ask pretty basic questions that would indicate otherwise. It definitely puts me in the mindset of "Don't let users do anything potentially risky unless they explicitly try."

Disabling the module by default is something that would work, though. I just tested it to make sure (and to verify that putting an empty exclude = in the config correctly overrides the default value). I'll reopen this for consideration, and probably blacklist ipython by default as well while I'm at it…

@dgw dgw reopened this Oct 4, 2018
@dgw
Copy link
Member

dgw commented Oct 4, 2018

@ralienpp Would you like me to just fix the code style myself if I decide to merge this, so you needn't spend any more time on it? flake8 failed the tests.

@ralienpp
Copy link
Author

ralienpp commented Oct 5, 2018

I updated it myself, and from now on I will make sure I use the checkstyle.sh script before submitting patches.

@dgw
Copy link
Member

dgw commented Jan 23, 2019

OK, @ralienpp, I'll give it to you straight. I think you deserve that, without beating around the bush.

Per #1291, Sopel is moving toward moving most of the core module set into external packages. The existing modules will get bug fixes if they break, but we're really trying to avoid adding new ones at this point.

Between that and the above reasons stated against including this in core, I'm going to close this again. (As for why now… it's because @Exirel linked this PR in #sopel this evening. 😸)

I encourage you to package it with the module template and release it to PyPI yourself! I'm also working on a module database (see sopel-irc/module-db) that will accept submissions of user-created modules. You'll be welcome to submit an entry for your module there even if it's not packaged and uploaded to PyPI—the site design (as currently prototyped) will accept either a package name or a source link.

@dgw dgw added the Declined Requests that will not be implemented for technical or project direction reasons label Jan 23, 2019
@dgw dgw closed this Jan 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Declined Requests that will not be implemented for technical or project direction reasons Feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants