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

reload: update command should require some kind of confirmation #1992

Closed
cottongin opened this issue Nov 23, 2020 · 3 comments
Closed

reload: update command should require some kind of confirmation #1992

cottongin opened this issue Nov 23, 2020 · 3 comments
Assignees

Comments

@cottongin
Copy link
Contributor

While writing my own plugin that featured an update command I ran into this since Sopel will run all commands matching a defined command name/alias.

I should think running any sort of git pull command should require an extra step or confirmation, e.g. .update --yes, or something.

proc = subprocess.Popen('/usr/bin/git pull',
stdout=subprocess.PIPE,
stderr=subprocess.PIPE, shell=True)

To be honest I'm not entirely sure what the benefit of this command is and I'm beginning to align with @Exirel with his disdain for this plugin in general.

@dgw
Copy link
Member

dgw commented Nov 23, 2020

If the command becomes .update --yes, then someone will complain that it should be .update --yes --really… But a nonce approach would be reasonable: .update (or the nickname_command equivalent) on its own would send back in a NOTICE something like, "To confirm the update operation, send .update ABAD1DEA" — where ABAD1DEA is randomly generated for that request and stored somewhere in bot.memory until used or replaced (possibly also until some timeout period expires, but that might be unnecessary complication). This works well for dangerous services commands like /ns drop, and we might as well steal the pattern since bot admins are likely already familiar.

Personally, I've been thinking that .update belongs in a separate plugin anyway. All the heavy lifting is handled by the bot object itself, and the commands in reload just wrap those methods with argument checking & error messages (and permissions checks). There's no reason that we can't just split reload into two plugins instead of one when extracting things for 8.x.

@dgw dgw added this to the 8.0.0 milestone Nov 23, 2020
@dgw dgw self-assigned this Nov 23, 2020
@Exirel
Copy link
Contributor

Exirel commented Mar 3, 2023

We have two options:

For now, I'll remove the milestone, and @dgw will probably have an opinion on the keep or delete.

@Exirel Exirel removed this from the 8.0.0 milestone Mar 3, 2023
@dgw
Copy link
Member

dgw commented Mar 3, 2023

Oh, I forgot about this. Totally just close it; I'm not planning to re-add the .update command to reload even if it becomes an external plugin.

@dgw dgw closed this as not planned Won't fix, can't repro, duplicate, stale Mar 3, 2023
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

No branches or pull requests

3 participants