Did you check pylint? It gives me 2 errors (that don't seem to be related to your changes though... but just to be sure).

This line is too long.
E501 line too long (104 > 100 characters)

greenwave/policies.py|278 col 101| E501 line too long (103 > 100 characters)

greenwave/policies.py|374 col 101| E501 line too long (103 > 100 characters)

greenwave/policies.py|439 col 101| E501 line too long (103 > 100 characters)

greenwave/policies.py|478 col 101| E501 line too long (108 > 100 characters)

greenwave/policies.py|489 col 101| E501 line too long (101 > 100 characters)

Checked pylint and except those "line too long" messages didn't see anything else. I ignored those because I didn't think we use pylint. Let me fix it.

Is this method actually used somewhere?

We should keep pylint because it is necessary to the release. It is in the Jenkins job, if it doesn't work, the jenkins job fails and the release is blocked.
But we can discuss about the actual benefit of pylint and consider if we should remove it or keep it.

nvr?

invalid? It was there before, but let's change it..

I don't exactly understand where is the place that is getting the waivers only if actually needed.
Can you point me at it?

And can you provide a test for this case? For example: if I have only PASSED results, I don't need to ask for waivers. Additionally: I would maybe somewhere write (either in the doc, or either in the decision response) that waivers are not retrieved if it's not really needed (if the decision is already positive considering only the results).

nvr?

I just copied this from other test.

The retriever instances call _retrieve_data() to get the "data" field value from restulsdb/waiverdb requests. This is just overridden for tests.

I don't exactly understand where is the place that is getting the waivers only if actually needed.
Can you point me at it?
And can you provide a test for this case? For example: if I have only PASSED results, I don't need to ask for waivers. Additionally: I would maybe somewhere write (either in the doc, or either in the decision response) that waivers are not retrieved if it's not really needed (if the decision is already positive considering only the results).

Previously, all waivers for given product version and subjects were retrieved. Now, waivers are retrieved for specific test case only when the test fails or is missing.

Let me try to write a test and update docs.

We should keep pylint because it is necessary to the release. It is in the Jenkins job, if it doesn't work, the jenkins job fails and the release is blocked.
But we can discuss about the actual benefit of pylint and consider if we should remove it or keep it.

Let's keep pylint - it can find interesting errors (it's better in this than flake8).

nvr?

I just copied this from other test.

I wouldn't carry typos over.. I understand if you don't want to change the old test, it's not the scope of this PR, but let's not introduce the typo also in the new tests.

I got an idea how to improve this even further.

GW can make only single waiverdb request after it checks all the policies. Checking policies could give "result missing" or "results failed" answer - for those GW can create single query to waiverdb.

This will also decouple waiving results from policies - policies will only know how to retrieve results, waiving failed and missing results can be done separately.

nvr?
I just copied this from other test.

I wouldn't carry typos over.. I understand if you don't want to change the old test, it's not the scope of this PR, but let's not introduce the typo also in the new tests.

Is that typo? Doesn't E in NEVR mean "epoch"?

nvr?
I just copied this from other test.
I wouldn't carry typos over.. I understand if you don't want to change the old test, it's not the scope of this PR, but let's not introduce the typo also in the new tests.

Is that typo? Doesn't E in NEVR mean "epoch"?

I don't recall this type. If you are aware of it, please just ignore my comment.

I got an idea how to improve this even further.
GW can make only single waiverdb request after it checks all the policies. Checking policies could give "result missing" or "results failed" answer - for those GW can create single query to waiverdb.

please do. That was what I was hoping for in this change.

This will also decouple waiving results from policies - policies will only know how to retrieve results, waiving failed and missing results can be done separately.

@lholecek can you remove the conflicts here? I'll make another review then. Thank you.

We decided not to go with this approach, but I really appreciate this section since we still don't have a solution! Thank you for posting that.

mmm shouldn't that be done only if verbose == False? Otherwise (if verbose == True) you've already added these. Right?

Why removing this? Isn't this here because of this?
https://docs.pagure.org/greenwave/package-specific-policies.html#tolerate-an-invalid-gating-yaml-file

OK, let me remove the changelog entry.

There is probably no need to document any of this - how waivers are retrieved is an implementation detail.

mmm shouldn't that be done only if verbose == False? Otherwise (if verbose == True) you've already added these. Right?

Hmm, possibly. Let me try to omit this when verbose is set.

I only moved the code for waiving out of the policies submodule. Waiving invalid gating.yaml still works.

OK, let me remove the changelog entry.
There is probably no need to document any of this - how waivers are retrieved is an implementation detail.

I actually liked that! But you are right, this change maybe doesn't apply so much. We don't need to put in the changelog.

I only moved the code for waiving out of the policies submodule. Waiving invalid gating.yaml still works.

Sounds good!

In case we have verbose == False and all results satisfied, is this going to make a request?
If not: I'm confused about where this gets checked.
If yes: shouldn't we check if it is actually needed?

Can you provide some description to "_is_waived" and "_maybe_waive"? The names are a bit strange to me.

What do you mean by "unsatisfied waived"?

In case we have verbose == False and all results satisfied, is this going to make a request?
If not: I'm confused about where this gets checked.

It just returns empty list, see WaiversRetriever._retrieve_all().

What do you mean by "unsatisfied waived"?

Ah, I meant:

Returns answers with unsatisfied answers waived (RuleNotSatisfied.to_waived()) if there is matching waiver.

Fixing.

Can you provide some description to "_is_waived" and "_maybe_waive"? The names are a bit strange to me.

Done.

Last request, I promise :) Can you add one or two tests (maybe functional test)? One should check that no call to waiverdb is made when all results are satisfied and the other one should check that one call is made to waiverdb when one result is either missing or failed. You can use maybe https://docs.python.org/3/library/unittest.mock.html#unittest.mock.Mock.assert_called_once

I've added the tests.

:thumbsup: Nice!

@gnaponie Can you review? I've resolved some merge conflicts.

@lholecek looks good! Let's merge it \o/

Commit c5dcb5e3 fixes this pull-request