Some docstrings would help here... I actually meant to write some before I posted this, but I forgot.

Yeah, I guess we can filter the results by using the testcases to make it a bit more efficient like I did before. params={'item': item, 'testcases': ','.join(policy.rules)}

This looks fantastic, :thumbsup:

Yeah possibly. The idea here is that this code (fetching results over the network) should be isolated from the details of the policy. It's the policy's job to pick out which of the results it cares about and decide based on those.

That separation will let us more easily write unit tests for the policies (without having mock network requests) but it has this disadvantage: we have to just fetch everything to feed it into the policies.

There is probably some middle ground we can strike.

The idea here is that this code (fetching results over the network) should be isolated from the details of the policy. It's the policy's job to pick out which of the results it cares about and decide based on those.

That doesn't really make much sense to me. If the policies know what they need, that should be used to construct the query for the outside service. It seems equivalent to querying a database for everything in a table and then sorting through the results in Python.

That separation will let us more easily write unit tests for the policies (without having mock network requests) but it has this disadvantage: we have to just fetch everything to feed it into the policies.

For the record, I think recording requests to outside services is perfectly fine for unit tests and we should do that. It's trivial to re-record tests when necessary (e.g. when an API changes). It's also good to have a set of integration tests that use real instances of the services.

This whole block can go away, right? It's just set to the same boolean value as is_satisfied.

Why exactly is this a class?

This seems like an inappropriate function name since it returns a Python dictionary, not JSON.

Without any documentation it's hard for me to say what this is trying to be, but as the code currently stands there's no reason for this to be a class.

Wouldn't it be better to simply implement this as an SQLAlchemy model since we know we want a database? You can use the model the exact same way you're using this object, but it's also what we really want in the end anyway.

I'd very much like to see documentation blocks on everything since I'm not 100% sure I understand what you're trying to do here.

Yeah, for the integration tests, we are thinking of using ResultsDB+WaiverDB+Greenwave in OpenShift.

Yeah, it seems we can replace policies_satisified with is_satisfied and get rid of this block.

No, policies_satisfied is outside the loop and is the overall answer (after checking all policies). The way it is structured there can be more than one applicable policy, even though there is only one hardcoded one now. It would probably benefit from more refactoring to make that clearer... I am also not keen on the idea of just returning a newline-separated list of summaries for each policy either. It should probably roll up the results of applying all policies into one summary. But I would rather tackle that in a separate patch. Like I said, this one is preserving the same output, just refactoring.

I was going for an Algebraic Data Types kind of thing... It might be clearer (and more Pythonic) when I add docstrings, and maybe with an abstract base class to show the relationship between all these possible result types.

Hmm yeah... In WaiverDB we use __json__ for this which is also weird (it's not a Python protocol method, in spite of it using double underscores, it's just a convention that came from old Beaker code using TurboJSON).

I think I got this name from Javascript's .toJson(). It doesn't mean "to a JSON encoded representation" but rather, "to a representation which can be encoded as JSON".

I agree it's a confusing name, suggestions welcome.

The idea is for policies to be built out of various different kinds of rules, and each rule knows how to enforce itself with its .check() method. For example this one accepts a waiver from anyone but in future we will want which only accepts waivers from certain LDAP groups. And then there will be a specialisation with only accepts a failure if it has a waiver from two specific LDAP groups (devel and qa or whatever it is). So I was imagining that these would be different rule types.

Yeah. It might make more sense to only set the boolean value when policies_satisfied is True, such as "if not is_satisfied and policies_satisfied". But I agree, this code needs refactoring to make it clearer.

Okay, here is v2 (ending in commit 62ef9aa7). Changes:

• added docstrings to describe all the new pieces in policies.py
• added base classes -- even though they don't do anything useful, they help to make the relationship between all the pieces clearer and gives a place to describe the methods
• tried to tweak the big loop in make_decision() to be clearer

There are further steps I would like to take, like taking out the policy id from the summaries and cutting them down to a single line, with better wording for the humans. That should let us clean up the loop in make_decision() a little further I think. But I would like to do that in a separate PR.

So my overall opinion on this design is that it's far more complicated than it needs to be. There's no reason to have a base Answer class. If answers are only ever boolean yes/no, we should just use the built-in boolean types rather than re-inventing them. If the answers aren't always going to be yes/no, (and frankly, even if they are) you could just as easily model this using exceptions.

Pagure makes it basically impossible to carry on a conversation inline so I'm just going to respond here.

I think we should not go this direction and instead model this the way we would with a relational database.

The idea is for policies to be built out of various different kinds of rules, and each rule knows how to enforce itself with its .check() method. For example this one accepts a waiver from anyone but in future we will want which only accepts waivers from certain LDAP groups. And then there will be a specialisation with only accepts a failure if it has a waiver from two specific LDAP groups (devel and qa or whatever it is). So I was imagining that these would be different rule types.

This sounds like a good place to start with this data modeling. We have a Policy and a Rule object, and a Policy object is composed of a list of Rule objects.

A Rule has a check API that returns True (or maybe None) if the Rule is satisfied. If a rule isn't satisfied, False is returned (or maybe an exception is raised). An instance of a Rule has:

• A test case name (required)
• A list of LDAP groups that can create waivers (optional)
• A foreign key relationship to Policy

A Policy has a check API that is a shorthand for calling check on all its rules. An instance of Policy has:

• A list of rules
• A product it applies to
• A human-readable name?

The check API accepts a "decision context" which is a set of test results + waivers. Alternately the check API could be responsible for retrieving that information.

How does that sound?

I thought it would be worth creating what I'm talking about since seeing is much better than hearing - A really rough implementation is in #12

Just to reply to your first comment... The "answers" are not just yes/no, they also have to be able to describe themselves as "unsatisfied requirements" in the API response. So that was the idea of having the different Answer types with their different .to_json() implementations.

Will take a look at your #12

This sounds like a good place to start with this data modeling. We have [...]

So this all makes sense to me, and it's how I would model it in a db. It looks like what you are describing matches up with what I already have here, except that:

• you're leaving out the Answer bits (not sure if because you didn't realise about the unsatisfied_requirements part of the API response, or if you are suggesting to handle it a different way?)

• you've generalised the two Rule types I mentioned earlier into one single piece of logic, namely:

A test case name (required)
A list of LDAP groups that can create waivers (optional)
A foreign key relationship to Policy

... which is fine, for those first two Rule types. The generalisation makes sense and we can represent it in the db easily enough.

But as soon as we want to expand it to have some other slightly different logic we will have to find a way to cover it in the Rule implementation and expand the db representation. Like the next Rule type we probably want to add is for Bodhi or Freshmaker, where the waiver requirement is not based on LDAP groups but rather whether the user who waived is the package owner or component QE contact looked up in PDC.

So that's why I'm resisting hooking it up to a database for now. I want to continue filling out a pure Python implementation like this, and once we have figured out all those edge cases we can pick a good database representation (and then put an API in front etc).

The whole reason why I introduced the Policy and Rule classes here to begin with is because I want them to become SQLAlchemy models, just like you are proposing... just not yet. :-)

The other thing that I am worried about -- and I have not fully fleshed out these thoughts in my head yet, but we need to consider it -- is that I think the policies need to actually be immutable.

If we want to change a policy it has to be a new policy (or a new version) with the old one preserved and marked as obsolete.

I think we need that so that historical Greenwave decisions are reproducible. Otherwise it means Greenwave could give me a different answer now, based on the same test results and waivers, compared with when I asked it to give me a decision 6 months ago.

I am not sure how exactly we should deal with this, but it probably means when we have a db representation we have to either do immutable, versioned rows in the db or else some kind of audit log which lets us reconstruct policies back in time based on how they were changed.

Those things are all possible to do, but I just don't want to tackle them just yet. The advantage of having the policies hardcoded in Python (like this patch) or else defined in some YAML files (our proposed strategy in the focus doc) is at least they are versioned with git and we have a way to figure out what the policy was at any given point in time.

I extracted the first two commits (the test changes) as PR#14 as they seem to be uncontroversial, and I have been cooking up another related patch to show what I mean about unit tests vs. functional tests, which depends on those two.

Just to reply to your first comment... The "answers" are not just yes/no, they also have to be able to describe themselves as "unsatisfied requirements" in the API response. So that was the idea of having the different Answer types with their different .to_json() implementations.

Okay. A much cleaner, clearer way to implement that is via exceptions.

... which is fine, for those first two Rule types. The generalisation makes sense and we can represent it in the db easily enough.

But as soon as we want to expand it to have some other slightly different logic we will have to find a way to cover it in the Rule implementation and expand the db representation. Like the next Rule type we probably want to add is for Bodhi or Freshmaker, where the waiver requirement is not based on LDAP groups but rather whether the user who waived is the package owner or component QE contact looked up in PDC.

Okay, this is good to know and has not really been expressed anywhere I've seen. It sounds like all the rule types so far revolve around who is and is not allowed to waive something. Is that accurate? Doesn't it make more sense for WaiverDB to enforce ACLs for who's allowed to make waivers? Shouldn't we trust that if the waiver is in WaiverDB, it is allowed to waive whatever it's waiving?

Okay. A much cleaner, clearer way to implement that is via exceptions.

I don't think exceptions are the right fit. I don't think it would be cleaner at all... These are not error conditions, or exceptional control flows, it is simply the policy deciding "no" with a corresponding reason. You could imagine an individual rule .check() raising an exception if the rule is not safisfied, but how would you combine the answers from multiple rules? Collect a list of Exception instances and return them? At that point it's not really exceptional control flow, you're just dealing with normal values. So I don't see any reason to use exceptions for them.

Okay, this is good to know and has not really been expressed anywhere I've seen.

Some of this is mentioned in the focus doc, but it's true that a lot of these ideas are just tentative ones that have floated around amongst the team or even just in my head, so far. That is why we are trying to get Greenwave to the point of making real decisions ASAP so we can flesh this stuff out some more. :-)

It sounds like all the rule types so far revolve around who is and is not allowed to waive something. Is that accurate? Doesn't it make more sense for WaiverDB to enforce ACLs for who's allowed to make waivers? Shouldn't we trust that if the waiver is in WaiverDB, it is allowed to waive whatever it's waiving?

We did consider that in the original WaiverDB design, I think there was a paragraph in its focus doc about this. We decided to keep WaiverDB "dumb", in the sense that it won't enforce any restrictions about who can waive, it will accept waivers from anyone -- and Greenwave will have the complicated logic for deciding whose waivers will count (anyone else's waivers will just be ignored).

Mainly this is because we very quickly started to come across edge cases, for example imagine a build, which has a failing test result in ResultsDB, but the package can be incorporated into multiple different products (think RHEL and OpenShift) which have different policies about who is allowed to waive which tests. Managing that on the WaiverDB side with ACLs didn't seem like the best approach, since Greenwave is already the piece where we want to handle complicated logic like that.

Anyway, I think we will start to get a better feel for how to arrange this stuff as Greenwave evolves to cover more policies... That is why I really want to fill out the rest of the RHEL7 policy (which will be my next PR after this one is done) and more importantly, expand it to other products and other gating points.

We did consider that in the original WaiverDB design, I think there was a paragraph in its focus doc about this. We decided to keep WaiverDB "dumb", in the sense that it won't enforce any restrictions about who can waive, it will accept waivers from anyone -- and Greenwave will have the complicated logic for deciding whose waivers will count (anyone else's waivers will just be ignored).

This will lead to a support burden - users will create waivers that get ignored, but they won't know why they are getting ignored, or why they weren't told when they created them that they will be ignored. These users will come for support often, and it will be a burden for the admins. Thus, If a user's waiver isn't going to count, it's important that the user be told this when trying to create it.

Let's worry about that problem if and when we hit it. Knowing whether a waiver will count means basically evaluating the entire policy which means it would need cooperation with Greenwave anyway.

I think the best place to deal with this will be at the UI layer. Even if the WaiverDB API accepts anyone's waiver, we can still make sure to offer the operation only when it will make a difference.

For example in Bodhi/ET we already know we are going to need to display detailed information about why an update is not allowed to be pushed yet -- so that might be the best place to offer the user to waive the failing results, if that's what's holding up the update from being pushed and if the user is someone whose waiver will be counted.

Posted v3, this is just a rebase on top of the test changes in #15 to fix conflicts.

:thumbsup:

I don't think exceptions are the right fit. I don't think it would be cleaner at all... These are not error conditions, or exceptional control flows, it is simply the policy deciding "no" with a corresponding reason. You could imagine an individual rule .check() raising an exception if the rule is not safisfied, but how would you combine the answers from multiple rules? Collect a list of Exception instances and return them? At that point it's not really exceptional control flow, you're just dealing with normal values. So I don't see any reason to use exceptions for them.

I don't agree - this is a very common pattern in Python. If you want to aggregate all the error messages, sure, you'd catch them all and call str on them or whatever to get your message. You're also free to not catch them and error out immediately, or handle the error higher up the stack. There are tons of examples both in the standard library and in popular 3rd party Python libraries of APIs raise exceptions in cases like this. For example, StopIteration is raised when the end of an iterator is reached and cryptography returns nothing if the signature is valid or raises an exception if it's not. Breaking with the language pattern has a high cost since new maintainers/contributors have to figure out a new pattern that does the same thing.

Even supposing we opt not to use exceptions (which appears to be the case since this is merged), there are plenty of oddities about this PR that I would have liked to see fixed before it was accepted. For example, your class hierarchy means you don't need a is_satisfied attribute at all because the type of the object tells you that. There are now two ways to check and that's a confusing API.

If everyone else is happy with this then this is what we'll do, but I do want to express that I don't think this will lead to clear, maintainable code. Furthermore, we're setting ourselves up to do a lot of this work twice since we know we want a database. The rule and policy classes could have been implemented as SQLAlchemy models and then not persisted. This approach doesn't help us move faster or more rapidly prototype, but now we have to port these classes and the code that uses them.

We merged this to unblock our other work, but that doesn't mean any of it is set in stone. There is nothing to stop us from refactoring the answers to use exceptions, if that makes it more Pythonic. And there is nothing stopping us from converting the Policy and Rule classes to be SQLAlchemy mapped later on too.