@lucarval @mprahl PTAL

Please don't overdo it with indentation (https://youtu.be/ZsHMHukIlJY?t=1262).

Prepare parameters for what? The name doesn't suggest what the docstring above says.

I know! I'm so terrible with vars/methods names... :( help...

I know! I'm so terrible with vars/methods names... :( help...

Me too. It's common trait even in great developers like us! :)

Rebased. Is it better?

Me too. It's common trait even in great developers like us! :)

So true... totally not our fault. It's good I don't have children. For their own sake.

Wouldn't this be a problem? I expect the latest result to change often, so I probably don't want to cache it.

Mmm. You might be right, and also: it doesn't really take much that request, no need to cache it probably. Thanks.

Removed the cache for the "retrieve_latest".

Aren't results sorted by time starting from latest? It that case you could do something like:

try:
    latest = next(results_retriever.retrieve(subject_type, subject_identifier))
except StopIteration:
    latest = None

I cannot find documentation for "/results/latest" resultdb endpoint (I'm looking at API docs here).

OK, so the "/results/latest" endpoint gives you latest results for each of available test case names. That's very useful.

+1

Aren't results sorted by time starting from latest? It that case you could do something like:
try:
latest = next(results_retriever.retrieve(subject_type, subject_identifier))
except StopIteration:
latest = None

I cannot find documentation for "/results/latest" resultdb endpoint (I'm looking at API docs here).

Documentation here. It has a very wonky name but it does exactly what we want :)

Why is this no longer needed?

UPDATE: I see now it has been moved. Disregard this comment.

It's odd that these two methods exist: _retrieve_helper and retrieve_helper. Can we rename _retrieve_helper to something like _retrieve_all? It's not great but at least we don't have very similarly named methods. We can then rename this retrieve_helper to _retrieve_helper to indicate this is an "internal" method.

Let's add some testing for this method. It'll be great to verify that it's actually hitting the right API endpoint.

Question: can we use /results/latest in all cases? I suppose the challenge is detecting when a decision has changed since that API endpoint doesn't seem to allow excluding results.

+1 to this comment

After the method is renamed, :thumbsup: from me. Looks great!

Yeah, sorry, I didn't notice it. I'll change it.

I've renamed the methods as suggested and added a test.

Question: can we use /results/latest in all cases? I suppose the challenge is detecting when a decision has changed since that API endpoint doesn't seem to allow excluding results.

I think it might be actually possible. The "ignore" thingy is in the /decision endpoint of greenwave itself. I can give it a try to change it. Let's see.

This seems unnecessary.

Just a minor comment, +1 otherwise!

Yeah, you are right, it was a mistake. Rebased. It should be complete now.

Optional: It's easier to read if you did something like:

request_url = self.url + '/results'
if latest:
    request_url += '/latest'

:thumbsup:

mmm ok, but it should be instead something like:

request_url = self.url + '/results'
if latest:
    request_url += '/latest'

No?

Well, you don't need the condition at all:

def _fetch_results(self, params):
    self._make_request('/results', params)
def _fetch_latest_results(self, params):
    self._make_request('/results/latest', params)
def _make_request(self, endpoint, params):
    request_url = self.url + endpoint
    ...

mmm ok, but it should be instead something like:

request_url = self.url + '/results'
if latest:
request_url += '/latest'

No?

Yup, a copy-paste error

:thumbsup:

:thumbsup:

Commit 0e50708e fixes this pull-request