# Support sub-commands on cli.Application methodsClosedPublicActions

Authored by mmodell on Sep 14 2016, 10:18 AM.

# Details

T142880: Create `scap swat` command to automate patch merging & testing during a swat deployment
Reviewers
 thcipriani
Group Reviewers
 Release-Engineering-Team
Commits
rMSCA0d76e5fb54a5: Support sub-commands on cli.Application methods
Patch without arc
Summary

### What's new:

• cli.Application now supports sub-sub commands
• @cli.argument applies to any class methods, not just main()
• @cli.argument can also be applied directly to the class to specify args which are shared among all sub-commands
• I also significantly refactored the argparse code in arg.py and improved help formatting.
• Removed deprecated support for calling scap scripts directly (e.g. no more sync-file). I didn't remove the script files, I just added 'sys.exit(1)' to them. It may be time, however, to remove them entirely.

This is needed for my related work on scap swat which is pending review in gerrit: https://gerrit.wikimedia.org/r/#/c/306259/

### Demo

https://asciinema.org/a/1x54kw77tvatxiqv45ba6ael7

### Example usage

```@cli.command('swat', help='Mediawiki SWAT deployment helper.', subcommands=True)
@cli.argument('-b', '--branch', nargs="?",
help='One or more branches to merge into.'
+ ' Default: active wmf/branches.')
@cli.argument('-m', '--message', nargs=1,
help='Include a message or comment with your action.')
class Swat(cli.Application):
'''
scap swat: cherry-pick and deploy patches for Mediawiki SWAT.
'''

@cli.argument('term', nargs='+',
help='Gerrit search term(s). E.g: status:open owner:self')
@cli.argument('-n', type=int, nargs=1, default='10',
help='Number of results to show. Default: 10')
def query(self, args):
''' execute a gerrit query '''
# implement query sub-command here

@cli.argument('changeid', nargs=1,
help='The ChangeId of a patch to revert.')
def revert(self, changeids):
''' revert a change in gerrit '''
# implement revert sub-command here```

### Help output: scap swat -h

```usage: scap swat [-h] <sub-command> ...

scap swat: cherry-pick and deploy patches for Mediawiki SWAT.

optional arguments:
-h, --help     show this help message and exit

swat sub-commands:
Mediawiki SWAT deployment helper.

<sub-command>
query        execute a gerrit query
revert       revert a change in gerrit```

### Help output: scap swat query -h

```usage: scap swat query [-h] [-b [BRANCH]] [-m MESSAGE] [-n N] term [term ...]

execute a gerrit query

positional arguments:
term                  Gerrit search term(s). E.g: status:open owner:self

optional arguments:
-h, --help            show this help message and exit
-b [BRANCH], --branch [BRANCH]
One or more branches to merge into. Default: active
wmf/branches.
-m MESSAGE, --message MESSAGE
Include a message or comment with your action.
-n N                  Number of results to show. Default: 10```
Test Plan

I've tested the arg parsing fairly thoroughly, including autocomplete.
I haven't tested every scap command so it's possible that I broke something.
Testing on beta would definitely be a good idea.

# Diff Detail

Repository
rMSCA Scap
Branch
master
Lint
 Lint Passed
SeverityLocationCodeMessage
Unit
 Tests Passed
Build Status
 Buildable 977 Build 1420: differential-jessie Jenkins Build 1419: arc lint + arc unit

### Event Timeline

There are a very large number of changes, so older changes are hidden. Show Older Changes

W: scap source: changelog-should-mention-nmu
N:
N: When you NMU a package, that fact should be mentioned on the first line
N: in the changelog entry. Use the words "NMU" or "Non-maintainer upload"
N: (case insensitive).
N:
N: Maybe you didn't intend this upload to be a NMU, in that case, please
N: double-check that the most recent entry in the changelog is
N: byte-for-byte identical to the maintainer or one of the uploaders. If
N: this is a local package (not intended for Debian), you can suppress this
N: warning by putting "local" in the version number or "local package" on
N: the first line of the changelog entry.
N:
N: Refer to Debian Developer's Reference section 5.11.3 (Using the DELAYED/
N: queue) for details.
N:
N: Severity: normal, Certainty: certain
N:
N: Check: nmu, Type: source
N:
W: scap source: source-nmu-has-incorrect-version-number 3.2.5-1+0~20160921231540.116~1.gbp1246ac
N:
N: A source NMU should have a Debian
https://integration.wikimedia.org/ci/job/beta-build-deb/116/ for more details.

W: scap source: changelog-should-mention-nmu
N:
N: When you NMU a package, that fact should be mentioned on the first line
N: in the changelog entry. Use the words "NMU" or "Non-maintainer upload"
N: (case insensitive).
N:
N: Maybe you didn't intend this upload to be a NMU, in that case, please
N: double-check that the most recent entry in the changelog is
N: byte-for-byte identical to the maintainer or one of the uploaders. If
N: this is a local package (not intended for Debian), you can suppress this
N: warning by putting "local" in the version number or "local package" on
N: the first line of the changelog entry.
N:
N: Refer to Debian Developer's Reference section 5.11.3 (Using the DELAYED/
N: queue) for details.
N:
N: Severity: normal, Certainty: certain
N:
N: Check: nmu, Type: source
N:
W: scap source: source-nmu-has-incorrect-version-number 3.2.5-1+0~20160923001133.118~1.gbpe6769d
N:
N: A source NMU should have a Debian
https://integration.wikimedia.org/ci/job/beta-build-deb/118/ for more details.

W: scap source: changelog-should-mention-nmu
N:
N: When you NMU a package, that fact should be mentioned on the first line
N: in the changelog entry. Use the words "NMU" or "Non-maintainer upload"
N: (case insensitive).
N:
N: Maybe you didn't intend this upload to be a NMU, in that case, please
N: double-check that the most recent entry in the changelog is
N: byte-for-byte identical to the maintainer or one of the uploaders. If
N: this is a local package (not intended for Debian), you can suppress this
N: warning by putting "local" in the version number or "local package" on
N: the first line of the changelog entry.
N:
N: Refer to Debian Developer's Reference section 5.11.3 (Using the DELAYED/
N: queue) for details.
N:
N: Severity: normal, Certainty: certain
N:
N: Check: nmu, Type: source
N:
W: scap source: source-nmu-has-incorrect-version-number 3.3.0-1+0~20160924020305.119~1.gbpde16e9
N:
N: A source NMU should have a Debian
https://integration.wikimedia.org/ci/job/beta-build-deb/119/ for more details.

rebase on top of origin/master

W: scap source: changelog-should-mention-nmu
N:
N: When you NMU a package, that fact should be mentioned on the first line
N: in the changelog entry. Use the words "NMU" or "Non-maintainer upload"
N: (case insensitive).
N:
N: Maybe you didn't intend this upload to be a NMU, in that case, please
N: double-check that the most recent entry in the changelog is
N: byte-for-byte identical to the maintainer or one of the uploaders. If
N: this is a local package (not intended for Debian), you can suppress this
N: warning by putting "local" in the version number or "local package" on
N: the first line of the changelog entry.
N:
N: Refer to Debian Developer's Reference section 5.11.3 (Using the DELAYED/
N: queue) for details.
N:
N: Severity: normal, Certainty: certain
N:
N: Check: nmu, Type: source
N:
W: scap source: source-nmu-has-incorrect-version-number 3.3.0-1+0~20160924034458.120~1.gbpde16e9
N:
N: A source NMU should have a Debian
https://integration.wikimedia.org/ci/job/beta-build-deb/120/ for more details.

Mostly lgtm, there are a few nitpicks inline, plus the pep8 errors.

I have a concern about the MediawikiVersionAction breaking l10nupdate-1.

I haven't done a full review, sorry :(

There are a lot of changes here.

debian/bash_completion.d/scap
3

This doesn't match the format of the file

scap/arg.py
93

I'm not 100% on this, but I think this breaks l10nupdate-1. I don't think it ever cds to /srv/mediawiki-staging Also, I'm not sure this would support the cron call: https://github.com/wikimedia/operations-puppet/blob/production/modules/scap/manifests/l10nupdate.pp#L80

116

What is this for?

This revision now requires changes to proceed.Sep 24 2016, 5:03 PM
scap/arg.py
93

Damnit I think you're right.

I could hard-code the path the /srv/mediawiki-staging but that feels nasty. I can't see any better way though, unfortunately, because arg parsing has to happen before the config is loaded.

So should I just drop this and not validate the arg? It's nice to have auto-complete of the version...

116

This allows you to override an existing argument, and solves errors that result from args (such as -h) which exist in the default set but also get added to one or more of the subparsers.

Don't use MediaWikiVersionAction

W: scap source: changelog-should-mention-nmu
N:
N: When you NMU a package, that fact should be mentioned on the first line
N: in the changelog entry. Use the words "NMU" or "Non-maintainer upload"
N: (case insensitive).
N:
N: Maybe you didn't intend this upload to be a NMU, in that case, please
N: double-check that the most recent entry in the changelog is
N: byte-for-byte identical to the maintainer or one of the uploaders. If
N: this is a local package (not intended for Debian), you can suppress this
N: warning by putting "local" in the version number or "local package" on
N: the first line of the changelog entry.
N:
N: Refer to Debian Developer's Reference section 5.11.3 (Using the DELAYED/
N: queue) for details.
N:
N: Severity: normal, Certainty: certain
N:
N: Check: nmu, Type: source
N:
W: scap source: source-nmu-has-incorrect-version-number 3.3.0-1+0~20160926210328.121~1.gbpde16e9
N:
N: A source NMU should have a Debian
https://integration.wikimedia.org/ci/job/beta-build-deb/121/ for more details.

Handle mediawiki versions in bash completion shell script

W: scap source: changelog-should-mention-nmu
N:
N: When you NMU a package, that fact should be mentioned on the first line
N: in the changelog entry. Use the words "NMU" or "Non-maintainer upload"
N: (case insensitive).
N:
N: Maybe you didn't intend this upload to be a NMU, in that case, please
N: double-check that the most recent entry in the changelog is
N: byte-for-byte identical to the maintainer or one of the uploaders. If
N: this is a local package (not intended for Debian), you can suppress this
N: warning by putting "local" in the version number or "local package" on
N: the first line of the changelog entry.
N:
N: Refer to Debian Developer's Reference section 5.11.3 (Using the DELAYED/
N: queue) for details.
N:
N: Severity: normal, Certainty: certain
N:
N: Check: nmu, Type: source
N:
W: scap source: source-nmu-has-incorrect-version-number 3.3.0-1+0~20160926212920.122~1.gbpde16e9
N:
N: A source NMU should have a Debian
https://integration.wikimedia.org/ci/job/beta-build-deb/122/ for more details.

W: scap source: changelog-should-mention-nmu
N:
N: When you NMU a package, that fact should be mentioned on the first line
N: in the changelog entry. Use the words "NMU" or "Non-maintainer upload"
N: (case insensitive).
N:
N: Maybe you didn't intend this upload to be a NMU, in that case, please
N: double-check that the most recent entry in the changelog is
N: byte-for-byte identical to the maintainer or one of the uploaders. If
N: this is a local package (not intended for Debian), you can suppress this
N: warning by putting "local" in the version number or "local package" on
N: the first line of the changelog entry.
N:
N: Refer to Debian Developer's Reference section 5.11.3 (Using the DELAYED/
N: queue) for details.
N:
N: Severity: normal, Certainty: certain
N:
N: Check: nmu, Type: source
N:
W: scap source: source-nmu-has-incorrect-version-number 3.3.0-1+0~20160926213538.123~1.gbpde16e9
N:
N: A source NMU should have a Debian
https://integration.wikimedia.org/ci/job/beta-build-deb/123/ for more details.

scap/arg.py
17

I can't figure out how to include this url, as valid reStructuredText, without going over 79 chars

W: scap source: changelog-should-mention-nmu
N:
N: When you NMU a package, that fact should be mentioned on the first line
N: in the changelog entry. Use the words "NMU" or "Non-maintainer upload"
N: (case insensitive).
N:
N: Maybe you didn't intend this upload to be a NMU, in that case, please
N: double-check that the most recent entry in the changelog is
N: byte-for-byte identical to the maintainer or one of the uploaders. If
N: this is a local package (not intended for Debian), you can suppress this
N: warning by putting "local" in the version number or "local package" on
N: the first line of the changelog entry.
N:
N: Refer to Debian Developer's Reference section 5.11.3 (Using the DELAYED/
N: queue) for details.
N:
N: Severity: normal, Certainty: certain
N:
N: Check: nmu, Type: source
N:
W: scap source: source-nmu-has-incorrect-version-number 3.3.0-1+0~20160926215028.124~1.gbpde16e9
N:
N: A source NMU should have a Debian
https://integration.wikimedia.org/ci/job/beta-build-deb/124/ for more details.

strip slash from version arg

W: scap source: changelog-should-mention-nmu
N:
N: When you NMU a package, that fact should be mentioned on the first line
N: in the changelog entry. Use the words "NMU" or "Non-maintainer upload"
N: (case insensitive).
N:
N: Maybe you didn't intend this upload to be a NMU, in that case, please
N: double-check that the most recent entry in the changelog is
N: byte-for-byte identical to the maintainer or one of the uploaders. If
N: this is a local package (not intended for Debian), you can suppress this
N: warning by putting "local" in the version number or "local package" on
N: the first line of the changelog entry.
N:
N: Refer to Debian Developer's Reference section 5.11.3 (Using the DELAYED/
N: queue) for details.
N:
N: Severity: normal, Certainty: certain
N:
N: Check: nmu, Type: source
N:
W: scap source: source-nmu-has-incorrect-version-number 3.3.0-1+0~20160927055712.125~1.gbpde16e9
N:
N: A source NMU should have a Debian
https://integration.wikimedia.org/ci/job/beta-build-deb/125/ for more details.

• add @subcommand decorator and slightly fix up bash completion

W: scap source: changelog-should-mention-nmu
N:
N: When you NMU a package, that fact should be mentioned on the first line
N: in the changelog entry. Use the words "NMU" or "Non-maintainer upload"
N: (case insensitive).
N:
N: Maybe you didn't intend this upload to be a NMU, in that case, please
N: double-check that the most recent entry in the changelog is
N: byte-for-byte identical to the maintainer or one of the uploaders. If
N: this is a local package (not intended for Debian), you can suppress this
N: warning by putting "local" in the version number or "local package" on
N: the first line of the changelog entry.
N:
N: Refer to Debian Developer's Reference section 5.11.3 (Using the DELAYED/
N: queue) for details.
N:
N: Severity: normal, Certainty: certain
N:
N: Check: nmu, Type: source
N:
W: scap source: source-nmu-has-incorrect-version-number 3.3.0-1+0~20160928173947.126~1.gbpde16e9
N:
N: A source NMU should have a Debian
https://integration.wikimedia.org/ci/job/beta-build-deb/126/ for more details.

W: scap source: changelog-should-mention-nmu
N:
N: When you NMU a package, that fact should be mentioned on the first line
N: in the changelog entry. Use the words "NMU" or "Non-maintainer upload"
N: (case insensitive).
N:
N: Maybe you didn't intend this upload to be a NMU, in that case, please
N: double-check that the most recent entry in the changelog is
N: byte-for-byte identical to the maintainer or one of the uploaders. If
N: this is a local package (not intended for Debian), you can suppress this
N: warning by putting "local" in the version number or "local package" on
N: the first line of the changelog entry.
N:
N: Refer to Debian Developer's Reference section 5.11.3 (Using the DELAYED/
N: queue) for details.
N:
N: Severity: normal, Certainty: certain
N:
N: Check: nmu, Type: source
N:
W: scap source: source-nmu-has-incorrect-version-number 3.3.0-1+0~20160928181150.127~1.gbpde16e9
N:
N: A source NMU should have a Debian
https://integration.wikimedia.org/ci/job/beta-build-deb/127/ for more details.

I've spent a while on review of this. Currently full of nitpicks.

The only function that I have no idea about is ScapArgParser.get_valid_next_words—it's a hairy beast—I need fresh eyes to look at what it's doing. For now I'm submitting review as-is so that I don't delay it any longer.

Is it only used for getting command completions?

debian/bash_completion.d/scap
22

I think you probably want ls -1. ls -C seems to work fine, but seems like a weird choice

54

This is a neat trick.

Seems to only be activated in a very narrow set of circumstances. This works:

`scap l10n-purge --version<tab><tab>`

this doesn't:

```scap l10n-purge --version p<tab><tab>
scap l10n-purge --version 1<tab><tab>```
55

Remove commented-out code.

docs/scap2/index.rst
3

Dropping the ".0" is inconsistent with the rest of the docs.

7

"MediaWiki"
"itself"

Maybe use a ";" rather than a ",", i.e., " newer scap 3.0 architecture; however, ..."

20

This is nice!

scap/arg.py
8

for whatever reason my email is <tcipriani@wikimedia.org> :\

17

could just call it py_completion

42–55

Maybe use shlex.split here?

128

Can you maybe add some comments to this function? I'm having a hard time following it.

129

Is words < 1 an exceptional case? When will that happen?

138

What is happening here? Can you add a comment for why this is necessary? Also can you put in some spaces:

`words[-1 - (i + 1)]`
144

spaces

145

spaces

149

spaces

scap/cli.py
333–354

eh, I'm not sure this is compliant with pep257

352

This is dark.

scap/main.py
274

I suppose this is no longer needed since it's handled in the version argument type.

Mostly seems fine aside from nitpicks.

That completion code is unassailable and ugly. I'll leave it up to you if you want to land it.

This revision is now accepted and ready to land.Sep 30 2016, 9:07 PM
• Clean up completion code slightly

W: scap source: changelog-should-mention-nmu
N:
N: When you NMU a package, that fact should be mentioned on the first line
N: in the changelog entry. Use the words "NMU" or "Non-maintainer upload"
N: (case insensitive).
N:
N: Maybe you didn't intend this upload to be a NMU, in that case, please
N: double-check that the most recent entry in the changelog is
N: byte-for-byte identical to the maintainer or one of the uploaders. If
N: this is a local package (not intended for Debian), you can suppress this
N: warning by putting "local" in the version number or "local package" on
N: the first line of the changelog entry.
N:
N: Refer to Debian Developer's Reference section 5.11.3 (Using the DELAYED/
N: queue) for details.
N:
N: Severity: normal, Certainty: certain
N:
N: Check: nmu, Type: source
N:
W: scap source: source-nmu-has-incorrect-version-number 3.3.0-1+0~20161001053440.128~1.gbpde16e9
N:
N: A source NMU should have a Debian
https://integration.wikimedia.org/ci/job/beta-build-deb/128/ for more details.

This revision was automatically updated to reflect the committed changes.
scap/arg.py
138

Turned out to be completely unnecessary. I replaced it with the much more sensible / idomatic

for w in words

scap/main.py
274

Yes but it isn't hurting anything to have it here as well..

scap/arg.py
129

I think it could happen with scap<tab><tab> but I haven't tested this to be sure.

Build is unstable

W: scap source: changelog-should-mention-nmu
N:
N: When you NMU a package, that fact should be mentioned on the first line
N: in the changelog entry. Use the words "NMU" or "Non-maintainer upload"
N: (case insensitive).
N:
N: Maybe you didn't intend this upload to be a NMU, in that case, please
N: double-check that the most recent entry in the changelog is
N: byte-for-byte identical to the maintainer or one of the uploaders. If
N: this is a local package (not intended for Debian), you can suppress this
N: warning by putting "local" in the version number or "local package" on
N: the first line of the changelog entry.
N:
N: Refer to Debian Developer's Reference section 5.11.3 (Using the DELAYED/
N: queue) for details.
N:
N: Severity: normal, Certainty: certain
N:
N: Check: nmu, Type: source
N:
W: scap source: source-nmu-has-incorrect-version-number 3.3.0-1+0~20161002045840.130~1.gbp0d76e5
N:
N: A source NMU should have a Debian