@adamwill could you also please review it?

@everyone: please, don't merge this PR until this one https://pagure.io/taskotron/resultsdb/pull-request/129 doesn't get released.

Discussing with Luiz:
- It would be nice to have another test in which the policy defines scenario X, but there's only result for scenario Y
- "But AFAIK scenario is defined only for composes." --> if we group by scenario unconditionally, we should expect the same result set. But this doesn't happen. ResultsDB's related change is probably not handling NULL values as it should. We might need a LEFT JOIN there.

I really dislike the hardcoding too, to be honest, but I don't have enough context on what exactly is going on here and how it works to suggest a detailed alternative. @gnaponie 's suggestion that this is a bug in resultsdb seems plausible to me and if that's the case I'd prefer we get that fixed and don't workaround it by hardcoding here... @jskladan ?

@adamwill thank you for your feedback, that's what I wanted your opinion about.
The bug is not in resultsdb's code, but in my resultsdb's PR :D
It might be just ok to do a LEFT JOIN instead of a normal JOIN... I'll look into it tomorrow.

AFAIK, the change in resultsdb is not merged yet. Won't this change completely break getting latest results for composes?

This sounds a bit scary (I have to check the change in resultsdb). Will this work correctly for both Fedora and our internal composes?

@lholecek yeah, in fact we won't merge this one until the resultsdb's one is not released.
I wrote that in the first comment... but maybe I should put a "WIP" in the subject?

Anyhow, yeah, I should remove the "compose" thing improving the resultsdb part.

Don't forget to update the filter name to _distinct_on.

To be clear about something here, the 'scenario' concept is something that - AFAIK - only results from Fedora openQA include so far. I do not believe results for RHEL composes in any RH-internal resultsdb instance use it. I do not know exactly what results for compose testing in RH-internal resultsdb look like, but it would certainly be a good idea to find some and check this against it.

I did make an effort to set up some sort of framework for standardizing resultsdb results - it's called resultsdb_conventions - but it never really got any buy-in and I'm not aware of anything else along the same lines getting any buy-in, so practically speaking, you can never have any confidence that results for testing the same thing filed by different test systems will look anything like each other :( There is no assurance whatsoever that a result of testing a Fedora compose filed by openQA looks anything at all like a result of testing a RHEL compose filed by Jenkins or something, not in any way at all. Similarly, as we know, results for the same Koji package build filed by Taskotron and the CI pipeline do not look very much like each other at all.

Unfortunately, you have to figure out / decide which test system(s)'s results you care about and figure out how those results look and make sure whatever you're currently working on will work with that, which sucks and is what I wanted to avoid, but hey.

BTW, another correction I missed yesterday:

"But AFAIK scenario is defined only for composes." <-- this is not true. openQA also tests candidate updates, and includes scenarios in those results. The type for these results is "bodhi_update". Here is an example.

@adamwill So for bodhi_update items, Greenwave should only care about the latest result for each (testcase, scenario) tuple. Is that correct?

@adamwill You're right, we're suffering from a lack of consistency in how teams use ResultsDB. This is now becoming an issue because, in order to evaluate policy, Greenwave needs to be told about what results are relevant, and what aren't.

For this situation, rather than hard-coding the _distinct_on, we could make it configurable based on result type. The config would indicate that compose and bodhi_update items have scenario added to the DISTINCT ON clause. Other types may eventually use other fields, like arch or os. This should allow Greenwave to make the correct ResultsDB queries, and avoid hard-coding based on the peculiarities of specific testcases.

... @lucarval and I found a query that should work in resultsdb. I'll change the PR tomorrow and update you about it.

@mikeb

@adamwill So for bodhi_update items, Greenwave should only care about the latest result for each (testcase, scenario) tuple. Is that correct?

Yep, same as for openQA Fedora compose results.

@adamwill You're right, we're suffering from a lack of consistency in how teams use ResultsDB. This is now becoming an issue because, in order to evaluate policy, Greenwave needs to be told about what results are relevant, and what aren't.
For this situation, rather than hard-coding the _distinct_on, we could make it configurable based on result type. The config would indicate that compose and bodhi_update items have scenario added to the DISTINCT ON clause. Other types may eventually use other fields, like arch or os. This should allow Greenwave to make the correct ResultsDB queries, and avoid hard-coding based on the peculiarities of specific testcases.

This still only works if you can be sure results of the same 'type' will look the same, though, and there is no more guarantee of that than there is of anything else :/ (For bonus points, CI pipeline results do not have a type at all...)

@adamwill I think it's ok to say "if you want your results to apply to a Greenwave policy, it has to have certain fields". Maybe we just need to be more clear/strict around what acceptable test results look like? No one said we had to be able to handle every possible result format in Greenwave.

Well hey, I'm all in favour of someone getting into the business of defining some standards =) Ideally they'd look like the ones I invented, because that'd make my life easier...

It should be ok now.
@lholecek (and everyone else who is interested) could you review it?

@gnaponie Looks OK but I would wait for your patch to be merged to resultsdb. Do we also need to wait for new resultsdb release including the change?

Thanks for reviewing @lholecek.
I would wait resultsdb to be released. I wouldn't want to release this change if resultsdb change also is not released (mmm not a big issue I guess, just the test would probably fail).

LGTM! As mentioned earlier, let's hold off merging this until the corresponding ResultsDB change is done.

Make sure that the new resultsdb is deployed on prod - also internally - before this can be released to prod.

@lholecek we are working on it.