PR#2979: Add a utility function to watch builds - koji

koji

#2979 Add a utility function to watch builds

Closed 2 years ago by tkopecek. Opened 2 years ago by oturpe.

oturpe/koji watch-builds-binding into master

Add utility function to watch builds

Otto Urpelainen • 2 years ago

a42eac6

Add unit test for buildLabel

Otto Urpelainen • 2 years ago

c940e6b

cli/koji_cli/commands.py

file modified

+7 -34

		`@@ -44,6 +44,7 @@`
		`print_task_recurse,`
		`unique_path,`
		`warn,`
		`+ watch_builds,`
		`watch_logs,`
		`watch_tasks,`
		`truncate_string`
		`@@ -7339,8 +7340,6 @@`
		`"value only")`
		`(suboptions, args) = parser.parse_args(args)`

		`- start = time.time()`
		`-`
		`builds = [koji.parse_NVR(build) for build in suboptions.builds]`
		`if len(args) < 1:`
		`parser.error("Please specify a tag name")`
		`@@ -7382,38 +7381,12 @@`
		`warn("nvr %s is not current in tag %s\n latest build in %s is %s" %`
		`(expected_nvr, tag, tag, present_nvr))`

		`- last_repo = None`
		`- repo = session.getRepo(tag_id)`
		`-`
		`- while True:`
		`- if builds and repo and repo != last_repo:`
		`- if koji.util.checkForBuilds(session, tag_id, builds, repo['create_event'],`
		`- latest=True):`
		`- if not suboptions.quiet:`
		`- print("Successfully waited %s for %s to appear in the %s repo" %`
		`- (koji.util.duration(start), koji.util.printList(suboptions.builds), tag))`
		`- return`
		`-`
		`- if (time.time() - start) >= (suboptions.timeout * 60.0):`
		`- if not suboptions.quiet:`
		`- if builds:`
		`- error("Unsuccessfully waited %s for %s to appear in the %s repo" %`
		`- (koji.util.duration(start), koji.util.printList(suboptions.builds), tag))`
		`- else:`
		`- error("Unsuccessfully waited %s for a new %s repo" %`
		`- (koji.util.duration(start), tag))`
		`- error()`
		`-`
		`- time.sleep(options.poll_interval)`
		`- last_repo = repo`
		`- repo = session.getRepo(tag_id)`
		`-`
		`- if not builds:`
		`- if repo != last_repo:`
		`- if not suboptions.quiet:`
		`- print("Successfully waited %s for a new %s repo" %`
		`- (koji.util.duration(start), tag))`
		`- return`
		`+ try:`
		`+ watch_builds(`
		`+ session, tag_id, builds, quiet=suboptions.quiet,`
		`+ poll_interval=options.poll_interval, timeout=suboptions.timeout)`
		`+ except koji.GenericError as e:`
		`+ error(str(e))`
ferdnyc commented 2 years ago (IOW, regarding my previous comment, it would be better to always include the explanatory text in the exception, and change the call here to be something like...) try: watch_builds(...) except koji.GenericError as e: error(str(e) if not suboptions.quiet else "") Edited 2 years ago by ferdnyc


		`def handle_regen_repo(options, session, args):`

cli/koji_cli/lib.py

file modified

+57

		`@@ -819,6 +819,63 @@`
		`return tasklist`


		`+ def watch_builds(session, tag_id, builds, quiet=False, poll_interval=5, timeout=120):`
		`+ """Watch for given builds to appear in given tag. If no build are given,`
		`+ watch for new repo for given tag.`
		`+`
		`+ :param session: Koji session object`
		`+ :param tag_id: Tag id`
		`+ :param builds: List of builds as NVR dicts`
		`+ :param quiet: no/verbose`
		`+ :param poll_interval: Poll interval in seconds`
		`+ :param timeout: Watch timeout in minutes"""`
		`+ last_repo = None`
		`+ repo = session.getRepo(tag_id)`
		`+`
		`+ # String representations for logs and exceptions`
		`+ builds_str = koji.util.printList(`
		`+ [koji.buildLabel(build) for build in builds])`
		`+ tag_info = session.getTag(tag_id)`
		`+ if not tag_info:`
		`+ raise koji.GenericError("No tag with id: %s" % tag_id)`
		`+ tag_name = tag_info['name']`
		`+`
		`+ start = time.time()`
		`+ while True:`
		`+ if builds and repo and repo != last_repo:`
		`+ if koji.util.checkForBuilds(session, tag_id, builds,`
		`+ repo['create_event'], latest=True):`
		`+ if not quiet:`
		`+ print("Successfully waited %s for %s "`
ferdnyc commented 2 years ago Since the stated goal here is to make `watch_build` available to external tool implementations like bodhi, etc., would it make more sense to have `quiet=True` as the default, so that callers won't be surprised by status messages on the terminal unless they explicitly set `quiet=True` when calling `watch_build()`?
		`+ "to appear in the %s repo" %`
		`+ (koji.util.duration(start), builds_str, tag_name))`
		`+ return`
		`+`
		`+ if (time.time() - start >= timeout * 60.0):`
		`+ if not quiet:`
		`+ if builds:`
		`+ raise koji.GenericError(`
		`+ "Unsuccessfully waited %s for %s "`
		`+ "to appear in the %s repo" %`
		`+ (koji.util.duration(start), builds_str, tag_name))`
		`+ else:`
		`+ raise koji.GenericError(`
		`+ "Unsuccessfully waited %s for a new %s repo" %`
		`+ (koji.util.duration(start), tag_name))`
		`+ raise koji.GenericError()`
ferdnyc commented 2 years ago Having the `quiet` argument control the text content of the exception raised is... very weird. I would think that the exception should always be raised with all appropriate detail; the caller can determine whether or not to output that content based on their `quiet` status. (Even better would be to have more precise exceptions defined, where this could be e.g. a `koji.TimeoutError` derived from `koji.GenericError`. That way the caller can handle the different failure conditions differently.)
		`+`
		`+ time.sleep(poll_interval)`
		`+ last_repo = repo`
		`+ repo = session.getRepo(tag_id)`
		`+`
		`+ if not builds:`
		`+ if repo != last_repo:`
		`+ if not quiet:`
		`+ print("Successfully waited %s for a new %s repo" %`
		`+ (koji.util.duration(start), tag_name))`
		`+ return`
		`+`
		`+`
		`def format_inheritance_flags(parent):`
		`"""Return a human readable string of inheritance flags"""`
		`flags = ''`

tests/test_lib/test_build_label.py

file added

+72

		`@@ -0,0 +1,72 @@`
		`+ from __future__ import absolute_import`
		`+ import mock`
		`+ import os`
		`+ import rpm`
		`+ import unittest`
		`+`
		`+ import koji`
		`+`
		`+ class TestBuildLabel(unittest.TestCase):`
		`+ def test_buildLabel(self):`
		`+ """Test the buildLabel method"""`
		`+`
		`+ self.assertRaises(AttributeError, koji.buildLabel, None)`
		`+ self.assertRaises(AttributeError, koji.buildLabel, 1)`
		`+ self.assertRaises(AttributeError, koji.buildLabel, [])`
		`+`
		`+ input = {}`
		`+ ret = koji.buildLabel(input)`
		`+ self.assertEqual(ret, "None-None-None")`
		`+`
		`+ input = {"name": "foo"}`
		`+ ret = koji.buildLabel(input)`
		`+ self.assertEqual(ret, "foo-None-None")`
		`+`
		`+ input = {"version": "1.0.2"}`
		`+ ret = koji.buildLabel(input)`
		`+ self.assertEqual(ret, "None-1.0.2-None")`
		`+`
		`+ input = {"release": "2"}`
		`+ ret = koji.buildLabel(input)`
		`+ self.assertEqual(ret, "None-None-2")`
		`+`
		`+ input = {"name": "foo", "version": "1.0.2"}`
		`+ ret = koji.buildLabel(input)`
		`+ self.assertEqual(ret, "foo-1.0.2-None")`
		`+`
		`+ input = {"name": "foo", "release": "2"}`
		`+ ret = koji.buildLabel(input)`
		`+ self.assertEqual(ret, "foo-None-2")`
		`+`
		`+ input = {"version": "1.0.2", "release": "2"}`
		`+ ret = koji.buildLabel(input)`
		`+ self.assertEqual(ret, "None-1.0.2-2")`
		`+`
		`+ input = {"name": "foo", "version": "1.0.2", "release": "2"}`
		`+ ret = koji.buildLabel(input)`
		`+ self.assertEqual(ret, "foo-1.0.2-2")`
		`+`
		`+ input = {"package_name": "bar", "version": "1.0.2", "release": "2"}`
		`+ ret = koji.buildLabel(input)`
		`+ self.assertEqual(ret, "bar-1.0.2-2")`
		`+`
		`+ input = {`
		`+ "package_name": "bar",`
		`+ "name": "foo",`
		`+ "version": "1.0.2",`
		`+ "release": "2"`
		`+ }`
		`+ ret = koji.buildLabel(input)`
		`+ self.assertEqual(ret, "bar-1.0.2-2")`
		`+`
		`+ input = {"epoch": 7, "name": "foo", "version": "1.0.2", "release": "2"}`
		`+ ret = koji.buildLabel(input)`
		`+ self.assertEqual(ret, "foo-1.0.2-2")`
		`+`
		`+ input = {"epoch": 7, "name": "foo", "version": "1.0.2", "release": "2"}`
		`+ ret = koji.buildLabel(input, True)`
		`+ self.assertEqual(ret, "7:foo-1.0.2-2")`
		`+`
		`+ input = {"name": "foo", "version": "1.0.2", "release": "2"}`
		`+ ret = koji.buildLabel(input, True)`
		`+ self.assertEqual(ret, "foo-1.0.2-2")`

oturpe commented 2 years ago

External tools that interact with Koji
need to wait for builds to appear in a repo.
For example, Fedora cli tools 'bodhi' and 'fedpkg'
can create buildroot overrides,
which only become useful
after the override's build appears in the correct repo.
At the moment, 'bodhi' waits for a repo
by invoking the 'koji wait-repo' cli tool
while 'fedpkg' does not wait at all.
In order to make it easier for such tools to wait for a repo,
the wait implementation from 'koji wait-repo'
is moved to koji.utils namespace
where it is available for use through Python import.

oturpe commented 2 years ago

Tests for the new function watchBuilds are still missing.
I intend to add them,
but since this my first contribution to Koji,
I would like to get a round of review first,
so my approach is validated
and I will not end up spending time in
writing tests for a feature that will not be merged.

tkopecek commented on line 5 of koji/__init__.py 2 years ago

buildLabel does this (with option to not/show epoch) Also note that epoch is integer, not string.

Edited 2 years ago by tkopecek

tkopecek commented 2 years ago

This is not the right library - watch_builds is CLI thing, so it should be in the CLI lib. Moving to koji_cli/lib.py is ok (it is also importable)

tkopecek commented 2 years ago

tracking issue #2981

oturpe commented 2 years ago

buildLabel does this (with option to not/show epoch) Also note that epoch is integer, not string.

Nice, I was looking for existing implementation,
for some reason I did not spot buildLabel.
I will use that instead.

Now that I already wrote some unit tests,
and buildLabel apparently did not have any,
I will move the new tests to test buildLabel.

rebased onto c940e6b

2 years ago

ferdnyc commented on line 32 of cli/koji_cli/lib.py 2 years ago

Since the stated goal here is to make watch_build available to external tool implementations like bodhi, etc., would it make more sense to have quiet=True as the default, so that callers won't be surprised by status messages on the terminal unless they explicitly set quiet=True when calling watch_build()?

ferdnyc commented on line 48 of cli/koji_cli/lib.py 2 years ago

Having the quiet argument control the text content of the exception raised is... very weird. I would think that the exception should always be raised with all appropriate detail; the caller can determine whether or not to output that content based on their quiet status. (Even better would be to have more precise exceptions defined, where this could be e.g. a koji.TimeoutError derived from koji.GenericError. That way the caller can handle the different failure conditions differently.)

ferdnyc commented on line 59 of cli/koji_cli/commands.py 2 years ago

(IOW, regarding my previous comment, it would be better to always include the explanatory text in the exception, and change the call here to be something like...)

try:
    watch_builds(...)
except koji.GenericError as e:
    error(str(e) if not suboptions.quiet else "")

Edited 2 years ago by ferdnyc

tkopecek commented 2 years ago

I've added some changes in #3406

Pull-Request has been closed by tkopecek

2 years ago

Metadata

Assignee

None

Tags

No Tags

Changes Summary 3

+7 -34

file changed

cli/koji_cli/commands.py

file added

tests/test_lib/test_build_label.py

koji

Source Code

Documentation

#2979 Add a utility function to watch builds Closed 2 years ago by tkopecek. Opened 2 years ago by oturpe. oturpe/koji watch-builds-binding into master

Metadata

Changes Summary 3

#2979 Add a utility function to watch builds

Closed 2 years ago by tkopecek. Opened 2 years ago by oturpe.

oturpe/koji watch-builds-binding into master