#4 Various improvements and fixes, most for tagging packages
Merged 4 years ago by asaleh. Opened 4 years ago by nphilipp.
fedora-infra/ nphilipp/rpmautospec master--tag-epoch-and-misc  into  master

file modified
+124 -26
@@ -2,21 +2,102 @@ 

  

  import logging

  import os

+ import re

  import subprocess

+ from urllib import parse

  

  from .misc import get_package_builds, koji_init

  

  

  _log = logging.getLogger(__name__)

- escape_chars = {

-     "^": "%5E",

-     "%": "%25",

-     "~": "%7E",

- }

+ 

+ # See https://git-scm.com/docs/git-check-ref-format

+ git_tag_seqs_to_escape = [

+     "~",

+     "/",

+     re.compile(r"^\."),

+     re.compile(r"(\.)lock$"),

+     re.compile(r"\.\.+"),

+     re.compile(r"\.$"),

+     # This is a no-op, original "{" get quoted anyway.

+     # re.compile(r"(@)\{"),

+     re.compile(r"^@$"),

+ ]

+ 

+ tag_prefix = "build/"

+ 

+ 

+ def escape_sequence(str_seq: str) -> str:

+     """Compute a byte-escaped sequence for a string"""

+     return "".join(f"%{byte:02X}" for byte in str_seq.encode("utf-8"))

+ 

+ 

+ def escape_regex_match(match: re.Match) -> str:

+     """Escape whole or partial re.Match objects

+ 

+     match: The match object covering the sequence that should be

+         escaped (as a whole or partially). The regular expression may

+         not contain nested groups.

+     """

+     # The whole match

+     escaped = match.group()

+ 

+     # Spans count from the start of the string, not the match.

+     offset = match.start()

+ 

+     if match.lastindex:

+         # Work from the end in case string length changes.

+         idx_range = range(match.lastindex, 0, -1)

+     else:

+         # If match.lastindex is None, then operate on the whole match.

+         idx_range = (0,)

+ 

+     for idx in idx_range:

+         start, end = (x - offset for x in match.span(idx))

+         escaped = escaped[:start] + escape_sequence(match.group(idx)) + escaped[end:]

+ 

+     return escaped

+ 

+ 

+ def escape_tag(tagname: str) -> str:

+     """Escape prohibited character sequences in git tag names

+ 

+     tagname: An unescaped tag name.

+ 

+     Returns: An escaped tag name which can be converted back using

+         unescape_tag().

+     """

+     # Leave '+' as is, some version schemes may contain it.

+     escaped = parse.quote(tagname, safe="+")

+ 

+     # This will quote the string in a way that urllib.parse.unquote() can undo it, i.e. only replace

+     # characters by the URL escape sequence of their UTF-8 encoded value.

+     for seq in git_tag_seqs_to_escape:

+         if isinstance(seq, str):

+             escaped = escaped.replace(seq, escape_sequence(seq))

+         elif isinstance(seq, re.Pattern):

+             escaped = seq.sub(escape_regex_match, escaped)

+         else:

+             raise TypeError("Don't know how to deal with escape sequence: {seq!r}")

+ 

+     return escaped

+ 

+ 

+ def unescape_tag(escaped_tagname: str) -> str:

+     """Unescape prohibited characters sequences in git tag names

+ 

+     This essentially just exists for symmetry with escape_tag() and just

+     wraps urllib.parse.unquote().

+ 

+     escaped_tagname: A tag name that was escaped with escape_tag().

+ 

+     Returns: An unescaped tag name.

+     """

+     return parse.unquote(escaped_tagname)

  

  

  def register_subcommand(subparsers):

-     subcmd_name = "tag-project"

+     subcmd_name = "tag-package"

  

      tag_project_parser = subparsers.add_parser(

          subcmd_name, help="Tag the git commits corresponding to a build in koji",
@@ -50,32 +131,51 @@ 

      """ Main method. """

      kojiclient = koji_init(args.koji_url)

  

-     repopath = args.worktree_path

+     repopath = args.worktree_path.rstrip(os.path.sep)

  

      name = os.path.basename(repopath)

      for build in get_package_builds(name):

-         if build.get("epoch"):

-             nevr = f"{build['name']}-{build['epoch']}:{build['version']}-{build['release']}"

-         else:

-             nevr = f"{build['name']}-{build['version']}-{build['release']}"

+         nevr = f"{build['name']}-{build.get('epoch') or 0}-{build['version']}-{build['release']}"

+ 

+         owner_name = build["owner_name"]

+         # Ignore modular builds, account for staging

+         if owner_name.startswith("mbs/") and owner_name.endswith(".fedoraproject.org"):

+             _log.info(f"Ignoring modular build: {nevr}")

+             continue

+ 

          commit = None

-         if "source" in build and build["source"]:

+         # FIXME: We probably shouldn't hardcode "fedoraproject.org" here and below, rather use a

+         # configurable full host name.

+         if "source" in build and build["source"] and "fedoraproject.org" in build["source"]:

              com = build["source"].partition("#")[-1]

-             try:

-                 int(com, 16)

-                 commit = com

-             except ValueError:

-                 # The hash isn't an hexadecimal number so, skipping it

-                 pass

+             # git commit hashes are 40 characters long, so we will

+             # assume if the part after the '#' is 40 characters long it

+             # is a commit hash and not a 40 characters long branch or

+             # tag name

+             if len(com) == 40:

+                 try:

+                     int(com, 16)

+                     commit = com

+                 except ValueError:

+                     # The hash isn't a hexadecimal number, skip it

+                     pass

  

          if commit is None:

              tasks = kojiclient.getTaskChildren(build["task_id"])

-             task = [t for t in tasks if t["method"] == "buildSRPMFromSCM"][0]

+             try:

+                 task = [t for t in tasks if t["method"] == "buildSRPMFromSCM"][0]

+             except IndexError:

+                 _log.info(

+                     "Ignoring build without buildSRPMFromSCM task, or any task at all, "

+                     f"probably an old dist-cvs build: {nevr} (build id: {build['build_id']})"

+                 )

+                 continue

+ 

              task_req = kojiclient.getTaskRequest(task["id"])

              if "fedoraproject.org" in task_req[0]:

                  com = task_req[0].partition("#")[-1]

                  # git commit hashes are 40 characters long, so we will

-                 # assume if the part after the '#' in 40 characters long it

+                 # assume if the part after the '#' is 40 characters long it

                  # is a commit hash and not a 40 characters long branch or

                  # tag name

                  if len(com) == 40:
@@ -83,17 +183,15 @@ 

                          int(com, 16)

                          commit = com

                      except ValueError:

-                         # The hash isn't an hexadecimal number so, skipping it

+                         # The hash isn't a hexadecimal number, skip it

                          pass

  

          if commit:

-             # Escape un-allowed characters

-             for char in escape_chars:

-                 nevr = nevr.replace(char, escape_chars[char])

-             command = ["git", "tag", nevr, commit]

+             tag = tag_prefix + escape_tag(nevr)

+             command = ["git", "tag", tag, commit]

              try:

                  run_command(command, cwd=repopath)

              except RuntimeError as err:

                  print(err)

              else:

-                 print(f"tagged commit {commit} as {nevr}")

+                 print(f"tagged commit {commit} as {tag}")

tests/test_release.py tests/test_calc_release.py
file renamed
+3 -1
@@ -33,7 +33,9 @@ 

      return parameters

  

  

- class TestNextBuild:

+ class TestRelease:

+     """Test the rpmautospec.release module"""

+ 

      @pytest.mark.parametrize("test_data", data_as_test_parameters(test_data))

      def test_main(self, test_data, capsys):

          with open(

@@ -0,0 +1,246 @@ 

+ import logging

+ import subprocess

+ from unittest import mock

+ 

+ import pytest

+ 

+ from rpmautospec import tag_package

+ 

+ 

+ VALID_HASH = "0123456789abcdef0123456789abcdef01234567"

+ INVALID_HASH = "not a hash but long enough 0123456789012"

+ TOO_SHORT_HASH = "0123abcd"

+ 

+ 

+ def get_test_builds(phenomena):

+     if "stagingbuildsys" in phenomena:

+         buildsys_host = "src.stg.fedoraproject.org"

+     elif "wrongbuildsys" in phenomena:

+         buildsys_host = "someones.system.at.home.org"

+     else:

+         buildsys_host = "src.fedoraproject.org"

+ 

+     build = {

+         "name": "pkgname",

+         "epoch": None,

+         "version": "1.0",

+         "release": "1.fc32",

+         "owner_name": "hamburglar",

+         "source": f"git+https://{buildsys_host}/rpms/pkgname#{VALID_HASH}",

+         "build_id": 123,

+         "task_id": 54321,

+     }

+ 

+     for phenomenon in phenomena:

+         if phenomenon in (

+             "normal",

+             "nosrpmtask",

+             "notasks",

+             "trailingslashpath",

+             "stagingbuildsys",

+             "wrongbuildsys",

+             "tagcmdfails",

+         ):

+             # Ignore phenomena handled above and/or in the test method.

+             pass

+         elif phenomenon == "epoch":

+             build["epoch"] = 1

+         elif phenomenon == "modularbuild":

+             build["owner_name"] = "mbs/mbs.fedoraproject.org"

+         elif phenomenon in ("invalidhash", "tooshorthash"):

+             if "source" in build:

+                 # Don't accidentally put the source back if it shouldn't be.

+                 if phenomenon == "invalidhash":

+                     hash_val = INVALID_HASH

+                 else:

+                     hash_val = TOO_SHORT_HASH

+                 build["source"] = f"git+https://src.fedoraproject.org/rpms/pkgname#{hash_val}"

+         elif phenomenon == "nosource":

+             del build["source"]

+         else:

+             raise ValueError(f"Don't understand phenomenon: {phenomenon}")

+ 

+     return [build]

+ 

+ 

+ class TestTagPackage:

+     """Test the rpmautospec.tag_package module."""

+ 

+     test_escape_sequences = {

+         "%": "%25",

+         ":": "%3A",

+         "^": "%5E",

+         "~": "%7E",

+         "%:^~👍": "%25%3A%5E%7E%F0%9F%91%8D",

+     }

+ 

+     test_escape_tags = {

+         # Artificial, to cover all git tag constraints

+         ".boo": "%2Eboo",

+         "blah.lock": "blah%2Elock",

+         "foo..bar": "foo%2E%2Ebar",

+         "foo\x10bar": "foo%10bar",

+         "foo bar": "foo%20bar",

+         "foo~bar^baz:gna": "foo%7Ebar%5Ebaz%3Agna",

+         "?*[": "%3F%2A%5B",

+         "/foo/": "%2Ffoo%2F",

+         "foo/bar": "foo%2Fbar",

+         "foo//bar": "foo%2F%2Fbar",

+         "foo///bar": "foo%2F%2F%2Fbar",

+         "foo.": "foo%2E",

+         "foo@{bar": "foo%40%7Bbar",

+         "@": "%40",

+         "back\\slash": "back%5Cslash",

+         # We want plus signs to be preserved

+         "foo+bar": "foo+bar",

+         # Actual N[E]VRs go here

+         "gimp-2-2.10.18-1.fc31": "gimp-2-2.10.18-1.fc31",

+     }

+ 

+     @pytest.mark.parametrize("sequence", test_escape_sequences)

+     def test_escape_sequence(self, sequence):

+         """Test escape_sequence()"""

+         assert tag_package.escape_sequence(sequence) == self.test_escape_sequences[sequence]

+ 

+     @pytest.mark.parametrize("unescaped_tag", test_escape_tags)

+     def test_escape_tag(self, unescaped_tag):

+         """Test escape_tag()"""

+         assert tag_package.escape_tag(unescaped_tag) == self.test_escape_tags[unescaped_tag]

+ 

+     @mock.patch.object(tag_package, "git_tag_seqs_to_escape", [b"Not a string"])

+     def test_escape_tag_broken_git_tag_seqs_to_escape(self):

+         """Test escape_tag() with garbage in git_tag_seqs_to_escape"""

+         with pytest.raises(TypeError):

+             tag_package.escape_tag("a string")

+ 

+     @pytest.mark.parametrize("unescaped_tag", test_escape_tags)

+     def test_unescape_tag(self, unescaped_tag):

+         """Test escape_tag()"""

+         escaped_tag = self.test_escape_tags[unescaped_tag]

+         assert tag_package.unescape_tag(escaped_tag) == unescaped_tag

+ 

+     @mock.patch("rpmautospec.tag_package.subprocess.check_output")

+     @pytest.mark.parametrize("raise_exception", (False, True))

+     def test_run_command(self, check_output, raise_exception, caplog):

+         """Test run_command()"""

+         caplog.set_level(logging.DEBUG)

+ 

+         if not raise_exception:

+             check_output.return_value = "Some output"

+             assert tag_package.run_command(["command"]) == "Some output"

+             check_output.assert_called_once_with(["command"], cwd=None, stderr=subprocess.PIPE)

+             assert not any(rec.levelno >= logging.WARNING for rec in caplog.records)

+         else:

+             check_output.side_effect = subprocess.CalledProcessError(

+                 returncode=139, cmd=["command"], output="Some command", stderr="And it failed!",

+             )

+             with pytest.raises(RuntimeError) as excinfo:

+                 tag_package.run_command(["command"])

+             assert str(excinfo.value) == "Command `command` failed to run, returned 139"

+             assert any(rec.levelno == logging.ERROR for rec in caplog.records)

+ 

+     @pytest.mark.parametrize(

+         "phenomena",

+         (

+             "normal",

+             "stagingbuildsys",

+             "wrongbuildsys",

+             "trailingslashpath",

+             "epoch",

+             "modularbuild",

+             "invalidhash",

+             "tooshorthash",

+             "nosource",

+             "nosource,nosrpmtask",

+             "nosource,notasks",

+             "nosource,invalidhash",

+             "nosource,tooshorthash",

+             "nosource,stagingbuildsys",

+             "nosource,wrongbuildsys",

+             "tagcmdfails",

+         ),

+     )

+     @mock.patch("rpmautospec.tag_package.run_command")

+     @mock.patch("rpmautospec.tag_package.koji_init")

+     @mock.patch("rpmautospec.tag_package.get_package_builds")

+     def test_main(self, get_package_builds, koji_init, run_command, phenomena, capsys):

+         """Test the tag_package.main() under various conditions."""

+         phenomena = [p.strip() for p in phenomena.split(",")]

+         koji_init.return_value = kojiclient = mock.Mock()

+         get_package_builds.return_value = test_builds = get_test_builds(phenomena)

+ 

+         main_args = mock.Mock()

+         # This IP address is from the reserved TEST-NET-1 address space, i.e. should be

+         # guaranteed to not exist or be routable. Just in case we fail to mock out code that

+         # attempts to contact a remote Koji instance.

+         main_args.koji_url = "https://192.0.2.1"

+         main_args.worktree_path = repopath = "/path/to/my/package/repo/pkgname"

+         if "trailingslashpath" in phenomena:

+             # This shouldn't change anything, really.

+             main_args.worktree_path += "/"

+ 

+         if "invalidhash" in phenomena:

+             commit = INVALID_HASH

+         elif "tooshorthash" in phenomena:

+             commit = TOO_SHORT_HASH

+         else:

+             commit = VALID_HASH

+ 

+         if any(

+             p in ("invalidhash", "tooshorthash", "nosource", "wrongbuildsys") for p in phenomena

+         ):

+             if "notasks" in phenomena:

+                 tasks = []

+             else:

+                 tasks = [{"method": "ignoremeplease", "task": 456789}]

+                 if "nosrpmtask" not in phenomena:

+                     tasks.append({"method": "buildSRPMFromSCM", "id": 123456})

+                     if "stagingbuildsys" in phenomena:

+                         buildsys_host = "src.stg.fedoraproject.org"

+                     elif "wrongbuildsys" in phenomena:

+                         buildsys_host = "someones.system.at.home.org"

+                     else:

+                         buildsys_host = "src.fedoraproject.org"

+                     task_source = f"git+https://{buildsys_host}/rpms/pkgname#{commit}"

+                     kojiclient.getTaskRequest.return_value = [task_source]

+             kojiclient.getTaskChildren.return_value = tasks

+ 

+         if "tagcmdfails" in phenomena:

+             run_command.side_effect = RuntimeError("lp0 is on fire")

+ 

+         tag_package.main(main_args)

+ 

+         if "nosource" in phenomena:

+             kojiclient.getTaskChildren.assert_called_once_with(test_builds[0]["task_id"])

+ 

+             if any(p in ("notasks", "nosrpmtask") for p in phenomena):

+                 kojiclient.getTaskRequest.assert_not_called()

+ 

+         if any(

+             p

+             in (

+                 "invalidhash",

+                 "tooshorthash",

+                 "modularbuild",

+                 "nosrpmtask",

+                 "notasks",

+                 "wrongbuildsys",

+             )

+             for p in phenomena

+         ):

+             run_command.assert_not_called()

+         else:

+             build = test_builds[0]

+             nevr = "-".join(

+                 str(build[partname]) if build[partname] else "0"

+                 for partname in ("name", "epoch", "version", "release")

+             )

+             tag = f"build/{nevr}"

+             run_command.assert_called_once_with(["git", "tag", tag, commit], cwd=repopath)

+ 

+             stdout, stderr = capsys.readouterr()

+ 

+             if "tagcmdfails" in phenomena:

+                 assert stdout.strip() == "lp0 is on fire"

+             else:

+                 assert stdout.strip() == f"tagged commit {commit} as {tag}"

Tagging packages:
- Rename the command to tag-package (rather than tag-project)
- Improve escaping NEVRs for use as git tags
- Cope with trailing slashes in the worktree path
- Don't trip over very old (dist-cvs rather than dist.git) builds
- Don't tag modular builds
- Test tag_package more thoroughly
- Fix typos and phrasing
- Cope with some more edge cases

Other stuff:
- Use consistent name for the class testing the release module

rebased onto 4b742d3ff209837b255965b99d232086052f72e4

4 years ago

rebased onto 4b742d3ff209837b255965b99d232086052f72e4

4 years ago

rebased onto b492261f256bebe7bca4a6530d2a5dcf80fb3ee1

4 years ago

This is good to be reviewed now. I think. :grinning:

Let's not merge this yet. I want to do something about the epoch separator, right now it doesn't read well, e.g.: gimp-2%3A2.10.18-1.fc31

rebased onto bd20a7d

4 years ago

Much better:

tagged commit 8e1f9d25f7c1a66b1ba30c153a01c124bf30c4b7 as gimp-2@2.10.18-1.fc31

I'm not too enthusiastic about using @ here—something rubs me the wrong way about it, but I can't put a finger on it. However, it seems like all other ASCII characters fall into one of these categories:

  • They're normally used in a NEVR.
  • They have special meaning in the shell.
  • They have special meaning for git refnames or a tag cannot contain them.

For my internal stuff, I usually just converted : to -, since it's essentially a separator and not a modifier. In order to do this sanely, I always made sure the value was populated in the tags. So no Epoch would map to 0.

That should work, thanks!

6 new commits added

  • Don't tag modular builds
  • We were young and needed the version control
  • Cope with trailing slashes in the worktree path
  • Improve escaping NEVRs for use as git tags
  • Use test module and class names mapping to modules
  • Rename 'tag-project' command to 'tag-package'
4 years ago

This encodes the epoch as described above and puts the tags into the build/ namespace. Thanks @ngompa and @clime for the suggestions!

6 new commits added

  • Don't tag modular builds
  • We were young and needed the version control
  • Cope with trailing slashes in the worktree path
  • Improve escaping NEVRs for use as git tags
  • Use test module and class names mapping to modules
  • Rename 'tag-project' command to 'tag-package'
4 years ago

And this time, it even doesn't encode a missing epoch as None! :wink:

3 new commits added

  • Cope with a couple of more edge cases
  • fix typos and phrasing
  • Test tag_package more thoroughly
4 years ago

9 new commits added

  • Cope with a couple of more edge cases
  • fix typos and phrasing
  • Test tag_package more thoroughly
  • Don't tag modular builds
  • We were young and needed the version control
  • Cope with trailing slashes in the worktree path
  • Improve escaping NEVRs for use as git tags
  • Use test module and class names mapping to modules
  • Rename 'tag-project' command to 'tag-package'
4 years ago

And now with proper tests.

9 new commits added

  • Cope with some more edge cases
  • Fix typos and phrasing
  • Test tag_package more thoroughly
  • Don't tag modular builds
  • We were young and needed the version control
  • Cope with trailing slashes in the worktree path
  • Improve escaping NEVRs for use as git tags
  • Use test module and class names mapping to modules
  • Rename 'tag-project' command to 'tag-package'
4 years ago

9 new commits added

  • Cope with some more edge cases
  • Fix typos and phrasing
  • Test tag_package more thoroughly
  • Don't tag modular builds
  • We were young and needed the version control
  • Cope with trailing slashes in the worktree path
  • Improve escaping NEVRs for use as git tags
  • Use test module and class names mapping to modules
  • Rename 'tag-project' command to 'tag-package'
4 years ago

Looks good enough to merge :)

Pull-Request has been merged by asaleh

4 years ago