greenwave

#398 Omit comparing result_id values for decision change
Merged by lholecek. Opened by lholecek.
lholecek/greenwave fix-decision-change-check  into  master

Download 398.patch

When Greenwave receives new result message from ResultsDB it tries to
compare old decision (ignoring the new result) with new one (for all its
policies) so it can publish decision update message only when the
decision changed.

The new decision was seen as "changed" when any of its data differ from
the old decision. The problem is that decision data include result IDs
so it's always seen as "changed" if the new result is part of the new
decision.

Example:

# Decision before new result is available:
{
    "applicable_policies": [
        "taskotron_release_critical_tasks_for_stable"
    ],
    "policies_satisfied": true,
    "satisfied_requirements": [
        {
            "result_id": 27968725,
            "testcase": "dist.abicheck",
            "type": "test-result-passed"
        },
        {
            "result_id": 28015726,
            "testcase": "dist.rpmdeplint",
            "type": "test-result-passed"
        }
    ],
    "summary": "All required tests passed",
    "unsatisfied_requirements": []
}
# Decision after new result with ID=28015899 is available:
{
    "applicable_policies": [
        "taskotron_release_critical_tasks_for_stable"
    ],
    "policies_satisfied": true,
    "satisfied_requirements": [
        {
            "result_id": 27968725,
            "testcase": "dist.abicheck",
            "type": "test-result-passed"
        },
        {
            "result_id": 28015899,
            "testcase": "dist.rpmdeplint",
            "type": "test-result-passed"
        }
    ],
    "summary": "All required tests passed",
    "unsatisfied_requirements": []
}

With this change, the new decision is seen as "changed" when any of its
data changes except result IDs.

Fixes #395

Signed-off-by: Lukas Holecek hluk@email.cz

I find all this block a bit confusing (maybe it's just the variables name...)
Can't you just loop over decision['satisfied_requirements'] and decision['unsatisfied_requirements'] and for each of these elements make del(item['result_id']).

It is similar to what you did, but I find it less messy.

I did this before, but it changes the decision data that goes into the published message later (therefore the decision.copy() above).

There is a definitely smarter way to do this but I failed to find something like deep comparison and all my other code looked worse.

Maybe doing deepcopy and then removing the result_id values, but that seems like overkill (the decision data can be relatively large).

Yeah deepcopy would work. What about something like that? (flake8 needs to be fixed):
https://paste.fedoraproject.org/paste/An9s75xBojrREXR86YjW1w

Is that still confusing? It seems ok to me.

rebased onto b20483c42a336a41f35f91f8d0232ea01bc47115

rebased onto 4e842f5590153e79666b59dbee6ba91662db5489

How about straightforward compare without any copy operation? (Updated.)

What's lhs and rhs supposed to stand for?

What's lhs and rhs supposed to stand for?

Left/right hand side! :raised_hands:

We really only care about comparing the policies_satisfied key, right? Can we be explicit about that instead of potentially comparing some junk?

Do we need to sort them first?

I cannot find the discussion but I the change in satisfied/unsatisfied requirements was also needed (I also mentioned it here).

I think it follows the order of rules defined in policies, so the order shouldn't change even after setting the ignore_result parameter.

:thumbsup: LGTM!

If it's not required for the input variables to be dictionaries, perhaps it makes sense for except_keys to default to None.

:thumbsup:

If it's not required for the input variables to be dictionaries, perhaps it makes sense for except_keys to default to None.

I'll remove this check and add following to the function below to make it more clear.

    if old_decision is None or decision is None:
        return old_decision == decision

rebased onto 883f1792ffdfbd1e0e086c0e8835f2e914cf2a30

Commit 54407b0e fixes this pull-request

Pull-Request has been merged by lholecek

Pull-Request has been merged by lholecek
Metadata
None
No Tags
Static archive. pagure.io was sunset in 2026; this is a read-only snapshot.
Red Hat Logo

Fedora is sponsored by Red Hat. Learn more about the relationship between Red Hat and Fedora »

© Red Hat, Inc. and others.