The test is provided by @lholecek (I've just changed it a bit, I hope you don't mind I've put it here).

I think you can just define Policy.get_rules() and override it in RemotePolicy.

        for policy in current_app.config['policies']:
            if policy.subject_type in subject_types:
                testcases = (
                    getattr(rule, 'test_case_name', None)
                    for rule in policy.get_rules()) # <- CHANGED HERE.
                if testcase in testcases:
                    applicable_policies.add(policy)

LGTM

Yeah we have RemotePolicy, but we never actually use it... in the conf we always use Policy, so if I put "get_rules" in the RemotePolicy it will never be called. Am I wrong?

Yeah we have RemotePolicy, but we never actually use it... in the conf we always use Policy, so if I put "get_rules" in the RemotePolicy it will never be called. Am I wrong?

@gnaponie Oh, yeah, that's true. My bad. You would also need to recurse to Rules.get_rules().

@lholecek so is it fine for you if we just leave it like that or you don't like it?

Apart from me not liking isinstance(), the code considers remote policies disregarding the product versions and context in parent policy. E.g.

# Defined in Greenwave configuration
--- !Policy
id: "test_policy"
decision_context: test_context
product_versions: [f28]
subject_type: koji_build
rules:
- !RemoteRule {}

# gating.yaml
--- !Policy
decision_context: another_test_context
product_versions: [epel7]
rules:
  - !PassingTestCaseRule {test_case_name: some.test.case.name}

@lholecek But at this point it's really just loading all the policies (remotes + in the server). And then is going to (code already there) check if they are applicable policies (in all the policies). So it should be right. Do you see an issue?

The purpose of the _publish_decision_changes() function is to publish decision change message whenever the result of a decision API call changes.

With the configuration above GW will give HTTP 404 ("Cannot find any applicable policies") for following request data.

{
  "product_version": "epel7",
  "decision_context": "another_test_context",
  "subject_type": "koji_build",
  "subject_identifier": "<nvr-with-dist-git-containing-the-gating.yaml>"
}

You can test it by changing one of the decision_context values in the test and check no messages are published (assert len(mock_fedmsg.mock_calls) == 0).

You are right, but we don't want to publish a message for the decision change if there are no applicable policies.
That is anyhow the same behavior as before... This PR just wants to fix the main issue (= not loading the remote rules). If we don't like the fact that the method is called "publish_decision_changes" and then returns an error if there no applicable policies (I'm not sure this is the wrong behaviour - it's reasonable for me that we return an error) I believe we can change it another time (with another PR), when there's less priority to fix that...

Ok Lukas and I had a meeting about it, I got his point. I'm fixing it.

Rebased. @lholecek can you have a look?

Drop the unneeded product_versions from the remote policy:

remote_policy.product_versions = set(remotepolicy.product_versions).intersection(set(policy.product_versions))
if remote_policy.product_versions and
...

Looks like indentation is wrong. Is flake8 complaining?

It is not. I'll try to run the JJ (I don't see the summary here...)

+1

Commit a1697dc7 fixes this pull-request