This feels awkward. It would be preferable to construct the PassingTestCaseRule object as a local variable first, and then append it to processed_rules once it's fully created.

Consider using isinstance(), to handle potential subclasses of OnDemandPolicy in the future.

This feels like it should be a @classmethod that returns a new OnDemandPolicy instance.

See other comment, it feels like _create_on_demand_policy() should be a @classmethod that returns the fully populated OnDemandPolicy instance.

Assuming that the request comes directly from the user input. If i would but True as the 'decision_context' value would not it brake the intended logic?

Same here as above for the 'rules'.

So i was looking how this method is used, and it is always used on the same place when the actual object is created. Can we use the contstructor instead? Or is there some other reason that i am not seeing?

Wouldn't it be better to have different types of tests (success, failure etc.) in different tests?

So i was looking how this method is used, and it is always used on the same place when the actual object is created. Can we use the contstructor instead? Or is there some other reason that i am not seeing?

As we discussed on the call, I tried it using in the constructor but the __new__ method has been overridden for parent classes and it breaks the current workflow if we pass a param in the constructor (as we would have to do in order to put the logic in the constructor).

Assuming that the request comes directly from the user input. If i would but True as the 'decision_context' value would not it brake the intended logic?

Do you mean when decision_context is also specified along with rules? That's being handled later in the method on line 36 in api_v1.py

This feels awkward. It would be preferable to construct the PassingTestCaseRule object as a local variable first, and then append it to processed_rules once it's fully created.

Agreed, but if there are multiple rules of the same object, assigning it to a local variable won't work as it would end up altering the same object in every iteration.

Could you simplify this if statement to:

req_json = requests.get_json()
if not req_json.get('decision_context') and not req_json.get('rules'):

How about?

log.error('Either decision_context or rules is required.')

Optional: I'd remove the 'Invalid request. part of the error message. The status code tells the user it's an invalid request.

@mprahl made the changes. :)

If you made this elif rule['type'] == 'PassingTestCaseRule':, then you could bring your raise BadRequest... line to an else. That might be a little cleaner.

Why are you filtering on product_versions instead of decision_context like it is below?

Maybe you should name it create_from_json or something similar?

So falsy values are allowed?

This feels awkward. I would assign PassingTestCaseRule() to a separate variable, set the properties, and then append it.

Why is this part of the response for requests with on-demand policies, but it doesn't seem to be for normal requests?

Why are you filtering on product_versions instead of decision_context like it is below?

Because the decision_context parameter will not be specified in an on-demand policy request. So , we would not be able to filter by that.

Why is this part of the response for requests with on-demand policies, but it doesn't seem to be for normal requests?

This part was mentioned in the design doc example. I don't know the reason why it is not present for normal requests already.

So falsy values are allowed?

Yes, for optional params like blacklist, excluded_packages, packages and relevance_key.

Probably OK for now, but it won't scale well. Something involving Rule.__subclasses__ could work well but it will probably need some code changes to initialize and validate arguments for the rule.

Why is this part of the response for requests with on-demand policies, but it doesn't seem to be for normal requests?

This part was mentioned in the design doc example. I don't know the reason why it is not present for normal requests already.

@gnaponie or @lholecek, should this be added to normal request responses as well for consistency?

@mprahl @yashn I think is just an error in the document. Sorry about that. I don't think is needed. Can we remove it? I'll update the doc too.

Those information are usually in the message, but they are not needed in the API response. We can remove them.

this gives me flake8 error: functional-tests/test_api_v1.py|1482 col 54| W292 no newline at end of file

This gives me flake8 error: greenwave/api_v1.py|8 col 1| F401 'greenwave.policies.Rule' imported but unused

I get several pylint errors. Could you check them?

(env_resultsdb) ~/proj/greenwave (pr428)$ pylint --reports=n greenwave/
************* Module greenwave.policies
greenwave/policies.py:327:16: W0201: Attribute 'test_case_name' defined outside __init__ (attribute-defined-outside-init)
greenwave/policies.py:328:16: W0201: Attribute 'scenario' defined outside __init__ (attribute-defined-outside-init)
greenwave/policies.py:654:8: W0212: Access to a protected member __validate_attributes of a client class (protected-access)
************* Module greenwave.api_v1
greenwave/api_v1.py:327:31: W0212: Access to a protected member _create_on_demand_policy of a client class (protected-access)
greenwave/api_v1.py:8:0: W0611: Unused Rule imported from greenwave.policies (unused-import)
************* Module greenwave.tests.test_policies
greenwave/tests/test_policies.py:929:25: W0212: Access to a protected member _create_on_demand_policy of a client class (protected-access)
greenwave/tests/test_policies.py:895:45: W0613: Unused argument 'tmpdir' (unused-argument)
-------------------------------------------------------------------
Your code has been rated at 9.96/10 (previous run: 10.00/10, -0.04)

We could just remove in the response the "applicable_policies", since it doesn't make more sense to include them in this case.

If I make a request with parameter rules:
[{'typo': 'PassingTestCaseRule', 'test_case_name': 'something'}]
With 'typo' instead of 'type' I get a traceback. We should return some error like "type" key is required.

Same thing happens when I put "type": "PassingTestCaseRule" correctly, but then I put a typo in "test_case_name" key.

@yashn Great job!! This new feature is so cool I think none will ever use the old way anymore :D

We might want to make the decision_context in the gating.yaml file optional at this point... Otherwise it might be confusing for the user.. what should they put there? But... this would need some changes in the RemoteRule part... Other opinions? @yashn @mprahl @lucarval

Could you add a test to check if the user is putting the right keys (type and test_case_name) in the rules? Once you handle possible typos or absence of the keys.

We might want to make the decision_context in the gating.yaml file optional at this point... Otherwise it might be confusing for the user.. what should they put there? But... this would need some changes in the RemoteRule part... Other opinions? @yashn @mprahl @lucarval

I agree. We can do that. @gnaponie My only question is whether we can rely on product_version here instead of the decision_context for RemotePolicy as well? I am unsure if any complications can arise if we use product_version here.

@yashn Great job!! This new feature is so cool I think none will ever use the old way anymore :D

ahahaha, thanks for the review @gnaponie!

I have made the changes you asked. Could you please take a look again?

I agree. We can do that. @gnaponie My only question is whether we can rely on product_version here instead of the decision_context for RemotePolicy as well? I am unsure if any complications can arise if we use product_version here.

@yashn can you make an example of complications about the product_version? Nothing comes to my mind...

That's not true for "RemoteRule".
I should be able to make a request with rules like these:
{'type': 'PassingTestCaseRule', 'test_case_name': 'baseos-ci.redhat-module.tier0.functional'}, {'type': "RemoteRule"}

So we need the check on "type" always. And the check on "test_case_name" only for "PassingTestCaseRule".
Sorry, I should have been clearer in the beginning.

You might change it, but it gives me flake8 error:
functional-tests/test_api_v1.py|1401 col 101| E501 line too long (102 > 100 characters)

Beside those small comments it looks fine!
Then I think we can merge it and worry about the "decision_context" optional in the gating.yaml as future improvement. I don't think people will immediately start using the RemoteRule in this feature, and even if, it's not a bug, just a clearer thing.

Optional: Assigning this message to a variable would be nice so the string isn't duplicated

So we need the check on "type" always. And the check on "test_case_name" only for "PassingTestCaseRule".
Sorry, I should have been clearer in the beginning.

Ah, Sorry about that. I should have been more careful. :(

For compatibility, does it make sense to have applicable_policies default to an empty list? I'm not quite sure if that's an improvement over the implementation in this PR. Thoughts?

getattr(self, attribute) should be getattr(self, attribute, None). If you don't add the third argument, then an AttributeError exception is raised.

The rules parameter should be documented in the docstring, so that the API docs that get autogenerated include it.

@yashn after the comments are addressed, this is good to merge in my opinion. Nice job!

For compatibility, does it make sense to have applicable_policies default to an empty list? I'm not quite sure if that's an improvement over the implementation in this PR. Thoughts?

It already was defaulting to an empty list before. @gnaponie suggested we should remove it in this case. Hence, we only add the applicable_policies key if rules are not specified.

The rules parameter should be documented in the docstring, so that the API docs that get autogenerated include it.

I am going to put that in a separate commit coming soon, after these changes are approved :- )

Did you consider creating new endpoint? This could lead to less error prone and simpler API. Now there is more parameters which are optional or required depending on presence of other parameters.

@lholecek it sounds a valid alternative, but we discussed about it and we preferred just to keep the same endpoint.

Can we add in the documentation that if you use this feature Greenwave won't publish any message about it, but it's only a "on-demand" thing? (maybe a bit better rephrased).

Is the response in the example (in the doc) the result of a request with "verbose" == False? (Because there's no results or waivers...). If yes, I would also put verbose=false in the example request parameter. So that if someone just try to copy-paste the example doesn't get something different.

Beside those 2 minor comments on the doc, IMHO this PR is good to be merged.
Good job, Yash!

Is the response in the example (in the doc) the result of a request with "verbose" == False? (Because there's no results or waivers...). If yes, I would also put verbose=false in the example request parameter. So that if someone just try to copy-paste the example doesn't get something different.

Aha, Yes, you are absolutely right. Changed it.

Can we add in the documentation that if you use this feature Greenwave won't publish any
message about it, but it's only a "on-demand" thing? (maybe a bit better rephrased).

Yes, nice idea. Added a note.

Beside those 2 minor comments on the doc, IMHO this PR is good to be merged.
Good job, Yash!

Thanks Giulia! I have made those changes you recommended above. :-)

UMB => message bus

UMB is Red Hat specific.

This indentation looks off.

Optional: I believe the term for a dictionary in JSON is an object, so it might be nice to use that terminology in the documentation.

containing => containing the

Optional: Also, I believe lists are called arrays in JSON.

A few minor documentation changes, but otherwise, +1. Nice job!

Optional: Also, I believe lists are called arrays in JSON.

Optional: I believe the term for a dictionary in JSON is an object, so it might be nice to use that terminology in the documentation.

Docs for the rest of params use list so I think it would be nice to be consistent. Also, an object seems a little vague to me, I thought being specific will avoid confusion. :-)

Made rest of the changes. Thanks Matt!

:thumbsup:

Commit 4f2e79d6 fixes this pull-request