Add downloading latest stable build to koji directive
ClosedPublic

Authored by mkrizek on Jul 7 2015, 8:46 AM.

Diff Detail

Repository
rLTRN libtaskotron
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
mkrizek retitled this revision from to Add downloading latest stable build to koji directive.Jul 7 2015, 8:46 AM
mkrizek updated this object.
mkrizek edited the test plan for this revision. (Show Details)
mkrizek added reviewers: tflink, kparal, jskladan.
tflink added a comment.Jul 8 2015, 4:38 AM

Minor nit - this diff was not done against one of the canonical upstream branches (it was against one of the commits in D383). it applied fine but there was a warning message.

My other concern is about how this would be used. The code itself works fine but I'm wondering if we want to make the downloaded n(e)vr available. We do have the list of downloaded rpms but I'm not seeing a way to know what the "latest stable" build actually is.

I'm not sure if the abidiff folks actually need this or not, though. I can't think of other places it might be needed.

Other than that question, the code looks good to me.

kparal requested changes to this revision.Jul 8 2015, 11:12 AM

As usual, a few documentation nitpicks :) There are two more important issues, I think, and that is one unchecked IndexError and a simple optimization improvement for Koji query speed.

In D417#7728, @tflink wrote:

We do have the list of downloaded rpms but I'm not seeing a way to know what the "latest stable" build actually is.

Two options come to my mind:

  1. download to a specific directory. You'll run the koji directive twice, one with download action and download to e.g. latest/ directory, and once with download_latest_stable action and download to e.g. stable/ directory. Then you can pass these two dirs into the check as arguments.
  2. save the list of RPMs into a different variable for each of the koji directive invocation (let's say ${latest_rpms} and ${stable_rpms}) and then pass the check (or probably rather its python wrapper) these two lists. It will then do some crunching magic and convert it to something usable by the check itself.

I'm not sure if the abidiff folks actually need this or not, though. I can't think of other places it might be needed.

Yes, we can adjust this once we learn their real use case. But the current implementation seems generic enough to me to allow some basic implementation.

libtaskotron/directives/koji_directive.py
34–37
N(E)VR of a Koji build to download (for ``action="download"``) or to search 
the latest stable build for (for ``action="download_latest_stable"``). Not 
required for ``action="download_tag"``. Example: ``xchat-2.8.8-21.fc20``
100
target_dir: ${workdir}/stable/
159–164

Just in case you wanted to format this without the long tiny column on the right and didn't know how, this is also PEP8-compliant:

output_data['downloaded_rpms'] = self.koji.get_nvr_rpms(
    nvr, self.target_dir, arches=self.arches, debuginfo=self.debuginfo, src=self.src)
182

'download_latest_stable'

190

typo: latest

libtaskotron/koji_utils.py
47–50

I'd like to change this method to latest_by_tag(self, pkgname, tags), and tags would be a non-empty list of searched tags. We would then call it like:

latest_by_tag('pidgin', ['f22', 'f22-updates'])

The reason to do this (instead of calling the method twice) is that we can now use multicall inside that function and make several calls at once, speeding up the query (and decreasing network error possibility).

52

Let's make NVR uppercase, thanks :)

55

Won't this crash if there's no such package?

This revision now requires changes to proceed.Jul 8 2015, 11:12 AM
mkrizek updated this revision to Diff 1120.Jul 9 2015, 11:34 AM
  • Optimize latest_by_tag by doing multiCall
mkrizek updated this revision to Diff 1121.Jul 9 2015, 11:38 AM
  • Optimize latest_by_tag by doing multiCall
mkrizek updated this object.Jul 9 2015, 11:39 AM
mkrizek removed a dependency: D383: Add distgit directive.
kparal requested changes to this revision.Jul 10 2015, 8:36 AM
kparal added inline comments.
libtaskotron/koji_utils.py
47

Does it make sense to raise an exception if the package is not tagged in the list of searched koji tags? Why not return simply None?

48–50

Unfortunately, this won't work. If you also use updates-testing tag, it can contain newer or *older* version of package that is now in updates tag. Don't ask me why, I just know it happens quite frequently :) I guess it happens if Bodhi fails to untag something from time to time, of if a packaged is pushed stable before even reaching testing. So, in the end, you have no way of knowing what order the tags should have.

You have to compare builds from all the tags and pick the latest version. We have some methods around for that.

67–68

I don't like this. There's no explanation when this happens and why we silently ignore it. There is a documentation in rpms_to_build(), so I understand what's going on with the IndexError, but still if any error happens in the koji call, it is silently ignored. We can easily return wrong results if just one call fails and one passes.

Please do a similar approach as in rpms_to_build() - if there is an error (dict returned), raise it. It should be no different from a network error, this is just a server error.

Why do you ignore KeyError? Should we expect that a build is missing nvr key? When?

This revision now requires changes to proceed.Jul 10 2015, 8:36 AM
mkrizek updated this revision to Diff 1156.Jul 14 2015, 10:59 AM
  • Fix multiCall processing
kparal added inline comments.Jul 14 2015, 11:43 AM
libtaskotron/koji_utils.py
67

I wonder if we can receive more builds if latest=True? I don't see it exactly documented in the API docs. And it always returns a single build in my testing. But an assert here could potentially help us discover some issues in the long run.

assert len(build[0]) <= 1, 'More than one build returned with latest=True'

It should only crash development environment this way (python -O flag removes assertions). Some guidelines here: http://stackoverflow.com/a/945135

70–71

Please document in the docstring that we return None if no builds are found.

libtaskotron/rpm_utils.py
104–110

Wouldn't it be easier to just call this code where appropriate instead of defining a new function for it? I.e. in koji_utils instead of

sorted_nvrs = rpm_utils.sort_by_nvr(nvrs, ascending=False)

you would do

sorted_nvrs = sorted(nvrs, cmp=rpm_utils.cmpNEVR, reverse=True)

What's the point of having a dedicated function?

mkrizek updated this revision to Diff 1164.Jul 14 2015, 12:29 PM
  • Assert
kparal accepted this revision.Jul 15 2015, 10:40 AM

Looks good.

This revision is now accepted and ready to land.Jul 15 2015, 10:40 AM
This revision was automatically updated to reflect the committed changes.