PR#3311: New feature: command download-scratch-build is added - koji

koji

#3311 New feature: command download-scratch-build is added

Closed 2 years ago by geoargyr. Opened 3 years ago by geoargyr.

geoargyr/koji downlscratch into master

New feature: command download-scratch-build added

geargyri • 3 years ago

0de341a

cli/koji

file modified

+1 -1

		`@@ -103,7 +103,7 @@`
		`def get_options():`
		`"""process options from command line and config file"""`

		`- common_commands = ['build', 'help', 'download-build',`
		`+ common_commands = ['build', 'help', 'download-build', 'download-scratch-build',`
		`'latest-build', 'search', 'list-targets']`
		`usage = "%%prog [global-options] command [command-options-and-arguments]\n\n" \`
		`"Common commands: %s" % ', '.join(sorted(common_commands))`

cli/koji_cli/commands.py

file modified

+69

		`@@ -7147,6 +7147,75 @@`
		`quiet=suboptions.quiet, noprogress=suboptions.noprogress)`


		`+ def anon_handle_download_scratch_build(options, session, args):`
		`+ # Read, filter and check arguments`
		`+ "[download] Download a scratch built package"`
		`+ usage = "usage: %prog download-scratch-build [options] <task-id>"`
		`+ parser = OptionParser(usage=get_usage_str(usage))`
		`+ parser.add_option("--logs", dest="logs", action="store_true", default=False,`
		`+ help="Also download build logs")`
		`+ parser.add_option("--topurl", metavar="URL", default=options.topurl,`
tkopecek commented 3 years ago `--topurl` is already toplevel option (see `koji --help`, so it is a duplicate here).
		`+ help="URL under which Koji files are accessible")`
		`+ parser.add_option("--noprogress", action="store_true",`
		`+ help="Do not display progress meter")`
		`+ parser.add_option("-q", "--quiet", action="store_true",`
		`+ help="Suppress output", default=options.quiet)`
		`+ (suboptions, args) = parser.parse_args(args)`
		`+`
		`+ if len(args) < 1:`
		`+ parser.error("Please specify a valid task id")`
		`+ elif len(args) > 1:`
		`+ parser.error("Only a single task id may be specified")`
		`+ try:`
		`+ taskid = int(args[0])`
		`+ except ValueError:`
		`+ error("The argument %r cannot be a task-id number" % args[0])`
		`+ if not suboptions.topurl:`
		`+ error("You must specify --topurl to download files")`
		`+`
		`+ # Connect, check the id and download the builds`
		`+ ensure_connection(session, options)`
		`+`
		`+ def check_taskid(taskid):`
		`+ taskinfo = session.getTaskInfo(taskid)`
		`+ taskchildren = session.getTaskChildren(taskid)`
		`+ if not taskinfo["parent"] and taskinfo["method"] == "image" and len(taskchildren) == 1:`
		`+ # taskid belongs to a scratch image build parent task`
		`+ taskid_upd = taskchildren[0]['id']`
		`+ return taskid_upd`
		`+ else:`
		`+ error("No such scratch image build")`
		`+`
		`+ taskinfo = session.getTaskInfo(taskid)`
		`+ taskchildren = session.getTaskChildren(taskid)`
		`+ if taskinfo["method"] == "createImage" and len(taskchildren) == 0:`
		`+ # taskid belongs to a child image task`
		`+ taskid_upd = check_taskid(taskinfo["parent"])`
		`+ else:`
		`+ taskid_upd = check_taskid(taskid)`
		`+`
		`+ files = list_task_output_all_volumes(session, taskid_upd)`
		`+ if not files:`
		`+ error("No files were found for taskid %i" %taskid_upd)`
		`+`
		`+ pi = koji.PathInfo(topdir=suboptions.topurl)`
		`+ urls = []`
		`+ for filename in files:`
		`+ if suboptions.logs:`
		`+ for volume in files[filename]:`
		`+ url = os.path.join(pi.task(taskid_upd), filename)`
		`+ urls.append(url)`
		`+ else:`
		`+ if not filename.endswith('.log'):`
		`+ for volume in files[filename]:`
		`+ url = os.path.join(pi.task(taskid_upd), filename)`
		`+ urls.append(url)`
		`+`
		`+ for url in urls:`
		`+ download_file(url, os.path.basename(url), quiet=suboptions.quiet,`
		`+ noprogress=suboptions.noprogress, filesize=None)`
		`+`
		`+`
		`def anon_handle_download_logs(options, session, args):`
		`"[download] Download logs for task"`

tests/test_cli/data/list-commands.txt

file modified

		`@@ -93,6 +93,7 @@`
		`download commands:`
		`download-build Download a built package`
		`download-logs Download logs for task`
		`+ download-scratch-build Download a scratch built package`
		`download-task Download the output of a build task`

		`info commands:`

tests/test_cli/test_download_scratch_build.py

file added

+204

		`@@ -0,0 +1,204 @@`
		`+ from __future__ import absolute_import`
		`+`
		`+ import mock`
		`+ from mock import call`
		`+ import six`
		`+ from six.moves import StringIO`
		`+ import koji`
		`+ from koji_cli.commands import anon_handle_download_scratch_build`
		`+ from . import utils`
		`+`
		`+`
		`+ class TestDownloadScratchBuild(utils.CliTestCase):`
		`+ # Show long diffs in error output...`
		`+ maxDiff = None`
		`+`
		`+ def setUp(self):`
		`+ self.options = mock.MagicMock()`
		`+ self.options.quiet = False`
		`+ self.options.topurl = 'https://topurl'`
		`+ self.session = mock.MagicMock()`
		`+ self.session.getAPIVersion.return_value = koji.API_VERSION`
		`+ self.stdout = mock.patch('sys.stdout', new_callable=six.StringIO).start()`
		`+ self.ensure_connection = mock.patch('koji_cli.commands.ensure_connection').start()`
		`+ self.download_file = mock.patch('koji_cli.commands.download_file').start()`
		`+ self.list_task_output_all_volumes = mock.patch(`
		`+ 'koji_cli.commands.list_task_output_all_volumes').start()`
		`+`
		`+`
		`+ def test_download_scratch_build(self):`
		`+ task_id = 123333`
		`+ args = [str(task_id)]`
		`+ self.session.getTaskInfo.return_value = {'id': task_id,`
		`+ 'parent': None,`
		`+ 'method': 'image'}`
		`+ self.session.getTaskChildren.return_value = [{'id': 22222,`
		`+ 'parent': 123333,`
		`+ 'method': 'createImage',`
		`+ 'arch': 'noarch',`
		`+ 'state': 2}]`
		`+ self.list_task_output_all_volumes.return_value = {`
		`+ 'ks-name_koji-tag.ks': ['DEFAULT'],`
		`+ 'oz-taskarch.log': ['DEFAULT'],`
		`+ 'ks-name.ks': ['DEFAULT'],`
		`+ 'tdl-taskarch.xml': ['DEFAULT'],`
		`+ 'build-name_date_release.tar.xz': ['DEFAULT']}`
		`+`
		`+ rv = anon_handle_download_scratch_build(self.options, self.session, args)`
		`+ actual = self.stdout.getvalue()`
		`+ expected = ''`
		`+ self.assertMultiLineEqual(actual, expected)`
		`+ self.ensure_connection.assert_called_once_with(self.session, self.options)`
		`+ self.assertEqual(self.list_task_output_all_volumes.mock_calls, [call(self.session, 22222)])`
		`+ self.assertListEqual(self.download_file.mock_calls, [`
		`+ call('https://topurl/work/tasks/2222/22222/ks-name_koji-tag.ks',`
		`+ 'ks-name_koji-tag.ks', filesize=None, quiet=False, noprogress=None),`
		`+ call('https://topurl/work/tasks/2222/22222/ks-name.ks',`
		`+ 'ks-name.ks', filesize=None, quiet=False, noprogress=None),`
		`+ call('https://topurl/work/tasks/2222/22222/tdl-taskarch.xml',`
		`+ 'tdl-taskarch.xml', filesize=None, quiet=False, noprogress=None),`
		`+ call('https://topurl/work/tasks/2222/22222/build-name_date_release.tar.xz',`
		`+ 'build-name_date_release.tar.xz', filesize=None, quiet=False, noprogress=None)])`
		`+ self.assertIsNone(rv)`
		`+`
		`+`
		`+ def test_download_scratch_build_wlogs(self):`
		`+ task_id = 123333`
		`+ args = [str(task_id), "--logs"]`
		`+ self.session.getTaskInfo.return_value = {'id': task_id,`
		`+ 'parent': None,`
		`+ 'method': 'image'}`
		`+ self.session.getTaskChildren.return_value = [{'id': 22222,`
		`+ 'parent': 123333,`
		`+ 'method': 'createImage',`
		`+ 'arch': 'noarch',`
		`+ 'state': 2}]`
		`+ self.list_task_output_all_volumes.return_value = {`
		`+ 'ks-name_koji-tag.ks': ['DEFAULT'],`
		`+ 'oz-taskarch.log': ['DEFAULT'],`
		`+ 'ks-name.ks': ['DEFAULT'],`
		`+ 'tdl-taskarch.xml': ['DEFAULT'],`
		`+ 'build-name_date_release.tar.xz': ['DEFAULT']}`
		`+`
		`+ rv = anon_handle_download_scratch_build(self.options, self.session, args)`
		`+ actual = self.stdout.getvalue()`
		`+ expected = ''`
		`+ self.assertMultiLineEqual(actual, expected)`
		`+ self.ensure_connection.assert_called_once_with(self.session, self.options)`
		`+ self.assertEqual(self.list_task_output_all_volumes.mock_calls, [call(self.session, 22222)])`
		`+ self.assertListEqual(self.download_file.mock_calls, [`
		`+ call('https://topurl/work/tasks/2222/22222/ks-name_koji-tag.ks',`
		`+ 'ks-name_koji-tag.ks', filesize=None, quiet=False, noprogress=None),`
		`+ call('https://topurl/work/tasks/2222/22222/oz-taskarch.log',`
		`+ 'oz-taskarch.log', filesize=None, quiet=False, noprogress=None),`
		`+ call('https://topurl/work/tasks/2222/22222/ks-name.ks',`
		`+ 'ks-name.ks', filesize=None, quiet=False, noprogress=None),`
		`+ call('https://topurl/work/tasks/2222/22222/tdl-taskarch.xml',`
		`+ 'tdl-taskarch.xml', filesize=None, quiet=False, noprogress=None),`
		`+ call('https://topurl/work/tasks/2222/22222/build-name_date_release.tar.xz',`
		`+ 'build-name_date_release.tar.xz', filesize=None, quiet=False, noprogress=None)])`
		`+ self.assertIsNone(rv)`
		`+`
		`+`
		`+ @mock.patch('sys.stderr', new_callable=StringIO)`
		`+ def test_download_scratch_build_nonscratch_img_id(self, stderr):`
		`+ task_id = 123333`
		`+ args = [str(task_id)]`
		`+ self.session.getTaskInfo.return_value = {'id': task_id,`
		`+ 'parent': None,`
		`+ 'method': 'image'}`
		`+ self.session.getTaskChildren.return_value = [{'id': 22222,`
		`+ 'parent': 123333,`
		`+ 'method': 'createImage',`
		`+ 'arch': 'noarch',`
		`+ 'state': 2},`
		`+ {'id': 33333,`
		`+ 'parent': 123333,`
		`+ 'method': 'createImage',`
		`+ 'arch': 'noarch',`
		`+ 'state': 2}]`
		`+ expected = "No such scratch image build\n"`
		`+ with self.assertRaises(SystemExit) as ex:`
		`+ anon_handle_download_scratch_build(self.options, self.session, args)`
		`+ self.assertExitCode(ex, 1)`
		`+ self.assert_console_message(stderr, expected)`
		`+`
		`+`
		`+ @mock.patch('sys.stderr', new_callable=StringIO)`
		`+ def test_download_scratch_build_rpm_id(self, stderr):`
		`+ task_id = 123333`
		`+ args = [str(task_id)]`
		`+ self.session.getTaskInfo.return_value = {'id': task_id,`
		`+ 'parent': None,`
		`+ 'method': 'build'}`
		`+ self.session.getTaskChildren.return_value = [{'id': 22222,`
		`+ 'parent': 123333,`
		`+ 'method': 'rebuildSRPM',`
		`+ 'arch': 'noarch',`
		`+ 'state': 2},`
		`+ {'id': 33333,`
		`+ 'parent': 123333,`
		`+ 'method': 'rebuildSRPM',`
		`+ 'arch': 'noarch',`
		`+ 'state': 2}]`
		`+ expected = "No such scratch image build\n"`
		`+ with self.assertRaises(SystemExit) as ex:`
		`+ anon_handle_download_scratch_build(self.options, self.session, args)`
		`+ self.assertExitCode(ex, 1)`
		`+ self.assert_console_message(stderr, expected)`
		`+`
		`+`
		`+ @mock.patch('sys.stderr', new_callable=StringIO)`
		`+ def test_download_scratch_build_without_option(self, stderr):`
		`+ expected = "Usage: %s download-scratch-build [options] <task-id>\n" \`
		`+ "(Specify the --help global option for a list of other help options)\n\n" \`
		`+ "%s: error: Please specify a valid task id\n" \`
		`+ % (self.progname, self.progname)`
		`+ with self.assertRaises(SystemExit) as ex:`
		`+ anon_handle_download_scratch_build(self.options, self.session, [])`
		`+ self.assertExitCode(ex, 2)`
		`+ self.assert_console_message(stderr, expected)`
		`+`
		`+`
		`+ @mock.patch('sys.stderr', new_callable=StringIO)`
		`+ def test_download_scratch_build_invalid_option(self, stderr):`
		`+ invalid_option = "random_string_arg"`
		`+ expected = "The argument '%s' cannot be a task-id number\n" % invalid_option`
		`+ with self.assertRaises(SystemExit) as ex:`
		`+ anon_handle_download_scratch_build(self.options, self.session, [invalid_option])`
		`+ self.assertExitCode(ex, 1)`
		`+ self.assert_console_message(stderr, expected)`
		`+`
		`+`
		`+ @mock.patch('sys.stderr', new_callable=StringIO)`
		`+ def test_download_scratch_build_no_files(self, stderr):`
		`+ task_id = '1'`
		`+ expected = "No files were found for taskid %s\n" % task_id`
		`+ self.session.getTaskInfo.return_value = {'id': task_id,`
		`+ 'parent': None,`
		`+ 'method': 'image'}`
		`+ self.session.getTaskChildren.return_value = [{'id': 1,`
		`+ 'parent': 123333,`
		`+ 'method': 'createImage',`
		`+ 'arch': 'noarch',`
		`+ 'state': 2}]`
		`+ self.list_task_output_all_volumes.return_value = {}`
		`+ with self.assertRaises(SystemExit) as ex:`
		`+ anon_handle_download_scratch_build(self.options, self.session, [task_id])`
		`+ self.assertExitCode(ex, 1)`
		`+ self.assert_console_message(stderr, expected)`
		`+`
		`+`
		`+ def test_handle_add_volume_help(self):`
		`+ self.assert_help(`
		`+ anon_handle_download_scratch_build,`
		`+ """Usage: %s download-scratch-build [options] <task-id>`
		`+ (Specify the --help global option for a list of other help options)`
		`+`
		`+ Options:`
		`+ -h, --help show this help message and exit`
		`+ --logs Also download build logs`
		`+ --topurl=URL URL under which Koji files are accessible`
		`+ --noprogress Do not display progress meter`
		`+ -q, --quiet Suppress output`
		`+ """ % self.progname)`

geoargyr commented 3 years ago

anon_handle_download_scratch_build() method implements the command
test_download_scratch_build.py holds all testing for the new method
Updates for supporting the new command

geoargyr commented 3 years ago

This Pull Request is a possible solution to the following issue: https://pagure.io/koji/issue/3182

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

--topurl is already toplevel option (see koji --help, so it is a duplicate here).

geoargyr commented 3 years ago

--topurl is already toplevel option (see koji --help, so it is a duplicate here).

The anon_handle_download_scratch_build() function is implemented like functions anon_handle_download_build() and anon_handle_download_task().
They also have a --topurl option. Why is it considered only now duplicate?

tkopecek commented 3 years ago

hmm, you're right. We've to cleanup systematically in the future :-( So, ignore that comment.
Anyway, it is now working only for images. If we want to introduce new command it should be also able to download (at least) rpm scratch builds.

$ koji -p fedora download-scratch-build 85288881
No such scratch image build

jcupova commented 2 years ago

@mikem I spoked with @tkopecek and current idea it to extend this PR for all builds where is --scratch option. What do you think? Are you agree with this idea?

mikem commented 2 years ago

As-is, this code doesn't really download from the scratch location. It downloads the task output paths, as returned by session.listTaskOutput(task_id, all_volumes=True). This is basically the same thing that download-task does.

Having a more convenient way to download scratch builds seems reasonable, but as Tomas rightly points out, it should handle all kinds of scratch builds.

Also, I'm not sure this needs its own command. Supporting a --scratch option in download-build would probably make more sense.

jcupova commented 2 years ago

@mikem sure, I make sense to use download-build with --scratch option. So ok, I'll create new PR with updated download-build and use --scratch option for all kinds of scratch builds. Thanks Mike.

mikem commented 2 years ago

Actually, the best way to address the original issue, #3182, would be to fix the download-task command

mikem commented 2 years ago

Maybe we can get a separate issue filed for supporting download-build --scratch, as this is quite a different from #3182

geoargyr commented 2 years ago

Hello all! As I worked on this issue/feature for some time, I'd like to say my opinion hoping that it will be helpful for you.
When I started working on this, I checked the other commands and realized that download-task downloads non-scratch rpms and scratch rpms, while the download-build downloads non-scratch images and non-scratch rpms. So, for some reason the scratch images are left outside, while the non-scratch rpms can be served in two ways!
As a user, it would make much more sense if I had a --scratch option in download-build, but as a developer I complied with the initial proposal (as stated by tkopecek here: https://pagure.io/koji/issue/3182) and created a new download-scratch-build. Nevertheless, -thinking again as a developer- it will be an overhead to implement a --scratch option in download-build, as its functionality is based on the buildID and the scratch image builds do not have a buildID (as far as I know).
Also, as a developer, based on the fact that scratch image builds have no buildID, I based the functionality of anon_handle_download_scratch_build() on anon_handle_download_task(), so extending the download-task command in a way that downloads scratch image builds seems more reasonable from the developer point of view.