From eaf944f09e6c36c0d265b0088ea9239b8d9ebc74 Mon Sep 17 00:00:00 2001 From: Ilya Etingof Date: Wed, 28 Feb 2018 15:09:38 +0100 Subject: [PATCH] Migrate CLI to cliff This patch replaces the use of argparse module by the cliff package. The rationale is that cliff infrastructure is easier to use and more functional. Some other OpenStack projects seem to migrate to cliff already. Change-Id: Ib2185dad50e64de354222669d2e65f411ddc96a4 Closes-Bug: #1752911 --- requirements.txt | 2 +- setup.cfg | 8 + virtualbmc/cmd/vbmc.py | 222 +++++++++++++++---------- virtualbmc/tests/unit/cmd/test_vbmc.py | 99 +++++------ 4 files changed, 183 insertions(+), 148 deletions(-) diff --git a/requirements.txt b/requirements.txt index fe72308..21ef598 100644 --- a/requirements.txt +++ b/requirements.txt @@ -6,4 +6,4 @@ pbr!=2.1.0,>=2.0.0 # Apache-2.0 six>=1.10.0 # MIT libvirt-python>=3.5.0 # LGPLv2+ pyghmi>=1.0.22 # Apache-2.0 -PrettyTable<0.8,>=0.7.1 # BSD +cliff>=2.8.0,!=2.9.0 # Apache-2.0 diff --git a/setup.cfg b/setup.cfg index 04cf361..8563d25 100644 --- a/setup.cfg +++ b/setup.cfg @@ -26,6 +26,14 @@ packages = console_scripts = vbmc = virtualbmc.cmd.vbmc:main +virtualbmc = + add = virtualbmc.cmd.vbmc:AddCommand + delete = virtualbmc.cmd.vbmc:DeleteCommand + start = virtualbmc.cmd.vbmc:StartCommand + stop = virtualbmc.cmd.vbmc:StopCommand + list = virtualbmc.cmd.vbmc:ListCommand + show = virtualbmc.cmd.vbmc:ShowCommand + [build_sphinx] source-dir = doc/source build-dir = doc/build diff --git a/virtualbmc/cmd/vbmc.py b/virtualbmc/cmd/vbmc.py index eb24097..cc0e33b 100644 --- a/virtualbmc/cmd/vbmc.py +++ b/virtualbmc/cmd/vbmc.py @@ -10,146 +10,194 @@ # License for the specific language governing permissions and limitations # under the License. -from __future__ import print_function - -import argparse +import logging import sys -from prettytable import PrettyTable +from cliff.app import App +from cliff.command import Command +from cliff.commandmanager import CommandManager +from cliff.lister import Lister import virtualbmc from virtualbmc import exception from virtualbmc.manager import VirtualBMCManager -def main(): - parser = argparse.ArgumentParser( - prog='Virtual BMC', - description='A virtual BMC for controlling virtual instances', - ) - parser.add_argument('--version', action='version', - version=virtualbmc.__version__) - subparsers = parser.add_subparsers() +class AddCommand(Command): + """Create a new BMC for a virtual machine instance""" - # http://bugs.python.org/issue9253#msg186387 - subparsers.required = True - subparsers.dest = 'command' + def get_parser(self, prog_name): + parser = super(AddCommand, self).get_parser(prog_name) - # create the parser for the "add" command - parser_add = subparsers.add_parser('add', help='Add a new virtual BMC') - parser_add.add_argument('domain_name', + parser.add_argument('domain_name', help='The name of the virtual machine') - parser_add.add_argument('--username', + parser.add_argument('--username', dest='username', default='admin', help='The BMC username; defaults to "admin"') - parser_add.add_argument('--password', + parser.add_argument('--password', dest='password', default='password', help='The BMC password; defaults to "password"') - parser_add.add_argument('--port', + parser.add_argument('--port', dest='port', type=int, default=623, help='Port to listen on; defaults to 623') - parser_add.add_argument('--address', + parser.add_argument('--address', dest='address', default='::', help=('The address to bind to (IPv4 and IPv6 ' 'are supported); defaults to ::')) - parser_add.add_argument('--libvirt-uri', + parser.add_argument('--libvirt-uri', dest='libvirt_uri', default="qemu:///system", help=('The libvirt URI; defaults to ' '"qemu:///system"')) - parser_add.add_argument('--libvirt-sasl-username', + parser.add_argument('--libvirt-sasl-username', dest='libvirt_sasl_username', default=None, help=('The libvirt SASL username; defaults to ' 'None')) - parser_add.add_argument('--libvirt-sasl-password', + parser.add_argument('--libvirt-sasl-password', dest='libvirt_sasl_password', default=None, help=('The libvirt SASL password; defaults to ' 'None')) + return parser - # create the parser for the "delete" command - parser_delete = subparsers.add_parser('delete', - help='Delete a virtual BMC') - parser_delete.add_argument('domain_names', nargs='+', - help='A list of virtual machine names') + def take_action(self, args): - # create the parser for the "start" command - parser_start = subparsers.add_parser('start', help='Start a virtual BMC') - parser_start.add_argument('domain_name', - help='The name of the virtual machine') + log = logging.getLogger(__name__) - # create the parser for the "stop" command - parser_stop = subparsers.add_parser('stop', help='Stop a virtual BMC') - parser_stop.add_argument('domain_names', nargs='+', - help='A list of virtual machine names') + # Check if the username and password were given for SASL + sasl_user = args.libvirt_sasl_username + sasl_pass = args.libvirt_sasl_password + if any((sasl_user, sasl_pass)): + if not all((sasl_user, sasl_pass)): + msg = ("A password and username are required to use " + "Libvirt's SASL authentication") + log.error(msg) + raise exception.VirtualBMCError(msg) - # create the parser for the "list" command - subparsers.add_parser('list', help='list all virtual BMCs') + self.app.manager.add(username=args.username, password=args.password, + port=args.port, address=args.address, + domain_name=args.domain_name, + libvirt_uri=args.libvirt_uri, + libvirt_sasl_username=sasl_user, + libvirt_sasl_password=sasl_pass) - # create the parser for the "show" command - parser_show = subparsers.add_parser('show', help='Show a virtual BMC') - parser_show.add_argument('domain_name', - help='The name of the virtual machine') - args = parser.parse_args() +class DeleteCommand(Command): + """Delete a virtual BMC for a virtual machine instance""" - manager = VirtualBMCManager() + def get_parser(self, prog_name): + parser = super(DeleteCommand, self).get_parser(prog_name) - try: - if args.command == 'add': + parser.add_argument('domain_names', nargs='+', + help='A list of virtual machine names') - # Check if the username and password were given for SASL - sasl_user = args.libvirt_sasl_username - sasl_pass = args.libvirt_sasl_password - if any((sasl_user, sasl_pass)): - if not all((sasl_user, sasl_pass)): - print("A password and username are required to use " - "Libvirt's SASL authentication", file=sys.stderr) - sys.exit(1) + return parser - manager.add(username=args.username, password=args.password, - port=args.port, address=args.address, - domain_name=args.domain_name, - libvirt_uri=args.libvirt_uri, - libvirt_sasl_username=sasl_user, - libvirt_sasl_password=sasl_pass) + def take_action(self, args): + for domain in args.domain_names: + self.app.manager.delete(domain) - elif args.command == 'delete': - for domain in args.domain_names: - manager.delete(domain) - elif args.command == 'start': - manager.start(args.domain_name) +class StartCommand(Command): + """Start a virtual BMC for a virtual machine instance""" - elif args.command == 'stop': - for domain in args.domain_names: - manager.stop(domain) + def get_parser(self, prog_name): + parser = super(StartCommand, self).get_parser(prog_name) - elif args.command == 'list': - fields = ('Domain name', 'Status', 'Address', 'Port') - ptable = PrettyTable(fields) - for bmc in manager.list(): - ptable.add_row([bmc['domain_name'], bmc['status'], - bmc['address'], bmc['port']]) - print(ptable.get_string(sortby=fields[0])) + parser.add_argument('domain_name', + help='The name of the virtual machine') - elif args.command == 'show': - ptable = PrettyTable(['Property', 'Value']) - bmc = manager.show(args.domain_name) - for key, val in sorted(bmc.items()): - ptable.add_row([key, val]) - print(ptable.get_string()) + return parser - except exception.VirtualBMCError as e: - print(e, file=sys.stderr) - sys.exit(1) + def take_action(self, args): + self.app.manager.start(args.domain_name) + + +class StopCommand(Command): + """Stop a virtual BMC for a virtual machine instance""" + + def get_parser(self, prog_name): + parser = super(StopCommand, self).get_parser(prog_name) + + parser.add_argument('domain_names', nargs='+', + help='A list of virtual machine names') + + return parser + + def take_action(self, args): + for domain_name in args.domain_names: + self.app.manager.stop(domain_name) + + +class ListCommand(Lister): + """List all virtual BMC instances""" + + def take_action(self, args): + header = ('Domain name', 'Status', 'Address', 'Port') + rows = [] + + for bmc in self.app.manager.list(): + rows.append( + ([bmc['domain_name'], bmc['status'], + bmc['address'], bmc['port']]) + ) + + return header, sorted(rows) + + +class ShowCommand(Lister): + """Show virtual BMC properties""" + + def get_parser(self, prog_name): + parser = super(ShowCommand, self).get_parser(prog_name) + + parser.add_argument('domain_name', + help='The name of the virtual machine') + + return parser + + def take_action(self, args): + header = ('Property', 'Value') + rows = [] + + bmc = self.app.manager.show(args.domain_name) + + for key, val in bmc.items(): + rows.append((key, val)) + + return header, sorted(rows) + + +class VirtualBMCApp(App): + + def __init__(self): + super(VirtualBMCApp, self).__init__( + description='Virtual Baseboard Management Controller (BMC) backed ' + 'by virtual machines', + version=virtualbmc.__version__, + command_manager=CommandManager('virtualbmc'), + deferred_help=True, + ) + + def initialize_app(self, argv): + self.manager = VirtualBMCManager() + + def clean_up(self, cmd, result, err): + self.LOG.debug('clean_up %s', cmd.__class__.__name__) + if err: + self.LOG.debug('got an error: %s', err) + + +def main(argv=sys.argv[1:]): + vbmc_app = VirtualBMCApp() + return vbmc_app.run(argv) if __name__ == '__main__': - main() + sys.exit(main()) diff --git a/virtualbmc/tests/unit/cmd/test_vbmc.py b/virtualbmc/tests/unit/cmd/test_vbmc.py index 396f892..2f045a0 100644 --- a/virtualbmc/tests/unit/cmd/test_vbmc.py +++ b/virtualbmc/tests/unit/cmd/test_vbmc.py @@ -13,7 +13,6 @@ # License for the specific language governing permissions and limitations # under the License. -import argparse import six import sys @@ -26,7 +25,6 @@ from virtualbmc.tests.unit import utils as test_utils @mock.patch.object(sys, 'exit', lambda _: None) -@mock.patch.object(argparse, 'ArgumentParser') class VBMCTestCase(base.TestCase): def setUp(self): @@ -34,54 +32,39 @@ class VBMCTestCase(base.TestCase): self.domain = test_utils.get_domain() @mock.patch.object(manager.VirtualBMCManager, 'add') - def test_main_add(self, mock_add, mock_parser): - args = mock.Mock() - args.parse_args.return_value = mock.Mock(command='add', **self.domain) - mock_parser.return_value = args - vbmc.main() - - args.parse_args.assert_called_once_with() + def test_main_add(self, mock_add): + argv = ['add'] + for option, value in self.domain.items(): + if option != 'domain_name': + argv.append('--' + option.replace('_', '-')) + argv.append(value and str(value)) + argv.append(self.domain['domain_name']) + vbmc.main(argv) mock_add.assert_called_once_with(**self.domain) @mock.patch.object(manager.VirtualBMCManager, 'delete') - def test_main_delete(self, mock_delete, mock_parser): - args = mock.Mock() - args.parse_args.return_value = mock.Mock(command='delete', - domain_names=['foo', 'bar']) - mock_parser.return_value = args - vbmc.main() - - args.parse_args.assert_called_once_with() + def test_main_delete(self, mock_delete): + argv = ['delete', 'foo', 'bar'] + vbmc.main(argv) expected_calls = [mock.call('foo'), mock.call('bar')] self.assertEqual(expected_calls, mock_delete.call_args_list) @mock.patch.object(manager.VirtualBMCManager, 'start') - def test_main_start(self, mock_start, mock_parser): - args = mock.Mock() - args.parse_args.return_value = mock.Mock(command='start', - domain_name='SpongeBob') - mock_parser.return_value = args - vbmc.main() - args.parse_args.assert_called_once_with() + def test_main_start(self, mock_start): + argv = ['start', 'SpongeBob'] + vbmc.main(argv) mock_start.assert_called_once_with('SpongeBob') @mock.patch.object(manager.VirtualBMCManager, 'stop') - def test_main_stop(self, mock_stop, mock_parser): - args = mock.Mock() - args.parse_args.return_value = mock.Mock(command='stop', - domain_names=['foo', 'bar']) - mock_parser.return_value = args - vbmc.main() - - args.parse_args.assert_called_once_with() + def test_main_stop(self, mock_stop): + argv = ['stop', 'foo', 'bar'] + vbmc.main(argv) expected_calls = [mock.call('foo'), mock.call('bar')] self.assertEqual(expected_calls, mock_stop.call_args_list) @mock.patch.object(manager.VirtualBMCManager, 'list') - def test_main_list(self, mock_list, mock_parser): - args = mock.Mock() - args.parse_args.return_value = mock.Mock(command='list') - mock_parser.return_value = args + def test_main_list(self, mock_list): + argv = ['list'] mock_list.return_value = [ {'domain_name': 'node-1', @@ -94,49 +77,45 @@ class VBMCTestCase(base.TestCase): 'port': 123}] with mock.patch.object(sys, 'stdout', six.StringIO()) as output: - vbmc.main() + vbmc.main(argv) out = output.getvalue() expected_output = """\ +-------------+---------+---------+------+ -| Domain name | Status | Address | Port | +| Domain name | Status | Address | Port | +-------------+---------+---------+------+ -| node-0 | running | :: | 123 | -| node-1 | running | :: | 321 | +| node-0 | running | :: | 123 | +| node-1 | running | :: | 321 | +-------------+---------+---------+------+ """ self.assertEqual(expected_output, out) - args.parse_args.assert_called_once_with() - mock_list.assert_called_once_with() + self.assertEqual(mock_list.call_count, 1) @mock.patch.object(manager.VirtualBMCManager, 'show') - def test_main_show(self, mock_show, mock_parser): - args = mock.Mock() - args.parse_args.return_value = mock.Mock(command='show', - domain_name='SpongeBob') - mock_parser.return_value = args + def test_main_show(self, mock_show): + argv = ['show', 'SpongeBob'] + self.domain['status'] = 'running' mock_show.return_value = self.domain with mock.patch.object(sys, 'stdout', six.StringIO()) as output: - vbmc.main() + vbmc.main(argv) out = output.getvalue() expected_output = """\ +-----------------------+-----------+ -| Property | Value | +| Property | Value | +-----------------------+-----------+ -| address | :: | -| domain_name | SpongeBob | -| libvirt_sasl_password | None | -| libvirt_sasl_username | None | -| libvirt_uri | foo://bar | -| password | pass | -| port | 123 | -| status | running | -| username | admin | +| address | :: | +| domain_name | SpongeBob | +| libvirt_sasl_password | None | +| libvirt_sasl_username | None | +| libvirt_uri | foo://bar | +| password | pass | +| port | 123 | +| status | running | +| username | admin | +-----------------------+-----------+ """ self.assertEqual(expected_output, out) - args.parse_args.assert_called_once_with() - mock_show.assert_called_once_with('SpongeBob') + self.assertEqual(mock_show.call_count, 1)