PR#2790: cli: list-builds sort-key warning - koji

koji

#2790 cli: list-builds sort-key warning

Merged 3 years ago by tkopecek. Opened 3 years ago by jcupova.

jcupova/koji issue2587 into master

cli: list-builds sort-key warning

Jana Cupova • 3 years ago

2c09339

cli/koji_cli/commands.py

file modified

+7 -1

		`@@ -3041,7 +3041,8 @@`
		`parser.add_option("--owner", help=_("List builds built by this owner"))`
		`parser.add_option("--volume", help=_("List builds by volume ID"))`
		`parser.add_option("-k", "--sort-key", action="append", metavar='FIELD',`
		`- default=[], help=_("Sort the list by the named field"))`
		`+ default=[], help=_("Sort the list by the named field. Allowed sort keys: "`
		`+ "nvr, owner_name, state"))`
		`parser.add_option("-r", "--reverse", action="store_true", default=False,`
		`help=_("Print the list in reverse order"))`
		`parser.add_option("--quiet", action="store_true", default=goptions.quiet,`
		`@@ -3126,6 +3127,11 @@`
		`parser.error(_("Filter must be provided for list"))`
		`if not options.sort_key:`
		`options.sort_key = ['nvr']`
		`+ else:`
		`+ for s_key in options.sort_key:`
		`+ if s_key not in ['nvr', 'owner_name', 'state']:`
		`+ warn("Invalid sort_key: %s." % s_key)`
		`+`
		`data = sorted(data, key=lambda b: [b.get(k) for k in options.sort_key],`
		`reverse=options.reverse)`
		`for build in data:`

tests/test_cli/test_list_builds.py

file modified

+189

		`@@ -10,10 +10,13 @@`

		`class TestListBuilds(utils.CliTestCase):`
		`def setUp(self):`
		`+ self.maxDiff = None`
		`self.options = mock.MagicMock()`
		`self.options.debug = False`
		`self.session = mock.MagicMock()`
		`self.session.getAPIVersion.return_value = koji.API_VERSION`
		`+ self.user_info = {'id': 1, 'name': 'kojiadmin', 'status': 0, 'usertype': 0,`
		`+ 'krb_principals': []}`

		`@mock.patch('sys.stderr', new_callable=StringIO)`
		`def test_list_builds_without_option(self, stderr):`
		`@@ -102,3 +105,189 @@`
		`['--build', build])`
		`self.assertExitCode(ex, 2)`
		`self.assert_console_message(stderr, expected)`
		`+`
		`+ @mock.patch('sys.stderr', new_callable=StringIO)`
		`+ @mock.patch('sys.stdout', new_callable=StringIO)`
		`+ def test_list_builds_invalid_key(self, stdout, stderr):`
		`+ list_build = [{'build_id': 1, 'epoch': 34, 'name': 'test-build',`
		`+ 'nvr': 'test-build-11-12', 'owner_name': 'kojiadmin',`
		`+ 'release': '12', 'state': 1, 'version': '11'},`
		`+ {'build_id': 4, 'epoch': 34, 'name': 'test-jx-build',`
		`+ 'nvr': 'test-jx-build-11-12', 'owner_name': 'kojiadmin',`
		`+ 'release': '12', 'state': 1, 'version': '11'},`
		`+ {'build_id': 2, 'epoch': 34, 'name': 'test-ax-build',`
		`+ 'nvr': 'test-ax-build-11-12', 'owner_name': 'kojiadmin',`
		`+ 'release': '12', 'state': 4, 'version': '11'},`
		`+ {'build_id': 3, 'epoch': 34, 'name': 'test-zx-build',`
		`+ 'nvr': 'test-zx-build-11-12', 'owner_name': 'kojiadmin',`
		`+ 'release': '12', 'state': 4, 'version': '11'}, ]`
		`+ test_key = 'test-key'`
		`+ expected_warn = "Invalid sort_key: %s." % test_key`
		`+ expected_output = """test-build-11-12 kojiadmin COMPLETE`
		`+ test-jx-build-11-12 kojiadmin COMPLETE`
		`+ test-ax-build-11-12 kojiadmin CANCELED`
		`+ test-zx-build-11-12 kojiadmin CANCELED`
		`+ """`
		`+ self.session.getUser.return_value = self.user_info`
		`+ self.session.listBuilds.return_value = list_build`
		`+ rv = anon_handle_list_builds(self.options, self.session, ['--owner', 'kojiadmin',`
		`+ '--sort-key', test_key])`
		`+ self.assertEqual(rv, None)`
		`+ self.assert_console_message(stdout, expected_output)`
		`+ self.assert_console_message(stderr, "%s\n" % expected_warn)`
		`+`
		`+ @mock.patch('sys.stdout', new_callable=StringIO)`
		`+ def test_list_builds_opt_owner_sorted_nvr(self, stdout):`
		`+ list_build = [{'build_id': 1, 'epoch': 34, 'name': 'test-build',`
		`+ 'nvr': 'test-build-11-12', 'owner_name': 'kojiadmin',`
		`+ 'release': '12', 'state': 1, 'version': '11'},`
		`+ {'build_id': 4, 'epoch': 34, 'name': 'test-jx-build',`
		`+ 'nvr': 'test-build-8-12', 'owner_name': 'kojiadmin',`
		`+ 'release': '12', 'state': 1, 'version': '8'},`
		`+ {'build_id': 2, 'epoch': 34, 'name': 'test-ax-build',`
		`+ 'nvr': 'test-build-11-9', 'owner_name': 'kojiadmin',`
		`+ 'release': '9', 'state': 4, 'version': '11'},`
		`+ {'build_id': 3, 'epoch': 34, 'name': 'test-zx-build',`
		`+ 'nvr': 'test-build-10-12', 'owner_name': 'kojiadmin',`
		`+ 'release': '12', 'state': 4, 'version': '10'}, ]`
		`+ expected_output = """test-build-10-12 kojiadmin CANCELED`
		`+ test-build-11-12 kojiadmin COMPLETE`
		`+ test-build-11-9 kojiadmin CANCELED`
		`+ test-build-8-12 kojiadmin COMPLETE`
		`+ """`
		`+ self.session.getUser.return_value = self.user_info`
		`+ self.session.listBuilds.return_value = list_build`
		`+ rv = anon_handle_list_builds(self.options, self.session, ['--owner', 'kojiadmin',`
		`+ '--sort-key', 'nvr'])`
		`+ self.assertEqual(rv, None)`
		`+ self.assert_console_message(stdout, expected_output)`
		`+`
		`+ @mock.patch('sys.stdout', new_callable=StringIO)`
		`+ def test_list_builds_opt_owner_sorted_state(self, stdout):`
		`+ list_build = [{'build_id': 1, 'epoch': 34, 'name': 'test-build',`
		`+ 'nvr': 'test-build-11-12', 'owner_name': 'kojiadmin',`
		`+ 'release': '12', 'state': 1, 'version': '11'},`
		`+ {'build_id': 4, 'epoch': 34, 'name': 'test-jx-build',`
		`+ 'nvr': 'test-build-8-12', 'owner_name': 'kojiadmin',`
		`+ 'release': '12', 'state': 1, 'version': '8'},`
		`+ {'build_id': 2, 'epoch': 34, 'name': 'test-ax-build',`
		`+ 'nvr': 'test-build-11-9', 'owner_name': 'kojiadmin',`
		`+ 'release': '9', 'state': 4, 'version': '11'},`
		`+ {'build_id': 3, 'epoch': 34, 'name': 'test-zx-build',`
		`+ 'nvr': 'test-build-10-12', 'owner_name': 'kojiadmin',`
		`+ 'release': '12', 'state': 4, 'version': '10'}, ]`
		`+ expected_output = """test-build-11-12 kojiadmin COMPLETE`
		`+ test-build-8-12 kojiadmin COMPLETE`
		`+ test-build-11-9 kojiadmin CANCELED`
		`+ test-build-10-12 kojiadmin CANCELED`
		`+ """`
		`+ self.session.getUser.return_value = self.user_info`
		`+ self.session.listBuilds.return_value = list_build`
		`+ rv = anon_handle_list_builds(self.options, self.session, ['--owner', 'kojiadmin',`
		`+ '--sort-key', 'state'])`
		`+ self.assertEqual(rv, None)`
		`+ self.assert_console_message(stdout, expected_output)`
		`+`
		`+ @mock.patch('sys.stdout', new_callable=StringIO)`
		`+ def test_list_builds_opt_owner_sorted_state_nvr(self, stdout):`
		`+ list_build = [{'build_id': 1, 'epoch': 34, 'name': 'test-build',`
		`+ 'nvr': 'test-build-11-12', 'owner_name': 'kojiadmin',`
		`+ 'release': '12', 'state': 1, 'version': '11'},`
		`+ {'build_id': 4, 'epoch': 34, 'name': 'test-jx-build',`
		`+ 'nvr': 'test-build-8-12', 'owner_name': 'kojiadmin',`
		`+ 'release': '12', 'state': 1, 'version': '8'},`
		`+ {'build_id': 2, 'epoch': 34, 'name': 'test-ax-build',`
		`+ 'nvr': 'test-build-11-9', 'owner_name': 'kojiadmin',`
		`+ 'release': '9', 'state': 4, 'version': '11'},`
		`+ {'build_id': 3, 'epoch': 34, 'name': 'test-zx-build',`
		`+ 'nvr': 'test-build-10-12', 'owner_name': 'kojiadmin',`
		`+ 'release': '12', 'state': 4, 'version': '10'}, ]`
		`+ expected_output = """test-build-11-12 kojiadmin COMPLETE`
		`+ test-build-8-12 kojiadmin COMPLETE`
		`+ test-build-10-12 kojiadmin CANCELED`
		`+ test-build-11-9 kojiadmin CANCELED`
		`+ """`
		`+ self.session.getUser.return_value = self.user_info`
		`+ self.session.listBuilds.return_value = list_build`
		`+ rv = anon_handle_list_builds(self.options, self.session, ['--owner', 'kojiadmin',`
		`+ '--sort-key', 'state',`
		`+ '--sort-key', 'nvr'])`
		`+ self.assertEqual(rv, None)`
		`+ self.assert_console_message(stdout, expected_output)`
		`+`
		`+ @mock.patch('sys.stdout', new_callable=StringIO)`
		`+ def test_list_builds_opt_prefix_sorted_owner(self, stdout):`
		`+ list_build = [{'build_id': 1, 'epoch': 34, 'name': 'test-build',`
		`+ 'nvr': 'test-build-11-12', 'owner_name': 'kojiadmin',`
		`+ 'release': '12', 'state': 1, 'version': '11'},`
		`+ {'build_id': 4, 'epoch': 34, 'name': 'test-jx-build',`
		`+ 'nvr': 'test-build-8-12', 'owner_name': 'kojitest',`
		`+ 'release': '12', 'state': 1, 'version': '8'},`
		`+ {'build_id': 2, 'epoch': 34, 'name': 'test-ax-build',`
		`+ 'nvr': 'test-build-11-9', 'owner_name': 'kojitest',`
		`+ 'release': '9', 'state': 4, 'version': '11'},`
		`+ {'build_id': 3, 'epoch': 34, 'name': 'test-zx-build',`
		`+ 'nvr': 'test-build-10-12', 'owner_name': 'kojiadmin',`
		`+ 'release': '12', 'state': 4, 'version': '10'}, ]`
		`+ expected_output = """test-build-11-12 kojiadmin COMPLETE`
		`+ test-build-10-12 kojiadmin CANCELED`
		`+ test-build-8-12 kojitest COMPLETE`
		`+ test-build-11-9 kojitest CANCELED`
		`+ """`
		`+ self.session.listBuilds.return_value = list_build`
		`+ rv = anon_handle_list_builds(self.options, self.session, ['--prefix', 'test-build',`
		`+ '--sort-key', 'owner_name'])`
		`+ self.assertEqual(rv, None)`
		`+ self.assert_console_message(stdout, expected_output)`
		`+`
		`+ @mock.patch('sys.stdout', new_callable=StringIO)`
		`+ def test_list_builds_opt_prefix_sorted_owner_nvr(self, stdout):`
		`+ list_build = [{'build_id': 1, 'epoch': 34, 'name': 'test-build',`
		`+ 'nvr': 'test-build-11-12', 'owner_name': 'kojiadmin',`
		`+ 'release': '12', 'state': 1, 'version': '11'},`
		`+ {'build_id': 4, 'epoch': 34, 'name': 'test-jx-build',`
		`+ 'nvr': 'test-build-8-12', 'owner_name': 'kojitest',`
		`+ 'release': '12', 'state': 1, 'version': '8'},`
		`+ {'build_id': 2, 'epoch': 34, 'name': 'test-ax-build',`
		`+ 'nvr': 'test-build-11-9', 'owner_name': 'kojitest',`
		`+ 'release': '9', 'state': 4, 'version': '11'},`
		`+ {'build_id': 3, 'epoch': 34, 'name': 'test-zx-build',`
		`+ 'nvr': 'test-build-10-12', 'owner_name': 'kojiadmin',`
		`+ 'release': '12', 'state': 4, 'version': '10'}, ]`
		`+ expected_output = """test-build-10-12 kojiadmin CANCELED`
		`+ test-build-11-12 kojiadmin COMPLETE`
		`+ test-build-11-9 kojitest CANCELED`
		`+ test-build-8-12 kojitest COMPLETE`
		`+ """`
		`+ self.session.listBuilds.return_value = list_build`
		`+ rv = anon_handle_list_builds(self.options, self.session, ['--prefix', 'test-build',`
		`+ '--sort-key', 'owner_name',`
		`+ '--sort-key', 'nvr'])`
		`+ self.assertEqual(rv, None)`
		`+ self.assert_console_message(stdout, expected_output)`
		`+`
		`+ @mock.patch('sys.stdout', new_callable=StringIO)`
		`+ def test_list_builds_opt_owner_reverse(self, stdout):`
		`+ list_build = [{'build_id': 1, 'epoch': 34, 'name': 'test-build',`
		`+ 'nvr': 'test-build-11-12', 'owner_name': 'kojiadmin',`
		`+ 'release': '12', 'state': 1, 'version': '11'},`
		`+ {'build_id': 4, 'epoch': 34, 'name': 'test-jx-build',`
		`+ 'nvr': 'test-build-8-12', 'owner_name': 'kojiadmin',`
		`+ 'release': '12', 'state': 1, 'version': '8'},`
		`+ {'build_id': 2, 'epoch': 34, 'name': 'test-ax-build',`
		`+ 'nvr': 'test-build-11-9', 'owner_name': 'kojiadmin',`
		`+ 'release': '9', 'state': 4, 'version': '11'},`
		`+ {'build_id': 3, 'epoch': 34, 'name': 'test-zx-build',`
		`+ 'nvr': 'test-build-10-12', 'owner_name': 'kojiadmin',`
		`+ 'release': '12', 'state': 4, 'version': '10'}, ]`
		`+ expected_output = """test-build-8-12 kojiadmin COMPLETE`
		`+ test-build-11-9 kojiadmin CANCELED`
		`+ test-build-11-12 kojiadmin COMPLETE`
		`+ test-build-10-12 kojiadmin CANCELED`
		`+ """`
		`+ self.session.getUser.return_value = self.user_info`
		`+ self.session.listBuilds.return_value = list_build`
		`+ rv = anon_handle_list_builds(self.options, self.session, ['--owner', 'kojiadmin',`
		`+ '--reverse'])`
		`+ self.assertEqual(rv, None)`
		`+ self.assert_console_message(stdout, expected_output)`

jcupova commented 3 years ago

Fixes: https://pagure.io/koji/issue/2587
Fixes: https://pagure.io/koji/issue/2588

rebased onto 114df308dc298e322eace7406fdf6aeb30745661

3 years ago

rebased onto 11fca9225c43ec9969b90bf8f004cef96d3b8eb0

3 years ago

tkopecek commented on line 18 of cli/koji_cli/commands.py 3 years ago

Second part is not true - in that case it still will be sorted by 'something' but not necessarily by 'nvr'. (e.g. -k name -k owner_name will lead to ['name', 'owner_name']

rebased onto 38d13c4644237155c9e2e9471270ec115549e705

3 years ago

jcupova commented 3 years ago

@tkopecek fixed

rebased onto 2c09339

3 years ago