#999 Allow koji task URLs in download-task command
Closed 3 years ago by mikem. Opened 5 years ago by abitrolly.
abitrolly/koji downurl  into  master

file modified
+11 -4
@@ -6972,7 +6972,7 @@ 

  

  def anon_handle_download_task(options, session, args):

      "[download] Download the output of a build task"

-     usage = _("usage: %prog download-task <task_id>")

+     usage = _("usage: %prog download-task <task_id|task_url>")

      parser = OptionParser(usage=get_usage_str(usage))

      parser.add_option("--arch", dest="arches", metavar="ARCH", action="append", default=[],

                        help=_("Only download packages for this arch (may be used multiple times)"))
@@ -6989,11 +6989,18 @@ 

  

      (suboptions, args) = parser.parse_args(args)

      if len(args) == 0:

-         parser.error(_("Please specify a task ID"))

+         parser.error(_("Please specify a task ID or URL"))

      elif len(args) > 1:

-         parser.error(_("Only one task ID may be specified"))

+         parser.error(_("Only one task ID or URL may be specified"))

+ 

+     task_arg = args.pop()

+     if "?taskID=" in task_arg:

+       # extract base_task_id from the following URL structure

+       # https://koji.fedoraproject.org/koji/taskinfo?taskID=28483236

+       base_task_id = int(task_arg.split('=').pop())

+     else:

+       base_task_id = int(task_arg)

  

-     base_task_id = int(args.pop())

      if len(suboptions.arches) > 0:

          suboptions.arches = ",".join(suboptions.arches).split(",")

  

@@ -329,7 +329,7 @@ 

              anon_handle_download_task(self.options, self.session, args)

          self.assertExitCode(ex, 0)

          actual = self.stdout.getvalue()

-         expected = """Usage: %s download-task <task_id>

+         expected = """Usage: %s download-task <task_id|task_url>

  (Specify the --help global option for a list of other help options)

  

  Options:
@@ -359,12 +359,12 @@ 

          expected = ''

          self.assertMultiLineEqual(actual, expected)

          actual = self.stderr.getvalue()

-         expected = """Usage: %s download-task <task_id>

+         expected = """Usage: %s download-task <task_id|task_url>

  (Specify the --help global option for a list of other help options)

  

- %s: error: Please specify a task ID

+ %s: error: Please specify a task ID or URL

  """ % (progname, progname)

-         self.assertEqual(actual, expected)

+         self.assertMultiLineEqual(actual, expected)

  

      def test_handle_download_multi_task_id(self):

          args = ["123", "456"]
@@ -378,9 +378,9 @@ 

          expected = ''

          self.assertMultiLineEqual(actual, expected)

          actual = self.stderr.getvalue()

-         expected = """Usage: %s download-task <task_id>

+         expected = """Usage: %s download-task <task_id|task_url>

  (Specify the --help global option for a list of other help options)

  

- %s: error: Only one task ID may be specified

+ %s: error: Only one task ID or URL may be specified

  """ % (progname, progname)

-         self.assertEqual(actual, expected)

+         self.assertMultiLineEqual(actual, expected)

It is faster than selecting id part.

1 new commit added

  • Update tests for download-task
5 years ago

2 new commits added

  • Update tests for download-task
  • Allow koji task URLs in download-task command
5 years ago

You're assuming all koji instances are using https, I could see some using http behind a firewall

1 new commit added

  • Change URL check to pass http:// in addition to https://
5 years ago

What about using urlsplit + parse_qs? I'm thinking about potential changes in url (which is unlikely), but then you can be sure, that this is really taskID, not some other argument. regexp could be another solution.

I've also created issue #1004, so we've tracked it as a new feature. Please reference it in commit message (Fixes: https://pagure.io/koji/issue/1004)

I am not sure it is worth to spend too much time on this small change until there is another URL that download-task is unable to process. In ideal world it is dnf task to extract potential package info from any URL, validate that the package came from authoritative source (signed by koji at specified datetime) and install it.

Like I want to install testing packages from bodhi, scratches from koji etc. So that URL detection logic may end up in a separate function that could be pasted to dnf as-is, but it will require few more days for engineering, which I lack.

So how do we proceed? Is there enough code for a merger now? :D

Yes, @mikem is doing the merges and he should be online in the end of the week.

Will it confuse people that the actual remote server is not the same as the one in URL?

This feels like a questionable feature. Why is the url easier to copy/paste than the id? Can we address that instead?

This feels like a questionable feature. Why is the url easier to copy/paste than the id? Can we address that instead?

No answer. Where are folks typically copying the urls from that the id is unavailable?

This feels like a questionable feature. Why is the url easier to copy/paste than the id? Can we address that instead?

@mikem because Right-click + Copy Link Location is faster than Select + Right-click + Copy.

pretty please pagure-ci rebuild

3 years ago

pretty please pagure-ci rebuild

3 years ago

rebased onto 2b39879

3 years ago

Rebased to fix merge conflicts.

I still don't think this is a needed change.

While the patch in this PR appears to be relatively simple, it makes the handling of command line arguments inconsistent with other commands. It opens the door to much more significant changes in order to do the same thing for all command args that might also have an associated url, not just the ten or so commands that accept task ids but also build ids, rpm ids, tag ids, host ids, etc.

because Right-click + Copy Link Location is faster than Select + Right-click + Copy.

This is simply not enough justification for this change.

As far as I can tell, in all places where a task url shows, the task id also appears independently in a place that is easy to select. I can easily select the id with a double click and then copy/paste.

If there are places in the ui where the task id does not appear when it should, or is unduly difficult to select, we can look at changing that. However, we're not going to take this change.

Pull-Request has been closed by mikem

3 years ago