Page MenuHomePhabricator

Support sub-commands on cli.Application methods
ClosedPublic

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

Details

Maniphest Tasks
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
git checkout -b D349 && curl -L https://phabricator.wikimedia.org/D349?download=true | git apply
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
arcpatch-D349 (branched from master)
Lint
Lint Passed
Unit
Tests Passed
Build Status
Buildable 1067
Build 1539: build-beta-package.debJenkins
Build 1538: differential-jessieJenkins
Build 1537: 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.

thcipriani edited edge metadata.

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

What is this for?

106

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

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

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.

106

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...

mmodell edited edge metadata.

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.

mmodell edited edge metadata.

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
19

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

50

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>
57

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

58–68

Maybe use shlex.split here?

105

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

106

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

115

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)]
121

spaces

122

spaces

126

spaces

scap/cli.py
336–357

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

355

This is dark.

scap/main.py
274

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

thcipriani edited edge metadata.

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
mmodell edited edge metadata.
  • 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
115

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..

mmodell added inline comments.
scap/arg.py
106

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

Link to build: https://integration.wikimedia.org/ci/job/beta-build-deb/130/
See console output for more information: https://integration.wikimedia.org/ci/job/beta-build-deb/130/console