This is currently an incomplete WIP. The .check() implementations are not done yet. Posting it for feedback on the overall approach.

More importantly, I think we need to figure out what exactly the structure of the 'unsatisfied_requirements' should be (and if it changes, whether that is an API break that deserves v2 or not). Basically #169.

Also missing some tests to cover the backwards compatibility on the requesting side ('subject' -> type+identifier) and messaging changes etc.

Amended commit 75e49e6 is closer to the mark now. This has all tests passing, except the messaging stuff which I still need to figure out.

But I think I have pieced together how it will need to work.

I'm not super happy about having the big if subject_type == 'bodhi_update': logic in various places but I guess we could refactor that further in future if needed.

Remaining TODO: fix messaging, then go back over all the issues related to #126 and explain how the changes in this PR will help to solve each issue.

Finally figured out what was going wrong with the message consumers. Turns out we have to be careful with the arguments to the cached functions, because they are keyed on the repr of the arguments and we are on Python 2, the repr of a str and its equivalent unicode are not the same.

I will add some assertions to help catch mistakes like this in future (or at least until we switch over to Python 3).

One another annoying hiccough... I will have to fix the problem of loading the app config inside the message handlers. We need to look up the BODHI_URL setting.

Okay! Series ending in commit 1ef66e8 has finally got everything working nicely, I think.

This includes PR#196, PR#199, and PR#200 so it will need rebasing after those are merged.

Definitely review this as individual commits to make it more readable.

I'm not super happy with the way the logic for the subject types (in particular, the special casing for subject_type == 'bodhi_update') is kind of splattered all over the entire code base. But I guess the important part here is the API and message format changes. The internal logic can always be refactored later.

I still need to go back over the related outstanding issues and explain how the changes here will make them better (particularly #123). That'll have to be tomorrow though.

The only other thing I am unsure about, is it weird that policies for koji_build are applied when you ask for a decision about bodhi_update? The end result is that your decision about bodhi_update can have unsatisfied requirements about koji_build. It makes sense, if you think about it... but is it confusing/surprising?

This looks conceptually sane to me. Here are some things that need adjustment:

• The backwards compat support in the HTTP API needs to support a brew-build type that should be treated just like koji_build.
• Same goes for the message publisher.
• The code in the message publisher that maps an incoming NVR to possibly bodhi updates needs to be configurable (turned off or on). When we deploy it in Fedora, it is fine as is, but when deployed elsewhere it will try to query Bodhi for every non-Fedora build it sees... and that's not what we want.

With those fixes, let's try to build a container on the side and put it in an environment (staging, internally?) where our OSCI friends can try to integrate against it with their results and bots.

FWIW, I agree that the bodhi_update stuff feels splattered. That can be abstracted into a more general framework later when we need to handle "advisories" ;)

As for the part about returning results about koji builds when greenwave is asked about a bodhi_update -- I think that generousness is actually expected of greenwave. Certainly, it would just make things easier on Bodhi.

Come to think of it, Greenwave should expand both to koji_build results and original_spec_nvr results for a bodhi_update to match with how things are done there today. OTOH, perhaps this is our chance to get them to change the way they're storing/keying results.

Right, it essentially does expand to both, with these patches. The behaviour will be:

• A new result appearing for koji_build or original_spec_nvr (or brew-build when I add it) will trigger decisions for subject type of koji_build. It will also trigger a separate decision for subject type bodhi_update if the build is in any update.

• When making a decision about a subject type of koji_build, we consider results for koji_build and original_spec_nvr and brew-build to be equivalent. That is, we look up all the possible combinations in Resultsdb and accept them in whichever form we find.

• Unsatisfied requirements will still have an item key, such as item': {'item': 'glibc-1.1-1.fc27', 'type': 'koji_build'} but this is deprecated. Callers should look at the subject_type and subject_identifier if the unsatisfied requirement is 'test_result_missing', or the result_id if it's 'test_result_failed'. And I will properly document this too (#169).

So we are encapsulating the knowledge about original_spec_nvr vs. koji_build in Greenwave, so that calling tools like Bodhi don't need to worry about it.

Posted a new series ending in commit c194cebd:

• rebased now that dependent PRs are merged
• added handling for 'brew-build'
• skip Bodhi calls if 'BODHI_URL' is unset
• added docs

So I think this is ready for another review.

I haven't tackled #169 although I will do that next since it will help to illuminate what is going on here. Will also look at deploying this PR in our internal stage environment for some extra testing.

One more rebase, primarily to pick up the fix from PR#201. New series ends in commit 74b66d9.

Go through all items at most once:

bodhi_update = next(
    (entry for entry in subject if entry.get('type') == 'bodhi_update' and 'item' in entry), None)
if bodhi_update is not None:
    return ('bodhi_update', bodhi_update['item'])

It would make sense now to make /decision endpoint allow HTTP GET requests, if it's not difficult.

Looks like now it'll be possible to ask for multiple results (by using comma separated list of NVRs). I can look into this later and finally fix #117.

Makes more sense to throw an exception here (handle it elsewhere as HTTP 400) since Bodhi updates cannot be handled.

I tried to run the functional_tests and I get this error:
RuntimeError: Gave up waiting for port 5677

That is the port for the subprocess created for bodhi.
Am I the only one with this error? Am I missing something?

I tried to run the functional_tests and I get this error:
RuntimeError: Gave up waiting for port 5677

Works for me. Could be a process that blocks the port (pkill gunicorn).

The functional tests need some better context management for managing the subprocesses.

Agreed. We can add that after this.

Oh yeah, you mean we can tweak retrieve_results() and retrieve_waivers() to take one subject type but multiple identifiers, right? And then each function can use the more efficient querying mechanisms in Resultsdb and Waiverdb to reduce the number of HTTP requests. Sounds like it will be a nice improvement.

Oh yeah, not a bad suggestion, although I don't think it matters much since there will be at most 50 or so entries. I find the logic slightly easier to comprehend in my version. So I would prefer to leave it as is if you don't object.

Well, I'm not too sure... I was coding defensively. The thing is, we can still give a sensible decision about a bodhi_update without actually calling to Bodhi. Or least a half-way sensible decision, it just depends what the policies are like.

I am on the fence about which way is better but right now, with these patches, I am trying to be super conservative about backwards compatibility (see for example the extra commit I added in Waiverdb to avoid giving back a 400, it's kind of similar to this situation).

+1 Can we merge finally? I assume all tests went well. :P

Yep! There are some comments on the internal JIRA issue with more details about what I tested.

The only thing I need to do now is make sure that Fedora stg is properly pinned so that it won't automatically pick up this change when we merge it -- we don't want to disrupt the Bodhi devs who are working on waiving UI patches.

The only thing I need to do now is make sure that Fedora stg is properly pinned so that it won't automatically pick up this change when we merge it -- we don't want to disrupt the Bodhi devs who are working on waiving UI patches.

@dcallagh, if it's only devs who would be disrupted (using stage for development?), I'm still OK with merging this.