PR#5: Attempt at the changelog generating callback - fedora-infra/rpmautospec

fedora-infra / rpmautospec

#5 Attempt at the changelog generating callback

Merged 4 years ago by nphilipp. Opened 4 years ago by asaleh.

fedora-infra/ asaleh/rpmautospec koji_callback into master

Add Koji automatic release and changelog plugin

Adam Saleh • 4 years ago

62410d4

Reorganize test directory structure and naming

Nils Philippsen • 4 years ago

3348716

Add .gitignore

Adam Saleh • 4 years ago

ecdd77a

.gitignore

file added

		`@@ -0,0 +1,4 @@`
		`+ # Byte-compiled / optimized / DLL files`
		`+ __pycache__/`
		`+ *.py[cod]`
		`+ .vscode`

koji_plugin/rpmautospec_plugin.py

file added

+58

		`@@ -0,0 +1,58 @@`
		`+ import os`
		`+ from glob import glob`
		`+`
		`+ from koji.plugin import callback`
		`+`
		`+ from rpmautospec.changelog import produce_changelog`
		`+ from rpmautospec.misc import koji_init`
		`+ from rpmautospec.release import holistic_heuristic_algo`
		`+`
		`+ template = """%global function autorel() {{`
		`+ return {autorel}`
		`+ }}`
		`+ """`
		`+`
		`+`
		`+ def is_autorel(l):`
		`+ return "Release:" in l and "%{autorel}" in l`
		`+`
		`+`
		`+ def get_autorel(name, dist, session):`
		`+ koji_init(session)`
		`+ # evr=None forces to search from lower bound`
		`+ release = holistic_heuristic_algo(package=name, dist=dist, evr=None)`
		`+ return release`
		`+`
		`+`
		`+ @callback("postSCMCheckout")`
		`+ def autospec_cb(cb_type, , srcdir, build_tag, session, taskinfo, *kwargs):`
		`+ if taskinfo["method"] != "buildSRPMFromSCM":`
		`+ # callback should be run only in this instance`
		`+ # i.e. maven and image builds don't have spec-files`
		`+ return`
		`+`
		`+ dist = build_tag["tag_name"]`
		`+ name = os.path.basename(srcdir)`
		`+ autospec = False`
		`+ autorel = False`
		`+ new_rel = get_autorel(name, dist, session)`
		`+ specfiles = glob(f"{srcdir}/*.spec")`
		`+ if len(specfiles) != 1:`
		`+ # callback should be run only in if there is a single spec-file`
		`+ return`
		`+`
		`+ with open(specfiles[0], "r") as specfile:`
		`+ lines = [l.rstrip("\n") for l in specfile]`
		`+ autorel = any(is_autorel(l) for l in lines)`
		`+`
		`+ if lines[-1] == "%{autochangelog}" and lines[-2] == "%changelog":`
		`+ autospec = True`
		`+`
		`+ if autorel:`
		`+ lines.insert(0, template.format(autorel=new_rel))`
		`+ if autospec:`
		`+ del lines[-1]`
		`+ lines += produce_changelog(srcdir)`
		`+ if autorel or autospec:`
		`+ with open(f"{srcdir}/{name}.spec", "w") as specfile:`
		`+ specfile.writelines(l + "\n" for l in lines)`

rpmautospec/changelog.py

file modified

+75 -64

		`@@ -1,10 +1,11 @@`
		`#!/usr/bin/python3`
		`-`
		`import collections`
		`import datetime`
		`import logging`
		`import os`
		`+ import shutil`
		`import subprocess`
		`+ import tempfile`
		`import textwrap`

		`import pygit2`
		`@@ -40,65 +41,66 @@`
		`return output`


		`- def main(args):`
		`- """ Main method. """`
		`- repopath = args.worktree_path`
		`-`
		`- repo_obj = pygit2.Repository(repopath)`
		`+ def produce_changelog(repopath):`
		`name = os.path.basename(repopath)`
		`-`
		`- branch = repo_obj.lookup_branch(repo_obj.head.shorthand)`
		`- commit = branch.peel(pygit2.Commit)`
		`- data = collections.defaultdict(list)`
		`- for commit in repo_obj.walk(commit.hex, pygit2.GIT_SORT_TIME):`
		`- if len(commit.parents) > 1:`
		`- # Ignore merge commits`
		`- continue`
		`-`
		`- commit_dt = datetime.datetime.utcfromtimestamp(commit.commit_time)`
		`- if commit_dt < (datetime.datetime.utcnow() - datetime.timedelta(days=730)):`
		`- # Ignore all commits older than 2 years`
		`- break`
		`-`
		`- repo_obj.checkout_tree(`
		`- commit, strategy=pygit2.GIT_CHECKOUT_FORCE \| pygit2.GIT_CHECKOUT_RECREATE_MISSING,`
		`- )`
		`- if os.path.exists(os.path.join(repopath, f"{name}.spec")):`
		`- try:`
		`- output = run_command(`
		`- [`
		`- "rpm",`
		`- "--qf",`
		`- "%{name} %{version} %{release}\n",`
		`- "--specfile",`
		`- f"{name}.spec",`
		`- ],`
		`- cwd=repopath,`
		`- )`
		`- except Exception:`
		`+ with tempfile.TemporaryDirectory() as workdir:`
		`+ repocopy = f"{workdir}/{name}"`
		`+ shutil.copytree(repopath, repocopy)`
		`+ lines = []`
		`+ repo_obj = pygit2.Repository(repocopy)`
		`+`
		`+ branch = repo_obj.lookup_branch(repo_obj.head.shorthand)`
		`+ commit = branch.peel(pygit2.Commit)`
		`+ data = collections.defaultdict(list)`
		`+ for commit in repo_obj.walk(commit.hex, pygit2.GIT_SORT_TIME):`
		`+ if len(commit.parents) > 1:`
		`+ # Ignore merge commits`
		`continue`
		`- output = tuple(`
		`- output.decode("utf-8").strip().split("\n")[0].rsplit(".", 1)[0].split(" ")`
		`- )`
		`- nvr = "-".join(output)`

		`- if commit.parents:`
		`- diff = repo_obj.diff(commit.parents[0], commit)`
		`+ commit_dt = datetime.datetime.utcfromtimestamp(commit.commit_time)`
		`+ if commit_dt < (datetime.datetime.utcnow() - datetime.timedelta(days=730)):`
		`+ # Ignore all commits older than 2 years`
		`+ break`
		`+`
		`+ repo_obj.checkout_tree(`
		`+ commit, strategy=pygit2.GIT_CHECKOUT_FORCE \| pygit2.GIT_CHECKOUT_RECREATE_MISSING,`
		`+ )`
		`+ if os.path.exists(os.path.join(repocopy, f"{name}.spec")):`
		`+ try:`
		`+ output = run_command(`
		`+ [`
		`+ "rpm",`
		`+ "--qf",`
		`+ "%{name} %{version} %{release}\n",`
		`+ "--specfile",`
		`+ f"{name}.spec",`
		`+ ],`
		`+ cwd=repocopy,`
		`+ )`
		`+ except Exception:`
		`+ continue`
		`+ output = tuple(`
		`+ output.decode("utf-8").strip().split("\n")[0].rsplit(".", 1)[0].split(" "),`
		`+ )`
		`+ nvr = "-".join(output)`
		`+`
		`+ if commit.parents:`
		`+ diff = repo_obj.diff(commit.parents[0], commit)`
		`+ else:`
		`+ # First commit in the repo`
		`+ diff = commit.tree.diff_to_tree(swap=True)`
		`+`
		`+ if diff.stats.files_changed:`
		`+ files_changed = [d.new_file.path for d in diff.deltas]`
		`+ ignore = True`
		`+ for filename in files_changed:`
		`+ if filename.endswith((".spec", ".patch")):`
		`+ ignore = False`
		`+ if not ignore:`
		`+ data[output].append(commit)`
		`else:`
		`- # First commit in the repo`
		`- diff = commit.tree.diff_to_tree(swap=True)`
		`-`
		`- if diff.stats.files_changed:`
		`- files_changed = [d.new_file.path for d in diff.deltas]`
		`- ignore = True`
		`- for filename in files_changed:`
		`- if filename.endswith((".spec", ".patch")):`
		`- ignore = False`
		`- if not ignore:`
		`- data[output].append(commit)`
		`- else:`
		`- print("No more spec file, bailing")`
		`- break`
		`+ print("No more spec file, bailing")`
		`+ break`

		`for nvr, commits in data.items():`
		`for idx, commit in enumerate(reversed(commits)):`
		`@@ -108,14 +110,23 @@`
		`message = wrapper.fill(commit.message.split("\n")[0].strip("- "))`

		`if last_commit:`
		`- print(`
		`+ lines += [`
		`f"* {commit_dt.strftime('%a %b %d %Y')} {commit.author.name}"`
		`- f" <{commit.author.email}> - {nvr[1]}-{nvr[2]}"`
		`- )`
		`+ f" <{commit.author.email}> - {nvr[1]}-{nvr[2]}",`
		`+ ]`
		`else:`
		`- print(`
		`+ lines += [`
		`f"* {commit_dt.strftime('%a %b %d %Y')} {commit.author.name}"`
		`- f" <{commit.author.email}>"`
		`- )`
		`- print("- %s" % message)`
		`- print()`
		`+ f" <{commit.author.email}>",`
		`+ ]`
		`+ lines += ["- %s" % message]`
		`+ lines += [""]`
		`+ return lines`
		`+`
		`+`
		`+ def main(args):`
		`+ """ Main method. """`
		`+`
		`+ repopath = args.worktree_path`
		`+ changelog = produce_changelog(repopath)`
		`+ print("\n".join(changelog))`

rpmautospec/misc.py

file modified

+11 -5

		`@@ -1,6 +1,9 @@`
		`- from functools import cmp_to_key`
		`import re`
		`- from typing import List, Optional, Tuple`
		`+ from functools import cmp_to_key`
		`+ from typing import List`
		`+ from typing import Optional`
		`+ from typing import Tuple`
		`+ from typing import Union`

		`import koji`
		`import rpm`
		`@@ -14,7 +17,7 @@`
		`lambda b1, b2: rpm.labelCompare(`
		`(str(b1["epoch"]), b1["version"], b1["release"]),`
		`(str(b2["epoch"]), b2["version"], b2["release"]),`
		`- )`
		`+ ),`
		`)`

		`_kojiclient = None`
		`@@ -45,9 +48,12 @@`
		`return pkgrel, middle, minorbump`


		`- def koji_init(koji_url: str):`
		`+ def koji_init(koji_url_or_session: Union[str, koji.ClientSession]) -> koji.ClientSession:`
		`global _kojiclient`
		`- _kojiclient = koji.ClientSession(koji_url)`
		`+ if isinstance(koji_url_or_session, str):`
		`+ _kojiclient = koji.ClientSession(koji_url_or_session)`
		`+ else:`
		`+ _kojiclient = koji_url_or_session`
		`return _kojiclient`

rpmautospec/release.py

file modified

+18 -17

		`@@ -1,6 +1,4 @@`
		`#!/usr/bin/python3`
		`-`
		`- import argparse`
		`from collections import defaultdict`
		`from itertools import chain`
		`import logging`
		`@@ -8,9 +6,14 @@`
		`import sys`
		`import typing`

		`- from .misc import disttag_re, get_package_builds, koji_init, parse_evr, parse_release_tag`
		`- from .misc import rpmvercmp_key`
		`-`
		`+ from .misc import (`
		`+ disttag_re,`
		`+ get_package_builds,`
		`+ koji_init,`
		`+ parse_evr,`
		`+ parse_release_tag,`
		`+ rpmvercmp_key,`
		`+ )`

		`_log = logging.getLogger(__name__)`

		`@@ -63,13 +66,12 @@`


		`def holistic_heuristic_calculate_release(`
		`- args: argparse.Namespace, lower_bound: dict, higher_bound: typing.Optional[dict],`
		`+ dist, evr, lower_bound: dict, higher_bound: typing.Optional[dict],`
		`):`
		`- dist = args.dist`

		`# So what package EVR are we going for again? Default to "same as lower bound".`
		`try:`
		`- epoch, version, release = args.evr`
		`+ epoch, version, release = evr`
		`except TypeError:`
		`epoch, version, release = (`
		`lower_bound["epoch"],`
		`@@ -121,12 +123,11 @@`
		`return new_evr`


		`- def main_holistic_heuristic_algo(args):`
		`- match = disttag_re.match(args.dist)`
		`+ def holistic_heuristic_algo(package, dist, evr):`
		`+ match = disttag_re.match(dist)`
		`if not match:`
		`print(`
		`- f"Dist tag {args.dist!r} has wrong format (should be e.g. 'fc31', 'epel7')",`
		`- file=sys.stderr,`
		`+ f"Dist tag {dist!r} has wrong format (should be e.g. 'fc31', 'epel7')", file=sys.stderr,`
		`)`
		`sys.exit(1)`

		`@@ -135,9 +136,7 @@`

		`dtag_re = re.compile(fr"\.{distcode}(?P<distver>\d+)")`

		`- builds = [`
		`- build for build in get_package_builds(args.package) if dtag_re.search(build["release"])`
		`- ]`
		`+ builds = [build for build in get_package_builds(package) if dtag_re.search(build["release"])]`

		`# builds by distro release`
		`builds_per_distver = defaultdict(list)`
		`@@ -204,7 +203,7 @@`

		`print(f"Lowest satisfiable higher build in higher distro version: {higher_bound_nvr}")`

		`- new_evr = holistic_heuristic_calculate_release(args, lower_bound, higher_bound)`
		`+ new_evr = holistic_heuristic_calculate_release(dist, evr, lower_bound, higher_bound)`
		`if new_evr["epoch"]:`
		`new_evr_str = f"{new_evr['epoch']}:{new_evr['version']}-{new_evr['release']}"`
		`else:`
		`@@ -212,6 +211,8 @@`

		`print(f"Calculated new release, EVR: {new_evr['release']}, {new_evr_str}")`

		`+ return new_evr["release"]`
		`+`

		`def main(args):`
		`""" Main method. """`
		`@@ -220,4 +221,4 @@`
		`if args.algorithm == "sequential_builds":`
		`main_sequential_builds_algo(args)`
		`elif args.algorithm == "holistic_heuristic":`
		`- main_holistic_heuristic_algo(args)`
		`+ holistic_heuristic_algo(args.package, args.dist, args.evr)`

tests/__init__.py

file added

empty or binary file added

tests/koji_plugin/__init__.py

file added

empty or binary file added

tests/koji_plugin/test_rpmautospec_plugin.py

file added

+79

		`@@ -0,0 +1,79 @@`
		`+ import filecmp`
		`+ import os`
		`+ import tarfile`
		`+ import tempfile`
		`+ from unittest.mock import MagicMock`
		`+`
		`+ from koji_plugin.rpmautospec_plugin import autospec_cb`
		`+`
		`+`
		`+ __here__ = os.path.dirname(__file__)`
		`+`
		`+ commit = "5ab06967a36e72f66add9b6cfe08bd98f8900693"`
		`+ url = f"git+https://src.fedoraproject.org/rpms/dummy-test-package-gloster.git#{commit}"`
		`+`
		`+ data_scm_info = {`
		`+ "host": "src.fedoraproject.org",`
		`+ "module": "",`
		`+ "repository": "/rpms/dummy-test-package-gloster.git",`
		`+ "revision": "5ab06967a36e72f66add9b6cfe08bd98f8900693",`
		`+ "scheme": "git+https://",`
		`+ "scmtype": "GIT",`
		`+ "url": url,`
		`+ "user": None,`
		`+ }`
		`+`
		`+ data_build_tag = {"id": "dsttag", "tag_id": "fc32", "tag_name": "fc32"}`
		`+`
		`+`
		`+ class TestRpmautospecPlugin:`
		`+ """Test the koji_plugin.rpmautospec_plugin module"""`
		`+`
		`+ def test_autospec_cb(self):`
		`+ """Test the autospec_cb() function"""`
		`+ with tempfile.TemporaryDirectory() as workdir:`
		`+ with tarfile.open(`
		`+ os.path.join(`
		`+ __here__,`
		`+ os.path.pardir,`
		`+ "test-data",`
		`+ "repodata",`
		`+ "dummy-test-package-gloster_git.tar.gz"`
		`+ )`
		`+ ) as tar:`
		`+ tar.extractall(path=workdir)`
		`+`
		`+ koji_session = MagicMock()`
		`+ koji_session.getPackageID.return_value = 30489`
		`+ name = "dummy-test-package-gloster"`
		`+ builds = [`
		`+ {`
		`+ "epoch": None,`
		`+ "nvr": f"{name}-0-{x}.f32",`
		`+ "name": name,`
		`+ "release": f"{x}.fc32",`
		`+ "version": "0",`
		`+ }`
		`+ for x in range(2, 14)`
		`+ ]`
		`+ koji_session.listBuilds.return_value = builds`
		`+ args = ["postSCMCheckout"]`
		`+ kwargs = {`
		`+ "scminfo": data_scm_info,`
		`+ "build_tag": data_build_tag,`
		`+ "scratch": MagicMock(),`
		`+ "srcdir": os.path.join(workdir, "dummy-test-package-gloster"),`
		`+ "taskinfo": {"method": "buildSRPMFromSCM"},`
		`+ "session": koji_session,`
		`+ }`
		`+ autospec_cb(args, *kwargs)`
		`+ assert filecmp.cmp(`
		`+ os.path.join(kwargs["srcdir"], "dummy-test-package-gloster.spec"),`
		`+ os.path.join(`
		`+ __here__,`
		`+ os.path.pardir,`
		`+ "test-data",`
		`+ "repodata",`
		`+ "dummy-test-package-gloster.spec",`
		`+ ),`
		`+ )`

tests/rpmautospec/__init__.py

file added

empty or binary file added

tests/rpmautospec/test_release.py ~~tests/test_release.py~~

file renamed

+9 -1

		`@@ -39,7 +39,14 @@`
		`@pytest.mark.parametrize("test_data", data_as_test_parameters(test_data))`
		`def test_main(self, test_data, capsys):`
		`with open(`
		`- os.path.join(__here__, "koji-output", "list-builds", test_data["package"] + ".json"),`
		`+ os.path.join(`
		`+ __here__,`
		`+ os.path.pardir,`
		`+ "test-data",`
		`+ "koji-output",`
		`+ "list-builds",`
		`+ test_data["package"] + ".json",`
		`+ ),`
		`"rb",`
		`) as f:`
		`koji_list_builds_output = json.load(f)`
		`@@ -55,6 +62,7 @@`
		`main_args.package = test_data["package"]`
		`main_args.dist = test_data["dist"]`
		`main_args.evr = None`
		`+ main_args.koji_url = "http://192.0.2.1"`

		`release.main(main_args)`

tests/rpmautospec/test_tag_package.py ~~tests/test_tag_package.py~~

file renamed

file was moved with no change to the file

tests/test-data/koji-output/list-builds/gimp.json ~~tests/koji-output/list-builds/gimp.json~~

file renamed

file was moved with no change to the file

tests/test-data/repodata/dummy-test-package-gloster.spec

file added

+371

		`@@ -0,0 +1,371 @@`
		`+ %global function autorel() {`
		`+ return 14.fc32`
		`+ }`
		`+`
		`+ # Our dummy-test-packages are named after canary varieties, meet Gloster, Rubino and Crested`
		`+ # Source: https://www.omlet.co.uk/guide/finches_and_canaries/canary/canary_varieties`
		`+ Name: dummy-test-package-gloster`
		`+`
		`+ Version: 0`
		`+ Release: %{autorel}`
		`+ Summary: Dummy Test Package called Gloster`
		`+ License: CC0`
		`+ URL: http://fedoraproject.org/wiki/DummyTestPackages`
		`+`
		`+ # The tarball contains a file with an uuid to test later and a LICENSE`
		`+ Source0: %{name}-%{version}.tar.gz`
		`+`
		`+ BuildArch: noarch`
		`+`
		`+ %description`
		`+ This is a dummy test package for the purposes of testing if the Fedora CI`
		`+ pipeline is working. There is nothing useful here.`
		`+`
		`+ %prep`
		`+ %autosetup`
		`+`
		`+ %build`
		`+ # nothing to do`
		`+`
		`+ %install`
		`+ mkdir -p %{buildroot}%{_datadir}`
		`+ cp -p uuid %{buildroot}%{_datadir}/%{name}`
		`+`
		`+ %files`
		`+ %license LICENSE`
		`+ %{_datadir}/%{name}`
		`+`
		`+ %changelog`
		`+ * Fri Feb 28 2020 packagerbot <admin@fedoraproject.org> - 0-111`
		`+ - Bump release`
		`+`
		`+ * Fri Feb 28 2020 packagerbot <admin@fedoraproject.org> - 0-110`
		`+ - Bump release`
		`+`
		`+ * Fri Feb 28 2020 packagerbot <admin@fedoraproject.org> - 0-109`
		`+ - Bump release`
		`+`
		`+ * Fri Feb 28 2020 packagerbot <admin@fedoraproject.org> - 0-108`
		`+ - Bump release`
		`+`
		`+ * Fri Feb 28 2020 packagerbot <admin@fedoraproject.org> - 0-107`
		`+ - Bump release`
		`+`
		`+ * Fri Feb 28 2020 packagerbot <admin@fedoraproject.org> - 0-106`
		`+ - Bump release`
		`+`
		`+ * Fri Feb 28 2020 packagerbot <admin@fedoraproject.org> - 0-105`
		`+ - Bump release`
		`+`
		`+ * Fri Feb 28 2020 packagerbot <admin@fedoraproject.org> - 0-104`
		`+ - Bump release`
		`+`
		`+ * Fri Feb 28 2020 packagerbot <admin@fedoraproject.org> - 0-103`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 27 2020 packagerbot <admin@fedoraproject.org> - 0-102`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 27 2020 packagerbot <admin@fedoraproject.org> - 0-101`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 27 2020 packagerbot <admin@fedoraproject.org> - 0-100`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 27 2020 packagerbot <admin@fedoraproject.org> - 0-99`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 27 2020 packagerbot <admin@fedoraproject.org> - 0-98`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 27 2020 packagerbot <admin@fedoraproject.org> - 0-97`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 27 2020 packagerbot <admin@fedoraproject.org> - 0-96`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 27 2020 packagerbot <admin@fedoraproject.org> - 0-95`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 27 2020 packagerbot <admin@fedoraproject.org> - 0-94`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 27 2020 packagerbot <admin@fedoraproject.org> - 0-93`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 27 2020 packagerbot <admin@fedoraproject.org> - 0-92`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 27 2020 packagerbot <admin@fedoraproject.org> - 0-91`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 27 2020 packagerbot <admin@fedoraproject.org> - 0-90`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 26 2020 packagerbot <admin@fedoraproject.org> - 0-89`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 26 2020 packagerbot <admin@fedoraproject.org> - 0-88`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 26 2020 packagerbot <admin@fedoraproject.org> - 0-87`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 26 2020 packagerbot <admin@fedoraproject.org> - 0-86`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 26 2020 packagerbot <admin@fedoraproject.org> - 0-85`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 26 2020 packagerbot <admin@fedoraproject.org> - 0-84`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 26 2020 packagerbot <admin@fedoraproject.org> - 0-83`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 26 2020 packagerbot <admin@fedoraproject.org> - 0-82`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 26 2020 packagerbot <admin@fedoraproject.org> - 0-81`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 26 2020 packagerbot <admin@fedoraproject.org> - 0-80`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 26 2020 packagerbot <admin@fedoraproject.org> - 0-79`
		`+ - Bump release`
		`+`
		`+ * Tue Feb 25 2020 packagerbot <admin@fedoraproject.org> - 0-78`
		`+ - Bump release`
		`+`
		`+ * Tue Feb 25 2020 packagerbot <admin@fedoraproject.org> - 0-77`
		`+ - Bump release`
		`+`
		`+ * Tue Feb 25 2020 packagerbot <admin@fedoraproject.org> - 0-76`
		`+ - Bump release`
		`+`
		`+ * Tue Feb 25 2020 packagerbot <admin@fedoraproject.org> - 0-75`
		`+ - Bump release`
		`+`
		`+ * Tue Feb 25 2020 packagerbot <admin@fedoraproject.org> - 0-74`
		`+ - Bump release`
		`+`
		`+ * Tue Feb 25 2020 packagerbot <admin@fedoraproject.org> - 0-73`
		`+ - Bump release`
		`+`
		`+ * Mon Feb 24 2020 packagerbot <admin@fedoraproject.org> - 0-72`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 20 2020 packagerbot <admin@fedoraproject.org> - 0-71`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 20 2020 packagerbot <admin@fedoraproject.org> - 0-70`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 20 2020 packagerbot <admin@fedoraproject.org> - 0-69`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 20 2020 packagerbot <admin@fedoraproject.org> - 0-68`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 20 2020 packagerbot <admin@fedoraproject.org> - 0-67`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 20 2020 packagerbot <admin@fedoraproject.org> - 0-66`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 20 2020 packagerbot <admin@fedoraproject.org> - 0-65`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 20 2020 packagerbot <admin@fedoraproject.org> - 0-64`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 20 2020 packagerbot <admin@fedoraproject.org> - 0-63`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 20 2020 packagerbot <admin@fedoraproject.org> - 0-62`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 20 2020 packagerbot <admin@fedoraproject.org> - 0-61`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 20 2020 packagerbot <admin@fedoraproject.org> - 0-60`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 20 2020 packagerbot <admin@fedoraproject.org> - 0-59`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 20 2020 packagerbot <admin@fedoraproject.org> - 0-58`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 20 2020 packagerbot <admin@fedoraproject.org> - 0-57`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 20 2020 packagerbot <admin@fedoraproject.org> - 0-56`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 19 2020 packagerbot <admin@fedoraproject.org> - 0-55`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 19 2020 packagerbot <admin@fedoraproject.org> - 0-54`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 19 2020 packagerbot <admin@fedoraproject.org> - 0-53`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 19 2020 packagerbot <admin@fedoraproject.org> - 0-52`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 19 2020 packagerbot <admin@fedoraproject.org> - 0-51`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 19 2020 packagerbot <admin@fedoraproject.org> - 0-50`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 19 2020 packagerbot <admin@fedoraproject.org> - 0-49`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 19 2020 packagerbot <admin@fedoraproject.org> - 0-48`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 19 2020 packagerbot <admin@fedoraproject.org> - 0-47`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 19 2020 packagerbot <admin@fedoraproject.org> - 0-46`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 19 2020 packagerbot <admin@fedoraproject.org> - 0-45`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 19 2020 packagerbot <admin@fedoraproject.org> - 0-44`
		`+ - Bump release`
		`+`
		`+ * Wed Feb 19 2020 packagerbot <admin@fedoraproject.org> - 0-43`
		`+ - Bump release`
		`+`
		`+ * Tue Feb 18 2020 packagerbot <admin@fedoraproject.org> - 0-42`
		`+ - Bump release`
		`+`
		`+ * Tue Feb 18 2020 packagerbot <admin@fedoraproject.org> - 0-41`
		`+ - Bump release`
		`+`
		`+ * Tue Feb 18 2020 packagerbot <admin@fedoraproject.org> - 0-40`
		`+ - Bump release`
		`+`
		`+ * Tue Feb 18 2020 packagerbot <admin@fedoraproject.org> - 0-39`
		`+ - Bump release`
		`+`
		`+ * Thu Feb 13 2020 packagerbot <admin@fedoraproject.org> - 0-38`
		`+ - Bump release`
		`+`
		`+ * Thu Jan 30 2020 Adam Saleh <asaleh@redhat.com> - 0-37`
		`+ - Bump release`
		`+`
		`+ * Thu Jan 30 2020 Adam Saleh <asaleh@redhat.com> - 0-36`
		`+ - Bump release`
		`+`
		`+ * Thu Jan 30 2020 Adam Saleh <asaleh@redhat.com> - 0-35`
		`+ - Bump release`
		`+`
		`+ * Thu Jan 30 2020 Adam Saleh <asaleh@redhat.com> - 0-34`
		`+ - Bump release`
		`+`
		`+ * Thu Jan 30 2020 Adam Saleh <asaleh@redhat.com> - 0-33`
		`+ - Bump release`
		`+`
		`+ * Wed Jan 29 2020 Adam Saleh <asaleh@redhat.com> - 0-32`
		`+ - Bump release`
		`+`
		`+ * Tue Jan 28 2020 Adam Saleh <asaleh@redhat.com> - 0-31`
		`+ - Bump release`
		`+`
		`+ * Tue Jan 28 2020 Adam Saleh <asaleh@redhat.com> - 0-30`
		`+ - Bump release`
		`+`
		`+ * Tue Jan 28 2020 Adam Saleh <asaleh@redhat.com> - 0-29`
		`+ - Bump release`
		`+`
		`+ * Tue Jan 28 2020 Adam Saleh <asaleh@redhat.com> - 0-28`
		`+ - Bump release`
		`+`
		`+ * Tue Jan 28 2020 Adam Saleh <asaleh@redhat.com> - 0-27`
		`+ - Bump release`
		`+`
		`+ * Tue Jan 28 2020 Adam Saleh <asaleh@redhat.com> - 0-26`
		`+ - Bump release`
		`+`
		`+ * Tue Jan 28 2020 Adam Saleh <asaleh@redhat.com> - 0-25`
		`+ - Bump release`
		`+`
		`+ * Tue Jan 28 2020 Adam Saleh <asaleh@redhat.com> - 0-24`
		`+ - Bump release`
		`+`
		`+ * Mon Jan 27 2020 Adam Saleh <asaleh@redhat.com> - 0-23`
		`+ - Bump release`
		`+`
		`+ * Mon Jan 27 2020 Adam Saleh <asaleh@redhat.com> - 0-22`
		`+ - Bump release`
		`+`
		`+ * Sat Jan 25 2020 Pierre-Yves Chibon <pingou@pingoured.fr> - 0-21`
		`+ - Bump release`
		`+`
		`+ * Sat Jan 25 2020 Pierre-Yves Chibon <pingou@pingoured.fr> - 0-20`
		`+ - Bump release`
		`+`
		`+ * Sat Jan 25 2020 Pierre-Yves Chibon <pingou@pingoured.fr> - 0-19`
		`+ - Bump release`
		`+`
		`+ * Sat Jan 25 2020 Pierre-Yves Chibon <pingou@pingoured.fr> - 0-18`
		`+ - Bump release`
		`+`
		`+ * Sat Jan 25 2020 Pierre-Yves Chibon <pingou@pingoured.fr> - 0-17`
		`+ - Bump release`
		`+`
		`+ * Sat Jan 25 2020 Pierre-Yves Chibon <pingou@pingoured.fr> - 0-16`
		`+ - Bump release`
		`+`
		`+ * Fri Jan 24 2020 Pierre-Yves Chibon <pingou@pingoured.fr> - 0-15`
		`+ - Bump release`
		`+`
		`+ * Fri Jan 24 2020 Pierre-Yves Chibon <pingou@pingoured.fr> - 0-14`
		`+ - Bump release`
		`+`
		`+ * Fri Jan 24 2020 Pierre-Yves Chibon <pingou@pingoured.fr> - 0-13`
		`+ - Bump release`
		`+`
		`+ * Fri Jan 24 2020 Pierre-Yves Chibon <pingou@pingoured.fr> - 0-12`
		`+ - Bump release`
		`+`
		`+ * Fri Jan 24 2020 Pierre-Yves Chibon <pingou@pingoured.fr> - 0-11`
		`+ - Bump release`
		`+`
		`+ * Fri Jan 24 2020 Pierre-Yves Chibon <pingou@pingoured.fr> - 0-10`
		`+ - Bump release`
		`+`
		`+ * Fri Jan 24 2020 Pierre-Yves Chibon <pingou@pingoured.fr> - 0-9`
		`+ - Bump release`
		`+`
		`+ * Tue Jan 21 2020 Pierre-Yves Chibon <pingou@pingoured.fr> - 0-8`
		`+ - Bump release`
		`+`
		`+ * Tue Jan 21 2020 Pierre-Yves Chibon <pingou@pingoured.fr> - 0-7`
		`+ - Bump release`
		`+`
		`+ * Tue Jan 21 2020 Pierre-Yves Chibon <pingou@pingoured.fr> - 0-6`
		`+ - Bump release`
		`+`
		`+ * Tue Jan 21 2020 Pierre-Yves Chibon <pingou@pingoured.fr> - 0-5`
		`+ - Bump release`
		`+`
		`+ * Thu Jan 16 2020 Pierre-Yves Chibon <pingou@pingoured.fr> - 0-4`
		`+ - Bump release`
		`+`
		`+ * Fri Jan 10 2020 Pierre-Yves Chibon <pingou@pingoured.fr> - 0-3`
		`+ - Bump release`
		`+`
		`+ * Fri Jan 10 2020 Pierre-Yves Chibon <pingou@pingoured.fr> - 0-2`
		`+ - Bump release`
		`+`
		`+ * Thu Jan 09 2020 Pierre-Yves Chibon <pingou@pingoured.fr> - 0-1`
		`+ - Initial import post review`
		`+`

tests/test-data/repodata/dummy-test-package-gloster_git.tar.gz

file added

empty or binary file added

asaleh commented 4 years ago

It has a test even :)

Metadata Update from @nphilipp:
- Request assigned

4 years ago

nphilipp commented 4 years ago

Some general things I found:

IMO the name plugin is a little generic, let's use koji_plugin for the directory containing the plugin code, then someone new will know right away what's it about.
There are Python byte-compiled files and what looks like the dummy-test-package-gloster.spec file from the dummy repo tarball with an expanded %{autochangelog} which we don't want in the repository. We don't have a .gitignore file yet which would exclude this and the usual suspects (*.py?, __pycache__/, ...), to take care of that in the future, would you add it?
I know it's small change :wink: but the tarball shouldn't be gzipped, it actually takes up more space in the git repo than when it's uncompressed. Also it shouldn't be in the same place as
There are some PEP8 style issues (long lines, whitespace issues, wrong order of imports, unused imports, ...) which I won't list here, just change what flake8 and pyflakes are complaining about. :wink:

nphilipp commented on line 9 of plugin/rpmautospec_plugin.py 4 years ago

If we expect srcdir to always be present, let's just add it to the function header. ~~Does koji even pass positional arguments to plugin callbacks? If not,~~ Let's add srcdir as a mandatory keyword-only argument:

@callback('postSCMCheckout')
def autospec_cb(cbtype, *, srcdir, *args, **kwargs): 
    name = ...

Edited 4 years ago by nphilipp

nphilipp commented on line 44 of rpmautospec/changelog.py 4 years ago

Rather than mkdtemp() ... shutil.rmtree() and copying over the whole worktree, let's use tempfile.TemporaryDirectory as a context manager. Also, cope with trailing slashes while we're at it :wink::

def produce_changelog(repopath):
    repopath = repopath.rstrip(os.path.sep)
    with tempfile.TemporaryDirectory() as workdir:
        name = os.path.basename(repopath)
        ...
                print("No more spec file, bailing")
                break

    for nvr, commits in data.items():
    ...

This context manager takes care of cleaning up the temporary directory even if an exception happens.

I'm also not sure if we should do the equivalent of a "git clone --bare" rather than copying the whole repository (with all tracked and untracked files that might be in there), but we don't seem to have our mind made up yet if we can continue using the pygit2 module, or should stick to executing git directly. @pingou?

Edited 4 years ago by nphilipp

nphilipp commented on line 2 of tests/test_plugin_callback.py 4 years ago

This isn't used here.

nphilipp commented on line 28 of tests/test_plugin_callback.py 4 years ago

Do data_scratch, data_taskinfo and data_session have to be global variables? We could just set these directly in kwargs in the method.

nphilipp commented on line 33 of tests/test_plugin_callback.py 4 years ago

As above, use the tempfile.TemporaryDirectory() context manager, so that the temporary directory will be deleted after the test is run.

nphilipp commented on line 39 of tests/test_plugin_callback.py 4 years ago

Ah, there's the positional argument. :grinning:

asaleh commented 4 years ago

Apart from the "why don't we change arguments of the callback, if we know i.e. srcdir is always present", I am working on incorporating the suggested chages (and will create the damned .gitignore!)

With the callback, I might go in the opposite direction, looking if all the necessary keys are present, and if not, just logging an error and bailing?

rebased onto b8473a16f7b0903f30068765279ab49648381a2c

4 years ago

asaleh commented 4 years ago

One more thing, the compiled spec-file is for testing.

rebased onto c4601084a131589835903609dbd12faadf57be73

4 years ago

rebased onto e713cee5f76785061f92755377edbd3302586e72

4 years ago

nphilipp commented 4 years ago

Imports aren't in the right order, and there's a spurious blank line between them:

nils@gibraltar:~/src/fedora-infra/rpmautospec/asaleh (asaleh--koji_callback)> flake8 .
./rpmautospec/release.py:7:1: I100 Import statements are in the wrong order. 'from collections import defaultdict' should be before 'import typing'
from collections import defaultdict
^
./rpmautospec/misc.py:2:1: I100 Import statements are in the wrong order. 'from functools import cmp_to_key' should be before 'import re'
from functools import cmp_to_key
^
./koji_plugin/rpmautospec_plugin.py:2:1: I100 Import statements are in the wrong order. 'from collections import namedtuple' should be before 'import os'
from collections import namedtuple
^
./tests/test_plugin_callback.py:7:1: I100 Import statements are in the wrong order. 'from koji_plugin.rpmautospec_plugin import autospec_cb' should be before 'from mock import MagicMock'
from koji_plugin.rpmautospec_plugin import autospec_cb
^
./tests/test_plugin_callback.py:7:1: I202 Additional newline in a group of imports. 'from koji_plugin.rpmautospec_plugin import autospec_cb' is identified as Third Party and 'from mock import MagicMock' is identified as Third Party.
from koji_plugin.rpmautospec_plugin import autospec_cb
^
nils@gibraltar:~/src/fedora-infra/rpmautospec/asaleh (asaleh--koji_callback)>

nphilipp commented on line 18 of koji_plugin/rpmautospec_plugin.py 4 years ago

This won't do what you wanted: the string %{autorel} will always evaluate to True, so it would match with any valid spec file (because it has a Release: line in it). To cover what's allowed in terms of spec file syntax (e.g. Release is not case-sensitive and can have white space between it and :, with or without curly braces around the autorel macro, which is case-sensitive) , I'd suggest using a regular expression:

import re
...
autorel_re = re.compile(r"^\s+(?i:Release)\s+:\s+%(?:autorel(?:\s|$)|\{autorel[^\}]*\})")


def is_autorel(line):
    return autorel_re.match(line)

nphilipp commented on line 14 of koji_plugin/rpmautospec_plugin.py 4 years ago

The name is deceptive, fn is usually some kind of function. This here is a string template that becomes an (RPM macro) function when expanded and inserted, so let's call it something like autorel_fn_template which is more explicit than implicit. Try python -c "import this" if you don't know this easter egg yet. :wink:
Also, let's use a name for the placeholder in the template, e.g.: return {automatic_release_value}

nphilipp commented on line 23 of koji_plugin/rpmautospec_plugin.py 4 years ago

This would recreate the Args type/class every time the function is called, let's move it outside. Ideally, I'd like to fix the interface of the main...() function though -- using argparse arguments isn't a good one here, it only works well for the CLI case.

Edited 4 years ago by nphilipp

nphilipp commented on line 36 of koji_plugin/rpmautospec_plugin.py 4 years ago

This isn't necessary because lines is set below.

nphilipp commented on line 37 of koji_plugin/rpmautospec_plugin.py 4 years ago

This makes both srcdir and session mandatory keyword arguments (and `cb_type is an always present positional one), it's better to make that explicit in the function declaration, as discussed:

@callback("postSCMCheckout")
def autospec_cb(cb_type, *, srcdir, session, *args, **kwargs):
    repo_path = srcdir
    ...
    new_rel = get_autorel(name, dist, session)

1 new commit added

Problem with trailing whitespace in test-data.

4 years ago

nphilipp commented on line 39 of koji_plugin/rpmautospec_plugin.py 4 years ago

This is a pretty odd way to avoid lines = specfile.readlines(). :wink:

To make processing easier and be a bit more lenient when it comes to whitespace, we could strip off the trailing newline character here and add it back when writing the file:

        lines = [l.rstrip('\n') for l in specfile]
        ...
        autospec = (
            lines[-1].strip() in ("%autochangelog", "%{autochangelog}")
            and lines[-2].strip() == "%changelog"
        )

This doesn't allow blank lines, trailing or between %changelog and %autochangelog, but that shouldn't be hard to add.

nphilipp commented on line 46 of koji_plugin/rpmautospec_plugin.py 4 years ago

To match changes from above, and use that lists can be modified:

    if autorel:
        lines.insert(0, autorel_fn_template.format(automatic_release_value=new_rel))

Edited 4 years ago by nphilipp

nphilipp commented on line 49 of koji_plugin/rpmautospec_plugin.py 4 years ago

Assuming produce_changelog() returns a list of lines (need to check if they have trailing newlines themselves), this should work: lines += produce_changelog(repo_path)

Edited 4 years ago by nphilipp

nphilipp commented on line 52 of koji_plugin/rpmautospec_plugin.py 4 years ago

To accommodate that the lines in the list don't have trailing newlines after the above changes: specfile.writelines(l + "\n" for l in lines)

nphilipp commented on line 24 of rpmautospec/changelog.py 4 years ago

This is unnecessary when using TemporaryDirectory() below.

nphilipp commented on line 58 of rpmautospec/changelog.py 4 years ago

Let's avoid reusing the same variable for two different things and computing the same string twice:

def produce_changelog(orig_repopath):
    name = os.path.basename(orig_repopath)
    with tempfile.TemporaryDirectory() as workdir:
        repopath = f"{workdir}/{name}"
        shutil.copytree(orig_repopath, repopath)

nphilipp commented on line 146 of rpmautospec/changelog.py 4 years ago

We don't need/want the newline here with the changes above.

nphilipp commented on line 159 of rpmautospec/changelog.py 4 years ago

Same here (no newlines).

nphilipp commented on line 168 of rpmautospec/changelog.py 4 years ago

This would insert empty lines between every line of the output, without the newlines removed above. Could also be this: print("\n".join(produce_changelog(repopath)))

nphilipp commented on line 26 of rpmautospec/misc.py 4 years ago

To accommodate passing in an existing session, let's just use one positional arg (and complete type hints):

from typing import Union
...
def koji_init(koji_url_or_session: Union[str, koji.ClientSession]) -> koji.ClientSession:
    global _kojiclient
    if isinstance(koji_url_or_session, str):
        _kojiclient = koji.ClientSession(koji_url_or_session)
    else:
        _kojiclient = koji_url_or_session
    return _kojiclient

Edited 4 years ago by nphilipp

nphilipp commented on line 22 of koji_plugin/rpmautospec_plugin.py 4 years ago

Adapt to my suggestion below: koji_init(session)

nphilipp commented on line 19 of rpmautospec/release.py 4 years ago

I have a hunch your precommit hook thingy got a little overboard here... We use the google import order style (as per setup.cfg), so mixing import ... and from ... import ... is licit, as is multiple modules imported on one line.

nphilipp commented on line 28 of rpmautospec/release.py 4 years ago

We probably should do something about all the print() calls. We only want them run in the CLI, but this could be out of scope for this PR.

nphilipp commented on line 9 of tests/test_plugin_callback.py 4 years ago

Here should be two blank lines instead of one, not sure why flake8 doesn't flag it...

nphilipp commented on line 6 of tests/test_plugin_callback.py 4 years ago

This should be from unittest.mock import MagicMock, the (toplevel) mock package is a third party one and could be missing from the system this is run on. It's basically the same thing as unittest.mock anyway.

nphilipp commented on line 31 of tests/test_plugin_callback.py 4 years ago

I guess this got under the bus from my previous review: If we don't care for what happens to these values (data_scratch, data_taskinfo, data_session), let's just use the values in the code.

Also, remove data_srcdir, we don't seem to use it.

Edited 4 years ago by nphilipp

nphilipp commented on line 39 of tests/test_plugin_callback.py 4 years ago

As above, use the respective context managers so everything gets cleaned up automatically afterwards:

    def test_callback(self):
        with tempfile.TemporaryDirectory() as workdir, tarfile.open(
            "tests/repodata/dummy-test-package-gloster_git.tar.gz"
        ) as tar:
            tar.extractall(path=workdir)

            ...

nphilipp commented on line 40 of tests/test_plugin_callback.py 4 years ago

Do we need this?

nphilipp commented on line 62 of tests/test_plugin_callback.py 4 years ago

See above regarding data_scratch, data_taskinfo, data_session. Having these additional parameters in let's us verify the callback copes with these additional parameters. :thumbsup:

1 new commit added

Incorporating improvements from comments.

4 years ago

1 new commit added

Incorporating comments from tkopecek

4 years ago

nphilipp commented 4 years ago

As we discussed, I dumped a load change requests on you and some of it it just got under the bus. In order to not hold up the PR unduly, I'll summarize here so we don't lose track and can deal with it later:

Use e.g. a regex to detect the %autorel macro being used in all the permitted forms of the Release: line.
The name template is a little better than fn but it doesn't tell me what it's a template for. :wink:
Make the logic to detect that an automatic changelog is desired a little more lenient.
The odd print() which could be converted to a logging call.
Consistent typing hints where the release API was refactored.

Things I'll change myself before merging are:

The order of package, dist in the refactored functions in release, that should reflect the "traditional" order, which is how it's in the CLI: package (name), then dist.
The main_holistic_heuristic_algo() wrapper can go, the main() function can just call it directly.
Use 192.0.2.1 as a guaranteed unreachable host in tests, not names like koji which may or may not resolve to real hosts.
Use the tarfile context manager in, drop data_taskinfo, data_session as variables from the tests (but keep them in kwargs)
Structure names of test directories, files, classes etc. to mirror the names of the code being tested. I.e.: tests/test_calc_release.py becomes tests/rpmautospec/test_release.py, tests/test_plugin_callback.py becomes tests/koji_plugin/test_rpmautospec_plugin.py and TestAutospecCallback becomes TestRpmautospecPlugin.