#97 Tag only latest builds (instead of all, by default)
Merged 4 years ago by pingou. Opened 4 years ago by nphilipp.
fedora-infra/ nphilipp/rpmautospec master--tag-only-latest  into  master

@@ -70,7 +70,7 @@ 

          pagure_proxy = PagureTaggingProxy(base_url=base_url, auth_token=token, logger=_log)

  

      _log.info("Tagging existing builds...")

-     tag_package.tag_package(srcdir, session, pagure_proxy)

+     tag_package.tag_package(srcdir, session, pagure_proxy=pagure_proxy)

  

      buildroot = kwargs.get("buildroot")

      if not buildroot:

file modified
+12 -3
@@ -30,12 +30,21 @@ 

      logging.basicConfig(level=log_level, handlers=handlers)

  

  

+ class CustomArgumentParser(argparse.ArgumentParser):

+     """Custom argument parser class for this program

+ 

+     This overrides the `formatter_class` globally, for all created subparsers.

+     """

+ 

+     def __init__(self, *args, **kwargs):

+         kwargs.setdefault("formatter_class", argparse.ArgumentDefaultsHelpFormatter)

+         super().__init__(*args, **kwargs)

+ 

+ 

  def get_cli_args(args: typing.List[str]) -> argparse.Namespace:

      global subcmd_modules_by_name

  

-     parser = argparse.ArgumentParser(

-         prog="rpmautospec", formatter_class=argparse.ArgumentDefaultsHelpFormatter

-     )

+     parser = CustomArgumentParser(prog="rpmautospec")

  

      # global arguments

  

file modified
+5 -3
@@ -17,7 +17,7 @@ 

  

  

  release_re = re.compile(r"^(?P<pkgrel>\d+)(?:(?P<middle>.*?)(?:\.(?P<minorbump>\d+))?)?$")

- disttag_re = re.compile(r"^\.?(?P<distcode>[^\d\.]+)(?P<distver>\d+)")

+ disttag_re = re.compile(r"\.?(?P<distcode>[^\d\.]+)(?P<distver>\d+)")

  evr_re = re.compile(r"^(?:(?P<epoch>\d+):)?(?P<version>[^-:]+)(?:-(?P<release>[^-:]+))?$")

  

  rpmvercmp_key = cmp_to_key(
@@ -98,14 +98,16 @@ 

      return _kojiclient

  

  

- def get_package_builds(pkgname: str, extras: bool = False) -> List[dict]:

+ def get_package_builds(pkgname: str) -> List[dict]:

      assert _kojiclient

  

      pkgid = _kojiclient.getPackageID(pkgname)

      if not pkgid:

          raise ValueError(f"Package {pkgname!r} not found!")

  

-     return _kojiclient.listBuilds(pkgid, type="rpm", queryOpts={"order": "-nvr"})

+     # Don't add queryOpts={"order": "-nvr"} or similar, this sorts alphanumerically and and this is

+     # not how EVRs should be sorted.

+     return _kojiclient.listBuilds(pkgid, type="rpm")

  

  

  def run_command(command: list, cwd: Optional[str] = None) -> bytes:

file modified
+1 -1
@@ -118,7 +118,7 @@ 

      latest_evr: typing.Optional[typing.Tuple[int, str, str]] = None,

      strip_dist: bool = False,

  ):

-     match = disttag_re.match(dist)

+     match = disttag_re.fullmatch(dist)

      if not match:

          raise RuntimeError("Dist tag %r has wrong format (should be e.g. 'fc31', 'epel7')", dist)

  

file modified
+50 -5
@@ -1,9 +1,10 @@ 

  #!/usr/bin/python3

  

+ import argparse

  import logging

  import os

  

- from .misc import get_package_builds, koji_init, run_command

+ from .misc import disttag_re, get_package_builds, koji_init, rpmvercmp_key, run_command

  from .py2compat.tagging import escape_tag, PagureTaggingProxy, tag_prefix

  

  
@@ -17,7 +18,6 @@ 

          subcmd_name, help="Tag the git commits corresponding to a build in koji",

      )

  

-     tag_project_parser.add_argument("worktree_path", help="Path to the dist-git worktree")

      tag_project_parser.add_argument(

          "--pagure-url",

          help="Pagure url for tagging the package in the repo. Requires pagure_token to work.",
@@ -28,17 +28,57 @@ 

          "--pagure-token",

          help="Pagure token for tagging the package in the repo. Requires pagure_url to work.",

      )

+ 

+     tag_latest_group = tag_project_parser.add_mutually_exclusive_group()

+     tag_latest_group.add_argument(

+         "--tag-latest-builds",

+         action="store_true",

+         default=True,

+         help="Tag only the latest existing builds for each dist tag.",

+     )

+     tag_latest_group.add_argument(

+         "--tag-all-builds",

+         action="store_false",

+         dest="tag_latest_builds",

+         default=argparse.SUPPRESS,

+         help="Tag all existing historical builds (slow).",

+     )

+ 

+     tag_project_parser.add_argument("worktree_path", help="Path to the dist-git worktree")

+ 

      return subcmd_name

  

  

- def tag_package(srcdir, session, pagure_proxy=None):

+ def tag_package(srcdir, session, pagure_proxy=None, tag_latest_builds=True):

      koji_init(session)

      repopath = srcdir.rstrip(os.path.sep)

  

      name = os.path.basename(repopath)

-     for build in get_package_builds(name):

+ 

+     disttags_found = set()

+ 

+     # Iterate over builds in reverse ENVR order

+     for build in sorted(get_package_builds(name), key=rpmvercmp_key, reverse=True):

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

  

+         if tag_latest_builds:

+             # Check if we've seen a build for the respective dist tag already. If so, skip. The

+             # `disttag_matches` list contains tuples of the distcode, distver groups, but they map

+             # 1:1 to dist tags, so we don't need to process further.

+             disttag_matches = disttag_re.findall(build["release"])

+             if not disttag_matches:

+                 _log.warning("Can't find dist tag for build: %s", build["nvr"])

+                 continue

+ 

+             # The dist tag is the right-most match of the pattern in the release string.

+             disttag = disttag_matches[-1]

+ 

+             if disttag in disttags_found:

+                 _log.debug("Skipping older build: %s", build["nvr"])

+                 continue

+ 

+             disttags_found.add(disttag)

+ 

          owner_name = build["owner_name"]

          # Ignore modular builds, account for staging

          if owner_name.startswith("mbs/") and owner_name.endswith(".fedoraproject.org"):
@@ -114,4 +154,9 @@ 

      if args.pagure_url and args.pagure_token:

          pagure_proxy = PagureTaggingProxy(base_url=args.pagure_url, auth_token=args.pagure_token)

  

-     tag_package(args.worktree_path, session, pagure_proxy)

+     tag_package(

+         args.worktree_path,

+         session,

+         pagure_proxy=pagure_proxy,

+         tag_latest_builds=args.tag_latest_builds,

+     )

file added
+25
@@ -0,0 +1,25 @@ 

+ class MainArgs:

+     """Substitute for argparse.Namespace for tests

+ 

+     This simply returns None for any undefined attribute which is useful for

+     testing the main() functions of subcommands.

+ 

+     Use this instead of Mock or MagicMock objects for parsed arguments in

+     tests.

+     """

+ 

+     def __init__(self, **kwargs):

+         for k, v in kwargs.items():

+             setattr(self, k, v)

+ 

+     def __repr__(self):

+         clsname = self.__class__.__name__

+         kwargs = [

+             f"{attr}={getattr(self, attr)!r}" for attr in dir(self) if not attr.startswith("_")

+         ]

+         return f"{clsname}({', '.join(kwargs)})"

+ 

+     def __getattr__(self, attr):

+         # Set this on the object so it shows up in repr()

+         setattr(self, attr, None)

+         return None

@@ -153,7 +153,9 @@ 

              tag_package_fn.assert_not_called()

              return

  

-         tag_package_fn.assert_called_once_with(unpacked_repo_dir, koji_session, pagure_proxy)

+         tag_package_fn.assert_called_once_with(

+             unpacked_repo_dir, koji_session, pagure_proxy=pagure_proxy

+         )

  

          buildroot.getPackageList.assert_called_once()

          buildroot.path_without_to_within.assert_called_once()

@@ -6,6 +6,7 @@ 

  import pytest

  

  from rpmautospec import release

+ from ..common import MainArgs

  

  

  __here__ = os.path.dirname(__file__)
@@ -60,7 +61,7 @@ 

              mock_client.getPackageID.return_value = 1234

              mock_client.listBuilds.return_value = koji_list_builds_output

  

-             main_args = mock.Mock()

+             main_args = MainArgs()

              main_args.algorithm = "sequential_builds"

              main_args.dist_git = os.path.sep.join(["tmp", test_data["package"]])

              main_args.dist = test_data["dist"]

@@ -4,6 +4,7 @@ 

  import pytest

  

  from rpmautospec import tag_package

+ from ..common import MainArgs

  

  

  VALID_HASH = "0123456789abcdef0123456789abcdef01234567"
@@ -19,16 +20,25 @@ 

      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,

-     }

+     builds = [

+         {

+             "name": "pkgname",

+             "epoch": None,

+             "version": "1.0",

+             "release": f"{relver}.fc{distver}",

+             "nvr": f"pkgname-1.0-{relver}.fc{distver}",

+             "owner_name": "hamburglar",

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

+             "build_id": build_id,

+             "task_id": 54321,

+         }

+         for build_id, (distver, relver) in enumerate(

+             ((distver, relver) for distver in (32, 31, 30) for relver in (3, 2, 1)), 123

+         )

+     ]

+ 

+     firstbuild = builds[0]

+     lastbuild = builds[-1]

  

      for phenomenon in phenomena:

          if phenomenon in (
@@ -40,27 +50,30 @@ 

              "wrongbuildsys",

              "tagcmdfails",

              "pagure_tag",

+             "tagallbuilds",

          ):

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

              pass

          elif phenomenon == "epoch":

-             build["epoch"] = 1

+             firstbuild["epoch"] = 1

          elif phenomenon == "modularbuild":

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

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

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

-             if "source" in build:

+             if "source" in firstbuild:

                  # 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}"

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

          elif phenomenon == "nosource":

-             del build["source"]

+             del firstbuild["source"]

+         elif phenomenon == "releasewithoutdisttag":

+             lastbuild["release"] = "1"

          else:

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

  

-     return [build]

+     return builds

  

  

  class TestTagPackage:
@@ -86,6 +99,8 @@ 

              "nosource,wrongbuildsys",

              "tagcmdfails",

              "pagure_tag",

+             "tagallbuilds",

+             "releasewithoutdisttag",

          ),

      )

      @mock.patch("rpmautospec.tag_package.run_command")
@@ -101,7 +116,7 @@ 

  

          get_package_builds.return_value = test_builds = get_test_builds(phenomena)

  

-         main_args = mock.Mock()

+         main_args = MainArgs()

          # 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.
@@ -113,6 +128,8 @@ 

              main_args.pagure_url = "https://192.0.2.2"

              main_args.pagure_token = "token"

  

+         main_args.tag_latest_builds = "tagallbuilds" not in phenomena

+ 

          if "trailingslashpath" in phenomena:

              # This shouldn't change anything, really.

              main_args.worktree_path += "/"
@@ -144,15 +161,16 @@ 

              kojiclient.getTaskChildren.return_value = tasks

  

          if "tagcmdfails" in phenomena:

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

+             # It's always the first build that fails

+             run_command.side_effect = [RuntimeError("lp0 is on fire")] + [0] * len(test_builds)

  

          tag_package.main(main_args)

  

          if "pagure_tag" in phenomena:

-             pagure_post.assert_called_once_with(

+             pagure_post.assert_any_call(

                  "https://192.0.2.2/api/0/rpms/pkgname/git/tags",

                  data={

-                     "tagname": "build/pkgname-0-1.0-1.fc32",

+                     "tagname": "build/pkgname-0-1.0-3.fc32",

                      "commit_hash": "0123456789abcdef0123456789abcdef01234567",

                      "message": None,

                      "with_commits": True,
@@ -162,11 +180,14 @@ 

              )

  

          if "nosource" in phenomena:

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

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

  

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

                  kojiclient.getTaskRequest.assert_not_called()

  

+         if "modularbuild" in phenomena:

+             assert any("Ignoring modular build:" in m for m in caplog.messages)

+ 

          if any(

              p

              in (
@@ -176,22 +197,36 @@ 

                  "nosrpmtask",

                  "notasks",

                  "wrongbuildsys",

+                 "tagcmdfails",

              )

              for p in phenomena

          ):

-             run_command.assert_not_called()

+             first_build_skipped = True

          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", "--force", tag, commit], cwd=repopath

+             first_build_skipped = False

+ 

+         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}"

+ 

+         if first_build_skipped:

+             assert all(

+                 c.args != ["git", "tag", "--force", tag, commit] for c in run_command.call_args_list

              )

+         else:

+             run_command.assert_any_call(["git", "tag", "--force", tag, commit], cwd=repopath)

  

-             if "tagcmdfails" in phenomena:

-                 assert "lp0 is on fire" in caplog.text

-             else:

-                 assert f"Tagged commit {commit} as {tag}" in caplog.messages

+         if "tagcmdfails" in phenomena:

+             assert "lp0 is on fire" in caplog.text

+ 

+         if not first_build_skipped:

+             assert f"Tagged commit {commit} as {tag}" in caplog.messages

+ 

+         if "tagallbuilds" not in phenomena:

+             assert any("Skipping older build:" in s for s in caplog.messages)

+ 

+         if "releasewithoutdisttag" in phenomena:

+             assert any("Can't find dist tag for build:" in s for s in caplog.messages)

Also, add default values in help for options in all sub parsers. Fix some broken tests.

6 new commits added

  • Fix _not_ setting CLI options in tests
  • Tag only latest builds for a dist per default
  • Let `misc.disttag_re` match substrings
  • Name optional arguments for tag_package()
  • Tidy up get_package_builds()
  • Globally add default values to argument help
4 years ago

I'd say: and this is not how EVRs should be sorted

This confuses me a bit, when is this class not returning a None object?

Build succeeded.

This so __repr__() above lists an (unset) attribute which was queried as None. Should I add a comment?

8 new commits added

  • Test builds without a recognizable dist tag
  • Test tagging all or only latest builds
  • Fix _not_ setting CLI options in tests
  • Tag only latest builds for a dist per default
  • Let `misc.disttag_re` match substrings
  • Name optional arguments for tag_package()
  • Tidy up get_package_builds()
  • Globally add default values to argument help
4 years ago

Alright, :thumbsup: for me :)

Build succeeded.

Pull-Request has been merged by pingou

4 years ago