@lucarval @mprahl @yashn PTAL

results_until or results_before?

I knew you wouldn't like the name :)
eheh both are fine to me.

Hmm, is "until" the right word here? Seems that resultsdb API is using "since" too. :)

aaand that's why I chose "since" :)

Fields results_since and waivers_since should be merged into single field, something like decision_before_datetime which will tell GW to ignore both results and waivers older than that date/time.

This shouldn't say list type for the jsonparam.

Fields results_since and waivers_since should be merged into single field, something like decision_before_datetime which will tell GW to ignore both results and waivers older than that date/time.

I think you're right. Because we're using the submitted time of the waiver and the result combining the two might give us a more accurate representation of the state of things.

Yeah I thought about it... I can make the change. Also now we are ignoring only the specific result id or specific waiver id, so we are already not taking into account one of the two... I guess is a good enhancement.

since doesn't sound right to me. It would imply that Greenwave will only consider results from this point and newer. until or before sound better.

I had the same thought as @lholecek. Is there a use-case for having them separate? I don't think it's that important, but it would abstract complexity from the user.

Is the format of waivers_since validated at this point?

If I read the PR correctly, it seems you are no longer caching results, so perhaps it makes sense to remove this test.

This is repeated several times. Could you write a small function that provides this instead?

@gnaponie I left a few comments but it looks great. Nice job!

This is not correct. According to docs, you need to use datetime pair (range), something like '2010-01-01T00:00:00,{}'.format(self.results_since).

You can use both (datetime pair with the comma, or just one). That's what in the doc.

"Date (or datetime) in ISO8601 format. To specify range, separate start and end date(time) by comma."

Yes, but using one value only sets the date/time range start, so it'll only show the results older then the single specified value (hence the "since" name).

mmm I think here anyhow it is always passed a range: 1900-01-01T00:00:00.000000,2019-03-29T10:12:44.171649
for example

It might be not working if you just directly call the API. I'll write a test to be sure this works.

I've addressed some comments. I'll finish on Monday.

I've addressed all the comments and fixed the conflicts.
Can someone of you do another quick review? @mprahl @lucarval @lholecek Thanks.

Fixed the conflicts...

LGTM!

It seems a bit odd to specify the type in the JSON parameter name since before describes the parameter.

Also, you're not really getting the decision before this time, you're getting the decision at this specific time. @lucarval and I talked, and the parameter name of when would make sense. decision doesn't need to be in the name since the endpoint is called decision.

If you take the suggestion from the comment above and rename the parameter to when, then you would move this code to where Greenwave requests the old decision, and the API endpoint would use the date as-is.

You're missing a .format call on the string

@gnaponie I'll finish reviewing this tomorrow. Feel free to adjust the current comments.

This is still not correct. The parameter value in this case needs to be a pair, otherwise WaiverDB considers only items since the given date/time.

Same with the parameter for ResultsDB. Can you write functional tests for this?

Is this correct? I thought you were ignoring results after this date, not before.

What do you think about adding a timestamp in the decision updates messages sent on the message bus? That way, if there are multiple factors causing a decision change at the same time, an application that is stateful can ignore a decision update that was sent out of order due to race conditions.

Maybe I'm overthinking this, and we can implement this later if there's a need.

What is the benefit of update over just setting the value in data like the following?

data['decision_before_datetime'] = result['submit_time']

Same comment as above

Okay, I'm done reviewing. Please let me know when you've addressed the comments.

So just "when" is it fine for you? Let's decide so it would be the final decision.
@lholecek is it fine for you too?

Yes, "when" is easier to understand - you're basically asking Greenwave: "What was the decision at this point in time?"

What the function test should check in your opinion? There's already a test for consuming a waiver. What's not covering that?

Oh, why was the cache removed? I think this would cause some performance issues.

@lholecek because all that code of retrieving each result one by one is not necessary anymore.
We could cache just the call that retrieve latest, but it is pretty fast as query.

@lholecek because all that code of retrieving each result one by one is not necessary anymore.
We could cache just the call that retrieve latest, but it is pretty fast as query.

OK, great. Would be good to mention in commit message that only the latest resultsdb endpoint is used and caching is no longer required.

You can also remove all the cache stuff from greenwave/consumers/resultsdb.py.

The test could be: create a waiver for a failed result and ask for decision before the waiver was created (i.e. something like when=waiver.datetime-1) - the summary should be "1 failed test".

It looks to me that the test would fail.

This test seems to me it's already present. It's called "test_ignore_waiver".

I've rebased and I think I've addressed all comments.
@mprahl @lucarval another last review?

before that time => from that point in time

, None is not necessary

date results => date, results

Optional: It seems a but odd to use update when just setting one key.

It could just be:

params['since'] = f'1900-01-01T00:00:00.000000,{self.when}'

Should this be configurable? This seems very specific to Fedora's implementation.

Do WaiverDB and ResultsDB both support microseconds in their filtering?

This should likely have a docstring.

Even if there, but the results don't have such fields, the result of the query won't change. I don't think it's worth adding a configuration parameter. We have other things in Greenwave "specific" for just Fedora, but it is fine as long as they don't influence other instances.

It seems like they do.
ResultsDB compares the input intervals with "submit_time" in the database that is equal to datetime.datetime.utcnow() at the moment of the creation.

WaiverDB uses format: "%Y-%m-%dT%H:%M:%S.%f"

Comments addressed.

@mprahl @lucarval PTAL

Would it be OK to remove ignore_result/ignore_waiver parameter support completely? It was undocumented before so nobody should be using it (except greenwave internally).

Would it be OK to remove ignore_result/ignore_waiver parameter support completely? It was undocumented before so nobody should be using it (except greenwave internally).

I've discussed about it with @lucarval . And yes, none uses it (that we know about), but it would still require to make a not-backward-compatible release and it would be nice to release a new version of the API in that case.
Other changes like that are planned, so we might just want to change all of them in a future release eventually.