Differential D148

Add rpm_to_build() method to koji_utils
ClosedPublic
Actions

Authored by jskladan on Jun 23 2014, 10:25 AM.

Details

Reviewers
kparal
tflink
Maniphest Tasks
T195: report depcheck results per update
Commits
rLTRNb420fe644aea: Add rpm_to_build() method to koji_utils
Summary

This method is required for depcheck's squashing of per-rpm results
to per-update results.
It takes a rpm filename on input and returns build info from koji.
It has "caching/prefetching" capabilities to speed-up subsequent queries.

Test Plan

Tested with Depcheck several times

Diff Detail

Repository
rLTRN libtaskotron
Lint
Lint Skipped
Unit
Unit Tests Skipped
jskladan retitled this revision from to Add rpm_to_build() method to koji_utils.Jun 23 2014, 10:25 AM
jskladan updated this object.
jskladan edited the test plan for this revision. (Show Details)
jskladan added reviewers: tflink, kparal.
jskladan added subscribers: mkrizek, pschindl.
tflink requested changes to this revision.Jun 23 2014, 11:37 AM

I'd like to see some unit test coverage for the new method but the concept seems sound.

This revision now requires changes to proceed.Jun 23 2014, 11:37 AM
jskladan updated this revision to Diff 431.Jun 23 2014, 12:13 PM
  • added tests
tflink accepted this revision.Jun 23 2014, 12:18 PM

Looks good to me

This revision is now accepted and ready to land.Jun 23 2014, 12:18 PM
jskladan closed this revision.Jun 23 2014, 12:48 PM
jskladan updated this revision to Diff 436.

Closed by commit rLTRNb420fe644aea (authored by @jskladan).

kparal added a comment.Jun 23 2014, 1:01 PM

Too late, already pushed. But here are my comments.

libtaskotron/koji_utils.py
36

Please document what this is for and how does it look inside. It's very unpleasant if one has to find that out from reading the code scattered around. Either use:

# comment
self._rpm_to_build_cache = {}

or

self._rpm_to_build_cache = {}
''' a docstring '''
38

I understand the reasoning behind prefetch, but I'm a bit concerned whether it is helpful enough. RPM searches in Koji take a long time. If there are several dozens of builds to run through depcheck, it will take a really long time, no matter buildinfo caching.

I think this should rather take a list of rpm filenames as an input, and then make all the queries at once using Koji's multiCall. I imagine this will be at least 10x faster for larger sets of RPMs. You can have a look at upgradepath and see how multicall is used.

I don't say it has to be implemented right now, but I think it would be highly preferable. If you decide to stay with current implementation because of time constraints, please at least file a ticket?

41

Use need to use double backticks, if you're trying to highlight the text as code/value.

45

Return type is documented, but return value is not. Let's say something like:

:return: Koji buildinfo dictionary (as returned e.g. from :meth:`koji.getBuild`)
45
:rtype: :class:`bunch.Bunch`
46

I was a bit confused by the phrase "or build", because I don't specify "build" anywhere. Let's use "or its build", "or its related build" or something along the lines.

48

Something like # strip path would help a bit for those of us who don't know the whole Python library by heart ;)

jskladan added inline comments.Jun 23 2014, 1:10 PM
libtaskotron/koji_utils.py
36

Ooint taken, although I do not really see the need to document its contents as this is to be used only in the one piece of code, and the variable is marked as "private". But I guess there will be no harm in it.

38

I can file the ticket, although from what I tried, this helps quite a lot.

41

thx

46

Thx, I'll be your SPHINX padawan soon enough :)

48

*shrugs* OK, I can comment on that line, although # grab filename from path says more about the intent, I guess

kparal added a comment.Jun 23 2014, 1:33 PM

Resolved in D152.

Revision Contents

PathPackages
Mlibtaskotron/koji_utils.py (34 lines)
Mtesting/test_koji_utils.py (37 lines)

Diff 436

View Options

libtaskotron/koji_utils.py

Show All 27 Lines27 def __init__(self, koji_session=None):
28 '''Create a new KojiClient 28​ '''Create a new KojiClient
29 29​
30 :param koji_session: an existing Koji session instance or ``None`` if 30​ :param koji_session: an existing Koji session instance or ``None`` if
31 you want a new default session to be created 31​ you want a new default session to be created
32 :type koji_session: :class:`koji.ClientSession` 32​ :type koji_session: :class:`koji.ClientSession`
33 ''' 33​ '''
34 self.session = (koji_session or 34​ self.session = (koji_session or
35 koji.ClientSession(config.get_config().koji_url)) 35​ koji.ClientSession(config.get_config().koji_url))
36​ self._rpm_to_build_cache = {}
kparalUnsubmitted
Not Done

Please document what this is for and how does it look inside. It's very unpleasant if one has to find that out from reading the code scattered around. Either use:

# comment
self._rpm_to_build_cache = {}

or

self._rpm_to_build_cache = {}
''' a docstring '''
jskladanAuthorUnsubmitted
Not Done

Ooint taken, although I do not really see the need to document its contents as this is to be used only in the one piece of code, and the variable is marked as "private". But I guess there will be no harm in it.

37​
38​ def rpm_to_build(self, rpm, prefetch=True):
kparalUnsubmitted
Not Done

I understand the reasoning behind prefetch, but I'm a bit concerned whether it is helpful enough. RPM searches in Koji take a long time. If there are several dozens of builds to run through depcheck, it will take a really long time, no matter buildinfo caching.

I think this should rather take a list of rpm filenames as an input, and then make all the queries at once using Koji's multiCall. I imagine this will be at least 10x faster for larger sets of RPMs. You can have a look at upgradepath and see how multicall is used.

I don't say it has to be implemented right now, but I think it would be highly preferable. If you decide to stay with current implementation because of time constraints, please at least file a ticket?

jskladanAuthorUnsubmitted
Not Done

I can file the ticket, although from what I tried, this helps quite a lot.

39​ '''Get koji build object for the rpm.
40​
41​ :param str rpm: filename as either `/my/path/nvr.a.rpm` or `nvr.a.rpm`
kparalUnsubmitted
Not Done

Use need to use double backticks, if you're trying to highlight the text as code/value.

jskladanAuthorUnsubmitted
Not Done

thx

42​ :param bool prefetch: if set to True, get list of all rpms for the build
43​ and store it for future use - this will speed up subsequent queries
44​ for rpms belonging to already queried build.
45​ :rtype: bunch
kparalUnsubmitted
Not Done

Return type is documented, but return value is not. Let's say something like:

:return: Koji buildinfo dictionary (as returned e.g. from :meth:`koji.getBuild`)
kparalUnsubmitted
Not Done
:rtype: :class:`bunch.Bunch`
46​ :raise TaskotronRemoteError: if rpm or build is not found in koji
kparalUnsubmitted
Not Done

I was a bit confused by the phrase "or build", because I don't specify "build" anywhere. Let's use "or its build", "or its related build" or something along the lines.

jskladanAuthorUnsubmitted
Not Done

Thx, I'll be your SPHINX padawan soon enough :)

47​ '''
48​ rpm = os.path.split(rpm)[-1]
kparalUnsubmitted
Not Done

Something like # strip path would help a bit for those of us who don't know the whole Python library by heart ;)

jskladanAuthorUnsubmitted
Not Done

*shrugs* OK, I can comment on that line, although # grab filename from path says more about the intent, I guess

49​
50​ if rpm in self._rpm_to_build_cache:
51​ return self._rpm_to_build_cache[rpm]
52​
53​ rpminfo = self.session.getRPM(rpm)
54​ if rpminfo is None:
55​ raise exc.TaskotronRemoteError('RPM not found')
56​
57​ buildinfo = self.session.getBuild(rpminfo['build_id'])
58​ if buildinfo is None:
59​ raise exc.TaskotronRemoteError('Build %r not found' % rpminfo['build_id'])
60​
61​ if prefetch:
62​ rpms = self.session.listRPMs(rpminfo['build_id'])
63​ for r in rpms:
64​ filename = "%s.%s.rpm" % (r['nvr'], r['arch'])
65​ self._rpm_to_build_cache[filename] = buildinfo
66​ else:
67​ self._rpm_to_build_cache[rpm] = buildinfo
68​
69​ return buildinfo
36 70​
37 71​
38 def nvr_to_urls(self, nvr, arches=None, debuginfo=False, src=True): 72​ def nvr_to_urls(self, nvr, arches=None, debuginfo=False, src=True):
39 '''Get list of URLs for RPMs corresponding to a build. 73​ '''Get list of URLs for RPMs corresponding to a build.
40 74​
41 :param str nvr: build NVR 75​ :param str nvr: build NVR
42 :param arches: restrict the arches of builds to provide URLs for. Note: 76​ :param arches: restrict the arches of builds to provide URLs for. Note:
43 If basearch ``i386`` is in the list, ``i686`` arch is 77​ If basearch ``i386`` is in the list, ``i686`` arch is
▲ Show 20 Lines • Show All 129 Lines • Show Last 20 Lines
View Options

testing/test_koji_utils.py

Show All 14 Lines
15class TestKojiClient(): 15​class TestKojiClient():
16 def setup_method(self, method): 16​ def setup_method(self, method):
17 self.ref_nvr = 'foo-1.2-3.fc99' 17​ self.ref_nvr = 'foo-1.2-3.fc99'
18 self.ref_arch = 'noarch' 18​ self.ref_arch = 'noarch'
19 self.ref_name = 'foo' 19​ self.ref_name = 'foo'
20 self.ref_version = '1.2' 20​ self.ref_version = '1.2'
21 self.ref_release = '3.fc99' 21​ self.ref_release = '3.fc99'
22 self.ref_buildid = 123456 22​ self.ref_buildid = 123456
23​ self.ref_filename = "%s.%s.rpm" % (self.ref_nvr, self.ref_arch)
23 24​
24 self.ref_build = {'package_name': self.ref_name, 25​ self.ref_build = {'package_name': self.ref_name,
25 'version': self.ref_version, 26​ 'version': self.ref_version,
26 'release': self.ref_release, 27​ 'release': self.ref_release,
27 'id': self.ref_buildid} 28​ 'id': self.ref_buildid}
28 29​
29 self.ref_rpms = [{'name': self.ref_name, 'version': self.ref_version, 30​ self.ref_rpms = [{'name': self.ref_name, 'version': self.ref_version,
30 'release': self.ref_release, 'nvr': self.ref_nvr, 31​ 'release': self.ref_release, 'nvr': self.ref_nvr,
31 'arch': self.ref_arch}, 32​ 'arch': self.ref_arch, 'build_id': self.ref_buildid,},
32 {'name': self.ref_name, 'version': self.ref_version, 33​ {'name': self.ref_name, 'version': self.ref_version,
33 'release': self.ref_release, 'nvr': self.ref_nvr, 34​ 'release': self.ref_release, 'nvr': self.ref_nvr,
34 'arch': 'src'}] 35​ 'arch': 'src', 'build_id': self.ref_buildid, }]
36​
37​ def test_rpm_to_build(self):
38​ stub_koji = Dingus(getBuild__returns = self.ref_build,
39​ listRPMs__returns = self.ref_rpms,
40​ getRPM__returns = self.ref_rpms[0])
41​
42​ test_koji = koji_utils.KojiClient(stub_koji)
43​ outcome = test_koji.rpm_to_build(self.ref_filename)
44​
45​ assert outcome is self.ref_build
46​ assert len(stub_koji.calls) == 3
47​
48​ # test prefetch
49​ outcome = test_koji.rpm_to_build(self.ref_filename)
50​ assert len(stub_koji.calls) == 3
51​
52​ def test_rpm_to_build_exceptions(self):
53​ stub_koji = Dingus(getRPM__returns = None)
54​ test_koji = koji_utils.KojiClient(stub_koji)
55​
56​ with pytest.raises(exc.TaskotronRemoteError):
57​ test_koji.rpm_to_build(self.ref_filename)
58​
59​
60​ stub_koji = Dingus(getBuild__returns = None,
61​ listRPMs__returns = self.ref_rpms,
62​ getRPM__returns = self.ref_rpms[0])
63​
64​ test_koji = koji_utils.KojiClient(stub_koji)
65​ with pytest.raises(exc.TaskotronRemoteError):
66​ test_koji.rpm_to_build(self.ref_filename)
67​
35 68​
36 def test_get_noarch_rpmurls_from_nvr(self): 69​ def test_get_noarch_rpmurls_from_nvr(self):
37 stub_koji = Dingus(getBuild__returns = self.ref_build, 70​ stub_koji = Dingus(getBuild__returns = self.ref_build,
38 listRPMs__returns = self.ref_rpms) 71​ listRPMs__returns = self.ref_rpms)
39 72​
40 test_koji = koji_utils.KojiClient(stub_koji) 73​ test_koji = koji_utils.KojiClient(stub_koji)
41 74​
42 test_urls = test_koji.nvr_to_urls(self.ref_nvr) 75​ test_urls = test_koji.nvr_to_urls(self.ref_nvr)
▲ Show 20 Lines • Show All 61 Lines • Show Last 20 Lines