PR#3542: cli: fix nvr sorting in list-builds - koji

koji

#3542 cli: fix nvr sorting in list-builds

Merged a year ago by tkopecek. Opened 2 years ago by tkopecek.

tkopecek/koji issue3416 into master

cli: drop nvr sorting from list-builds

Tomas Kopecek • a year ago

e3c5914

cli/koji_cli/commands.py

file modified

+3 -3

		`@@ -3301,7 +3301,7 @@`
		`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. Allowed sort keys: "`
		`- "nvr, owner_name, state")`
		`+ "build_id, 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,`
		`@@ -3390,10 +3390,10 @@`
		`else:`
		`parser.error("Filter must be provided for list")`
		`if not options.sort_key:`
		`- options.sort_key = ['nvr']`
		`+ options.sort_key = ['build_id']`
		`else:`
		`for s_key in options.sort_key:`
		`- if s_key not in ['nvr', 'owner_name', 'state']:`
		`+ if s_key not in ['build_id', '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],`

tests/test_cli/test_list_builds.py

file modified

+12 -12

		`@@ -195,9 +195,9 @@`

		`@mock.patch('sys.stdout', new_callable=StringIO)`
		`def test_list_builds_opt_owner_sorted_nvr(self, stdout):`
		`- expected_output = """build-test-1-12 kojiadmin CANCELED`
		`- test-build-11-12 kojiadmin COMPLETE`
		`+ expected_output = """test-build-11-12 kojiadmin COMPLETE`
		`test-build-8-12 kojiadmin DELETED`
		`+ build-test-1-12 kojiadmin CANCELED`
		`"""`
		`self.session.getUser.return_value = self.user_info`
		`self.session.listBuilds.return_value = [self.list_build[0], self.list_build[1],`
		`@@ -261,9 +261,9 @@`
		`def test_list_builds_opt_prefix_sorted_owner(self, stdout):`
		`expected_output = """test-build-11-12 kojiadmin COMPLETE`
		`test-build-8-12 kojiadmin DELETED`
		`- build-test-1-12 kojiadmin CANCELED`
		`test-build-11-9 kojitest COMPLETE`
		`test-build-10-12 kojitest CANCELED`
		`+ build-test-1-12 kojiadmin CANCELED`
		`"""`
		`self.session.listBuilds.return_value = self.list_build`
		`rv = anon_handle_list_builds(self.options, self.session, ['--prefix', 'test-build',`
		`@@ -280,11 +280,11 @@`

		`@mock.patch('sys.stdout', new_callable=StringIO)`
		`def test_list_builds_opt_prefix_sorted_owner_nvr(self, stdout):`
		`- expected_output = """build-test-1-12 kojiadmin CANCELED`
		`- test-build-11-12 kojiadmin COMPLETE`
		`+ expected_output = """test-build-11-12 kojiadmin COMPLETE`
		`test-build-8-12 kojiadmin DELETED`
		`- test-build-10-12 kojitest CANCELED`
		`test-build-11-9 kojitest COMPLETE`
		`+ test-build-10-12 kojitest CANCELED`
		`+ build-test-1-12 kojiadmin CANCELED`
		`"""`
		`self.session.listBuilds.return_value = self.list_build`
		`rv = anon_handle_list_builds(self.options, self.session, ['--prefix', 'test-build',`
		`@@ -302,9 +302,9 @@`

		`@mock.patch('sys.stdout', new_callable=StringIO)`
		`def test_list_builds_opt_owner_reverse(self, stdout):`
		`- expected_output = """test-build-8-12 kojiadmin DELETED`
		`+ expected_output = """build-test-1-12 kojiadmin CANCELED`
		`+ test-build-8-12 kojiadmin DELETED`
		`test-build-11-12 kojiadmin COMPLETE`
		`- build-test-1-12 kojiadmin CANCELED`
		`"""`
		`self.session.getUser.return_value = self.user_info`
		`self.session.listBuilds.return_value = [self.list_build[0], self.list_build[1],`
		`@@ -376,8 +376,8 @@`
		`@mock.patch('sys.stdout', new_callable=StringIO)`
		`def test_list_builds_volume_not_int(self, stdout):`
		`volume = 'test-volume'`
		`- expected_output = """build-test-1-12 kojiadmin CANCELED`
		`- test-build-11-12 kojiadmin COMPLETE`
		`+ expected_output = """test-build-11-12 kojiadmin COMPLETE`
		`+ build-test-1-12 kojiadmin CANCELED`
		`"""`
		`self.session.listBuilds.return_value = [self.list_build[0], self.list_build[4]]`
		`self.session.listVolumes.return_value = [{'id': 0, 'name': 'DEFAULT'},`
		`@@ -396,8 +396,8 @@`
		`@mock.patch('sys.stdout', new_callable=StringIO)`
		`def test_list_builds_volume_int(self, stdout):`
		`volume = 1`
		`- expected_output = """build-test-1-12 kojiadmin CANCELED`
		`- test-build-11-12 kojiadmin COMPLETE`
		`+ expected_output = """test-build-11-12 kojiadmin COMPLETE`
		`+ build-test-1-12 kojiadmin CANCELED`
		`"""`
		`self.session.listBuilds.return_value = [self.list_build[0], self.list_build[4]]`
		`rv = anon_handle_list_builds(self.options, self.session, ['--volume', volume])`

tkopecek commented 2 years ago

mikem commented 2 years ago

I am concerned that offering this feature will lead to even more confusion about how Koji orders builds.

mikem commented 2 years ago

It appears that labelCompare support for EVR strings was added in 4.16.0 and probably isn't available on all our clients.

https://rpm.org/wiki/Releases/4.16.0

mikem commented 2 years ago

Most sensible default ordering for a general build query is probably by build id (as in the web ui), but unfortunately the behavior here has been different since it was added in #526

tkopecek commented 2 years ago

So, are you more for dropping nvr option?

mikem commented 2 years ago

I think it might be safest to not support rpm ordering, particularly since our nvrs might not be rpms.

Also I think we should change the default ordering to build_id. This is well-defined, easy to calculate, and matches the web ui.

rebased onto 7cf367a15759780a2bff2823a1765d7b765935d2

a year ago

rebased onto e3c5914

a year ago

mikem commented a year ago

I guess my only concern is returning an error for a command that used to work (i.e. -k nvr). I don't want to support rpm ordering here, but I don't know if we refuse to sort by nvr (as a string) as we have before.

Also a bit of a tangent, but it's a little strange that we have such a limited set of sorting options here, and that we do the sorting client side, when the call this is based on allows sorting on any field forward or reverse. But I guess we don't have to address all that here.

tkopecek commented a year ago

It is printing a warning but still works for -k nvr. Also created #3570 for server-side solution.

mikem commented a year ago

bq. It is printing a warning but still works

Ah yes, my mistake. That's fine then :thumbsup:

Metadata Update from @tkopecek:
- Pull-request tagged with: testing-ready

a year ago