Page MenuHomePhabricator

Maintenance scripts should fail on unknown parameters
Closed, ResolvedPublic

Description

Standard maintenance scripts (RUN_MAINTENANCE_IF_MAIN) should fail on unknown parameters.

This is good practice in general (e.g. most GNU/Linux commands do this) and would have prevented one cause of https://wikitech.wikimedia.org/wiki/Incident_documentation/20150825-Redis .

Event Timeline

Mattflaschen-WMF raised the priority of this task from to Needs Triage.
Mattflaschen-WMF updated the task description. (Show Details)

This follow-up task from an incident report has not been updated recently. If it is no longer valid, please add a comment explaining why. If it is still valid, please prioritize it appropriately relative to your other work. If you have any questions, feel free to ask me (Greg Grossmeier).

tstarling subscribed.

Originally commandLine.inc had a command line parser configured only with $optionsWithArgs, and every other option was presumed to be boolean and was put into a global variable $options for the specific script to deal with as desired. $optionsWithoutArgs was eventually introduced in 2016 and it looks like the remaining commandLine.inc callers in core now have all their options registered via these two global variables. So I think this could be done both for standard maintenance scripts and for commandLine.inc callers.

It's a breaking change, and it's hard to imagine how a deprecation period would work. We could do it without deprecation, but we would need to survey extensions. Presumably there would be some sort of opt-out mechanism for scripts that want to allow arbitrary options, like phpunit.php, which passes its command line through to a separate library.

In extensions, I count 380 scripts that use Maintenance.php, and 34 that use commandLine.inc. It's probably not feasible to manually open and assess 380 scripts. We could assume that the ones that use Maintenance.php are registering their options correctly unless they access the $argv global variable directly. Only 85 files contain "$argv", and in most of those it is a local variable.

Change 450097 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[mediawiki/core@master] Make maintenance scripts fail on unknown parameters

https://gerrit.wikimedia.org/r/450097

Change 450145 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[mediawiki/extensions/DumpHTML@master] Register all command line options

https://gerrit.wikimedia.org/r/450145

Change 450147 had a related patch set uploaded (by BPirkle; owner: BPirkle):
[mediawiki/extensions/GoToShell@master] Display both stdout and stderr output to the user

https://gerrit.wikimedia.org/r/450147

BPirkle triaged this task as Medium priority.Aug 3 2018, 2:22 PM

Change 450145 merged by jenkins-bot:
[mediawiki/extensions/DumpHTML@master] Register all command line options

https://gerrit.wikimedia.org/r/450145

Change 450147 merged by jenkins-bot:
[mediawiki/extensions/GoToShell@master] Display both stdout and stderr output to the user

https://gerrit.wikimedia.org/r/450147

Change 450097 merged by jenkins-bot:
[mediawiki/core@master] Make maintenance scripts fail on unknown parameters

https://gerrit.wikimedia.org/r/450097

There was one case of failure - one of the dumps scripts failed (see T201772). I would call this a success!

While I think it was unfeasible to check all scripts manually, it is a good idea when such a change with potential to break cron scripts and in general maintenance to shout out a warning to the ops list, so that we expect possible breakages and we keep a closer eye on maintenance scripts and their execution.