This is a duplicate import from above

It might make more sense to have the request parsers live on the Resource classes

We definitely want to use connection pooling here as well as a (possibly configurable) timeout on requests since by default they have no timeout. All you need to do is create a global Session for connection pooling.

There's a lot of reasons that this could have not been an HTTP 200. For example, the service might be temporarily unavailable (HTTP 503). It seems odd to use HTTP 400 here, as well, since the request could be well-formed, but the resource simply doesn't exist (404).

HTTP 201 seems incorrect. It's a GET request with the requested resource, so it should return HTTP 200.

I think we might want to think a bit about the URL scheme before I get too far into a review (and I know this is a WIP :grinning: )

GET /api/v1.0/decision?product_version=rhel-7&decision_context=errta_newfile_to_qe&subject=java-1.8.0-openjdk-1.8.0.131-3.b12.el7_3

seems a odd to me. Since all three query parameters are required, they really should just be part of the URL, so it would look something like:

GET /api/v1.0/decision/<product_version>/<decision_context>/<subject>/

although maybe that order doesn't make logical sense.

Either option seems fine to me.

Pointing out that all three query parameters are required and that the request will return an error if any of them are missing is a valid observation. But the same problem maps to the alternative API you suggested, @jcline:

Since /decision/ is meaningless without all three, what do we do about half-formed URLs? Should GET /api/v1.0/decision/<product_version>/ return a 400 error or a 404 error?

That said, I do think I like the slash-delimited form a bit better.

I would expect such a request to return a 404. It's a valid URL and the request itself is well-formed, but there's nothing there. I know everyone has different opinions about which error response code to use, but I see HTTP 400 as being used for problems with the syntax of the request itself. e.g. a client tells me the Content-Length is a negative number, or that the body is application/json and it's not (or I have a schema for the JSON I accept and the request doesn't adhere to that schema).

They're basically equivalent, but the query string variety is just unusual (to me) as far as APIs go.

The three query params are required for now but we will probably want to relax that in future (or more likely, add more possible params). In particular, I am sure we will want to change the subject parameter in future. The RPMDiff results (where the item is a pair of RPMs) and the OpenQA results (where the item is a compose... I think) are going to make it more complicated.

In fact we will probably end up needing to make the endpoint accept POST with JSON so that we can express more complex values for the subject.

If the query params approach is not sitting right with people, we could just switch it to a POST with JSON now?

Either way I think putting the params into the URL path is not a good choice.

The three query params are required for now but we will probably want to relax that in future (or more likely, add more possible params). In particular, I am sure we will want to change the subject parameter in future. The RPMDiff results (where the item is a pair of RPMs) and the OpenQA results (where the item is a compose... I think) are going to make it more complicated.

In fact we will probably end up needing to make the endpoint accept POST with JSON so that we can express more complex values for the subject.

If the query params approach is not sitting right with people, we could just switch it to a POST with JSON now?

Either way I think putting the params into the URL path is not a good choice.

(Sigh... how do I manage to keep double-commenting in Pagure?)

And yeah, the error code for a missing query param (or a query parameter value which doesn't match anything) should be 404.

404 means the URL doesn't exist, and the query string is a part of the URL just the same as the path is.

/decision?product_version=rhel-7&... -> 200
/decision?product_version=rehl-7&... -> 404 because the product doesn't exist
/decision -> 404 because a decision without any product doesn't exist

Of course the 404 would still ideally say what exactly is missing (not just "URL not found").

Don't save these globally at import time -- the config will not be loaded yet. You will want to reference .config[...] inside each request handler as you need it.

Flask-restful might be overkill for this service. You might find it is simpler to just write the request handlers by hand and grab the args out. It's up to you. I suggest try writing a version without flask-restful and if it is the same amount of code or less, then drop it. :-)

If we don't find any applicable policies, we should probably assume the product is not a real product and give back a 404 indicating that.

Yeah if we have an error responding to this request, it's not a BadRequest, it's a 5xx code.

For now the easiest approach is don't handle any errors at all, let everything raise out and give back a 500. We can refine the error handling later.

So for this code block that means you can just do:

response = requests.get(...)
response.raise_for_status()
results = response.json()['data']

Btw get_req is not a good variable name. The return type of requests.get() is a Response not a Request.

We will probably want to factor this out into some kind of "matchers", then each policy would be composed of multiple matchers. Anyway this is fine for now. As we expand the policies (and once we have good tests!) we can refactor.

Yeah so I think the "matchers" idea will help us here, with building up this output. We probably want this to be an array, where each item describes a single thing that was missing (or failed).

[ { 'type': 'test-result-missing', 'item': '...', 'testcase': '...' },
  { 'type': 'test-result-failed', 'item': '...', 'testcase': '...', 'result_id': '...' } ]

So then the caller knows for test-result-missing they have to figure out why the test case didn't run. For test-result-failed they have to either get it to pass or waive it. etc.

If we represent (internally) the policies as a set of rules, then we should find a nice correspondence from rule type -> Matcher class -> "type" in this return value.

Maybe drop this condition like we did in Waiverdb.

Ohh I missed the fact that that other bit of code was extracting this error string from the JSON response body... hmm. Not sure what is the cleanest way to handle it. Maybe a custom version of .raise_for_status() which annotates the exception with this message, if it's present?

When does ResultsDB (or WaiverDB) return an HTTP error with a JSON message key? Does that ever happen? I can't find any mention of it in the ResultsDB API docs. Maybe we don't need to bother with this at all?

Thanks.

Yeah, it might be easier as if what we have done in beaker.

Good point.

The JSON message key is returned in ResultsDB, not in WaiverDB. This method is copied from https://github.com/release-engineering/resultsdb-updater/blob/master/resultsdbupdater/utils.py

IMO, switching it to a POST with JSON may make more sense since the caller is expecting the API server to make/create a decision, even though we're not storing the decision.

Me too. I'm swayed by the argument that we'll need to "express more complex values."

+1 to POSTing a JSON body

Just have another look and I think you are right. We don't need to bother with this at all as the message key is only returned in POST requests in ResultsDB.

Rebased to address the code review comments:

• change /decision to POST
• add tests

Rebased to add the resultsdb api url and the waiverdb api url for the dev environment.

If we are going full container, then we can probably take this stuff back out. There is no real solution for structured logging in OpenShift, it's all just plain text to stdout (a big step backwards IMHO).

If we are going full container, then we can probably take this stuff back out. There is no real solution for structured logging in OpenShift, it's all just plain text to stdout (a big step backwards IMHO).

Actually looking at the ruleset it seems there is no actual requirement that the RPMDiff results pass or are waived. :-) But we can fix that in a later patch once this is merged...

I am not on board with this VCR cassette stuff.

It just produces these huge, unreadable blobs of YAML and if we ever need to change anything in the responses, someone has to re-record them by hand and you get a massive unreadable diff with no idea what is changing.

I think we really need to either:

1. write unit tests with mock Waiverdb and Resultsdb services

2. write functional tests which talk to a real Greenwave service deployed in OpenShift, and include ResultsDB and WaiverDB in the OpenShift template so they get deployed beside it

I am much more strongly in favour of option 2, because I always prefer tests that run the code in a realistic way as part of a real deployment, rather than faking pieces out.

But I think the key is that each test case should be specifying what setup it expects (in terms of, results and waivers existing) and then asserting the correct response comes back from Greenwave.

Probably the neatest approach for testing would be two-pronged:

If we introduce the idea of each policy consisting of "matcher" objects, like I floated earlier... the matchers would not be making HTTP requests, they would just deal with results and waivers in, and answers out. It should be easy to thoroughly unit test these without any HTTP request stuff.

And then on the outer layer, we have functional tests which interact with a real ResultsDB+WaiverDB+Greenwave in OpenShift. The test would POST some results to ResultsDB, POST some waivers to WaiverDB, and then ask Greenwave for a decision and assert it is correct.

We would not need too many heavy-weight functional tests because all the edge cases for individual policies could be covered by unit tests against the "matchers".

... But in the interest of keeping this pull request small, maybe let's leave out the tests entirely from this one and we can iterate on them separately? For the functional testing I guess we would want a Jenkinsfile, .spec file, Dockerfile, and OpenShift templates similar to WaiverDB...

Re: specifying what setup a testcase expects, I wonder if this would be a good use-case for custom Ansible modules. You would specify in yaml the data that should exist for each service, and the modules would handle the specifics of getting that data into the service, whether it's direct database inserts, XMLRPC POSTs, REST calls, etc. These modules could be reused for setup/bootstrapping of production services as well.

Well, I would say it doesn't hurt to have this here.

The VCR.py is being used in MBS and resutlsdb-updater. It states that it can simplify and speed up tests that make HTTP requests. However, it has a trade-off which would include some unreadable chunks of YAML. For each test, it does specify what data it expects from ResultsDB and WaiverDB, though it is not quite readable in the YAML file. But yeah, it might be overkill here.

The option 2 sounds great, but I think we can go with the option 1 for now as a quick solution.

I've changed to use requests-mock instead of the VCR.py for the tests. I would like to keep the tests here in order to prove that the code actually works as expected. We could refactor the code when the idea of using "matcher" objects in the policies is introduced.

Ahh very nice! I greatly prefer this requests_mock approach over the vcrpy stuff. It makes it easy to see in each test case what fake HTTP requests we are relying on, and to change them as the tests evolve.

And I agree, we can refactor the policies further and improve the tests in subsequent PRs. This is a good starting point.

:+1: from me to merge this!

@mjia any reason not to merge this?

Do you have sufficient rights in pagure?

This approach defeats the purpose of using a session since the session is discarded after each request is handled. The session must live beyond the request/response cycle.

I know there was a decision to not use a REST framework, but this schema validation could be handled in a much more readable way with any number of libraries. Something like marshmallow (which, incidentally, flask-restful is going to start recommending) would do this and it pulls the schema definition/validation out of the request handler.

Just a thought.

I don't see any error handling code for the exception this raises, so the response is likely an HTTP 500 rather than a more useful error message about the service being unavailable or something.

The cleanest way to handle this likely with an error handler for the various requests exception types.

I suggest refactoring this function into several smaller functions that the route handler ties together. It's difficult to follow due to the multiple nested loops and if statements.

Yeah, I believe this will more likely be changed when more policies are added. I would like to keep it like that for the first version.

Marshmallow might be a good option if we have many APIs doing this like that. For this small app, it might be overkill. I would rather improve the error messages to be more readable.

I'm not quite sure what you mean since I'm using a context manager here which creates a global session. It will be closed once all the requests inside the context manager are handled.

Yeah, I will fix this in the following PRs.

@mjia any reason not to merge this?
Do you have sufficient rights in pagure?

Yes, I can merge this. @jeremy, please open an issue if you think my replies regarding your code reviews do not make sense.