copr / copr

Clone

#601 [frontend] support ?priority=x for non copr:// repo
Merged 5 years ago by praiskup. Opened 5 years ago by frostyx.
copr/ frostyx/copr external-repo-priority  into  master

file modified
+9 -4 
@@ -3,7 +3,7 @@ 
 

  import string
 

  

  from six import with_metaclass
 

- from six.moves.urllib.parse import urlparse, parse_qs
 

+ from six.moves.urllib.parse import urlparse, parse_qs, urlunparse, urlencode
 

  import re
 

  

  import flask
 
@@ -431,6 +431,8 @@ 
 

      Expands variables and sanitize repo url to be used for mock config
 

      """
 

      parsed_url = urlparse(repo_url)
 

+     query = parse_qs(parsed_url.query)
 

+ 

      if parsed_url.scheme == "copr":
 

          user = parsed_url.netloc
 

          prj = parsed_url.path.split("/")[1]
 
@@ -439,6 +441,12 @@ 
 

              "results", user, prj, chroot
 

          ]) + "/"
 

  

+     elif "priority" in query:
 

+         query.pop("priority")

can be even shortened to elif query.pop("priority"): :-) 

+         query_string = urlencode(query, doseq=True)
 

+         parsed_url = parsed_url._replace(query=query_string)
 

+         repo_url = urlunparse(parsed_url)
 

+ 

      repo_url = repo_url.replace("$chroot", chroot)
 

      repo_url = repo_url.replace("$distname", chroot.rsplit("-", 2)[0])
 

      return repo_url
 
@@ -451,9 +459,6 @@ 
 

      :return: dict of optional parameters parsed from the repo URL
 

      """
 

      supported_keys = supported_keys or ["priority"]
 

-     if not repo.startswith("copr://"):
 

-         return {}
 

- 

      params = {}
 

      qs = parse_qs(urlparse(repo).query)
 

      for k, v in qs.items():

file modified
+5 -1 
@@ -127,6 +127,10 @@ 
 

              ("copr://someuser/someproject", "http://backend/results/someuser/someproject/fedora-rawhide-x86_64/"),
 

              ("copr://someuser/someproject?foo=bar&baz=10",
 

               "http://backend/results/someuser/someproject/fedora-rawhide-x86_64/"),
 

+             ("http://example1.com/foo/$chroot?priority=10",
 

+              "http://example1.com/foo/fedora-rawhide-x86_64"),
 

+             ("http://example1.com/foo/$chroot?priority=10&foo=bar",
 

+              "http://example1.com/foo/fedora-rawhide-x86_64?foo=bar"),
 

          ]
 

          with app.app_context():
 

              for url, exp in test_cases:
 
@@ -137,7 +141,7 @@ 
 

              ("copr://foo/bar", {}),
 

              ("copr://foo/bar?priority=10", {"priority": 10}),
 

              ("copr://foo/bar?priority=10&unexp1=baz&unexp2=qux", {"priority": 10}),
 

-             ("http://example1.com/foo?priority=10", {}),
 

+             ("http://example1.com/foo?priority=10", {"priority": 10}),
 

          ]
 

          for repo, exp in test_cases:
 

              assert parse_repo_params(repo) == exp

This is an alternative to PR#539

Initially, I thought, that it will be enough to remove the
copr:// condition from parse_repo_params to get the
special params dict and simultaneously keep ?priority=x
in the repo baseurl. However, although baseurl with ?priority
can be opened in the browser, DNF can't handle it, so we need
to strip it.

Theoretically, false positives can occur, but IMHO it is
not a real threat.

Theoretically, false positives can occur, but IMHO it is
not a real treat.

If you think otherwise, I can update the code to work with ,priority=x syntax, that is proposed in PR#539, instead of the standard ?priority=x.

You can see, how the repo is generated in a build config

External repo in project settings:

https://rpm.nodesource.com/pub_10.x/fc/28/x86_64?priority=90

Build config:

 'repos': [...
           {'baseurl': 'https://rpm.nodesource.com/pub_10.x/fc/28/x86_64',
            'id': 'https_rpm_nodesource_com_pub_10_x_fc_28_x86_64_priority_90',
            'name': 'Additional repo https_rpm_nodesource_com_pub_10_x_fc_28_x86_64_priority_90',
            'priority': 90}
          ],

(baseurl doesn't contain the priority parameter; there is a priority key)

https://copr-be-dev.cloud.fedoraproject.org/results/frostyx/NonCoprRepoPriority/fedora-28-x86_64/00839438-hello/builder-live.log

+1

rebased onto d5361b95ecf8813851114cfc3ce19afa33b68469
5 years ago

Thinking again; what about to not parse->unparse if there's no priority keyword at all? Something like:

query = parse_qs(parsed_url.query)
priority = query.pop("priority", None)
if priority:
    query_string = urlencode(query, doseq=True)
    parsed_url = parsed_url._replace(query=query_string)
    repo_url = urlunparse(parsed_url)

(I also used urlencode instead of the manual re-construction)

rebased onto 52dfff82df38a7d0ee9b7f2e990fa572f098f579
5 years ago

Thinking again; what about to not parse->unparse if there's no priority keyword at all?

Sounds good,
I've updated the code little bit though, to avoid the nested if

(I also used urlencode instead of the manual re-construction)

Aha, thank you for the hint, urlencode is exactly what I was looking for, but couldn't find.

I've fixed the PR and rebased it.
Edited 5 years ago by frostyx

can be even shortened to elif query.pop("priority"): :-)

Thank you, +1 (shortened or not)

can be even shortened to elif query.pop("priority"): :-)

It can, I thought about that ... but that would modify the query before jumping to the elif body, which shouldn't be a problem in this case, but I worry, that it could cause some issues when someone will modify the function the future (maybe by adding another elif block or something)

ack

rebased onto fb0448e
5 years ago

Pull-Request has been merged by praiskup
5 years ago
Metadata
None
No Tags
Changes Summary 2
+9 -4
file changed
frontend/coprs_frontend/coprs/helpers.py
+5 -1
file changed
frontend/coprs_frontend/tests/test_helpers.py
Powered by Pagure 5.13.3
Documentation • File an Issue • About • SSH Hostkey/Fingerprint
© Red Hat, Inc. and others.