Serve the execution results through an endpoint #40

dottorblaster · 2022-10-03T15:05:07Z

As above, we're exposing an API to look into historical execution results.

arbulu89

So far so good! I have noted some comments.
Could we add controller tests for the new endpoint?

lib/wanda_web/api_spec.ex

lib/wanda_web/controllers/executions_controller.ex

arbulu89 · 2022-10-04T06:53:21Z

lib/wanda_web/controllers/executions_controller.ex

+    items_per_page = Map.get(params, :items_per_page, 10)
+    group_id = Map.get(params, :group_id)
+
+    results = Results.get_execution_results(group_id, page, items_per_page)


Should we directly send the database representation of the result in the data field here?
I would expect some sort of transformation/mapping to send something more domain related.
I would maybe convert the ExecutionResult (which is the database mode) to the Result struct or something new. Something that we really need in the frontend, tailored for it.

we have views for that, i think we can iterate adding the views in a new PR.

https://hexdocs.pm/phoenix/views.html

@arbulu89 I'm super glad you noticed that, yeah it's something we have to do but I'd like to do that in a follow-up PR because I was afraid this PR was getting really huge

@dottorblaster Fine for me them! We can follow up.

arbulu89 · 2022-10-04T06:54:17Z

lib/wanda_web/controllers/executions_controller.ex

+    group_id = Map.get(params, :group_id)
+
+    results = Results.get_execution_results(group_id, page, items_per_page)
+    json(conn, %{page: page, items_per_page: items_per_page, data: results})


I don't really understand why we send back the page and item_per_page. They are coming directly from the request, so the user already know them.

+1 to remove them and maybe iterate to add the total count.
See for instance GH APi: https://api.github.com/search/repositories?q=tetris+language:assembly&sort=stars&order=desc&page=1&per_page=10

Can't we take inspiration from HTTP 206 (RFC 9110)?

I like how it proposed to send data on the HTTP header about the position of the served content, see here

Not suggesting do it on the header, just that I like the way it describes the range and total count

lib/wanda/results.ex

lib/wanda_web/controllers/executions_controller.ex

fabriziosestito

Some comments to be addressed and controller tests are missing, we need to test that the return json is in the format we specified in the open API spex.

Refer to this: https://github.com/open-api-spex/open_api_spex#validate-responses

We could use factory to generate a list of results.

nelsonkopliku · 2022-10-04T13:09:42Z

lib/wanda_web/api_spec.ex

@@ -0,0 +1,24 @@
+defmodule WandaWeb.ApiSpec do


In trento web we faced a similar situation in relation to apis/openapi/openapispex, see here

Let's consider If there's some valuable learning we can bring here - even on what not to do 😅

nelsonkopliku · 2022-10-04T13:11:27Z

lib/wanda_web/schemas/check_execution.ex

+    description: "A single execution of checks",
+    type: :object,
+    properties: %{
+      execution_id: %Schema{type: :string, description: "Execution ID"},


We could add a format: :uuid to execution_id and group_id, for a friendlier representation on the api doc.

I tried but I got an error, i googled a lot but I didn't find a solution that wasn't "create your own format with a huge regex"

Not talking about validation, no need for regex 😄

Just really a silly thing like we do here for example https://github.com/trento-project/web/blob/main/lib/trento_web/openapi/schema/host.ex#L37

%Schema{type: :string, description: "Execution ID", format: :uuid},

It would take care of giving that information in the swagger ui and it generate uuid samples.
If it is causing troubles feel free to skip.

nelsonkopliku · 2022-10-04T13:28:01Z

lib/wanda_web/schemas/check_execution.ex

+    properties: %{
+      execution_id: %Schema{type: :string, description: "Execution ID"},
+      group_id: %Schema{type: :string, description: "Group ID"},
+      payload: %Schema{type: :object, description: "Payload of the check execution"}


Probably you already thought of this.

What is the plan about the shape of the payload object?
As an API consumer, even if at the moment is just us 😄, I would find it useful to know that shape.

Does it have a fixed shape? I thought it was something that can really vary, therefore I didn't want to get stuck writing that schema right now, but it's material for a followup for sure

Well, it does have a shape, and that shape is Wanda.Execution.Result without :execution_id and :group_id.

Does it have a fixed shape? I thought it was something that can really vary

Until we change it 😄

The point to me is more
how stable do we think it is in relation to the needs?
What significant changes do we foresee to that representation in the upcoming iterations?

However, I understand it might be a tedious job, moreover if things change so fast that updating the documentation becomes a burden, yet if that happens there might be another kind of issue to tackle (coupling for instance, but let's leave it out)

Bottom line: if not planned to add that missing piece in this or following PRs, then I'd find it wise to track debt.
How does it sound?

Sounds good, I collected a followup task to better define that shape

arbulu89

Hey @dottorblaster ,
It looks good to me now. I have added some other minor comments. But feel free to omit them.

test/wanda_web/controllers/executions_controller_test.exs

lib/wanda_web/controllers/executions_controller.ex

arbulu89 · 2022-10-04T14:35:41Z

lib/wanda_web/schemas/check_execution_response.ex

+    description: "Check execution listing response",
+    type: :object,
+    properties: %{
+      page: %Schema{type: :string, description: "Current page"},


It looks like this got outdated

Damn you're right, thanks

fabriziosestito

LGTM, please track the debt and the leftovers

dottorblaster added 2 commits October 3, 2022 16:54

Add open_api_spex to deps

4d0e96d

Add openapispex to formatter rules

7814d76

dottorblaster added the enhancement New feature or request label Oct 3, 2022

dottorblaster self-assigned this Oct 3, 2022

dottorblaster force-pushed the endpoint-results branch 3 times, most recently from 04a42f4 to 6832695 Compare October 3, 2022 15:51

dottorblaster marked this pull request as ready for review October 3, 2022 15:52

dottorblaster requested review from fabriziosestito, arbulu89, nelsonkopliku and CDimonaco October 3, 2022 21:23

arbulu89 reviewed Oct 4, 2022

View reviewed changes

fabriziosestito reviewed Oct 4, 2022

View reviewed changes

lib/wanda/results.ex Outdated Show resolved Hide resolved

fabriziosestito reviewed Oct 4, 2022

View reviewed changes

lib/wanda_web/controllers/executions_controller.ex Outdated Show resolved Hide resolved

fabriziosestito suggested changes Oct 4, 2022

View reviewed changes

nelsonkopliku reviewed Oct 4, 2022

View reviewed changes

dottorblaster force-pushed the endpoint-results branch from d5f1a36 to 8afaf7e Compare October 4, 2022 13:49

arbulu89 approved these changes Oct 4, 2022

View reviewed changes

fabriziosestito approved these changes Oct 5, 2022

View reviewed changes

dottorblaster added 6 commits October 5, 2022 14:43

Add api specs

6494c02

Add a way to retrieve execution results in the Results context

79495c6

Lift off the executions controller

bc17455

Add tests

a37c430

Add Jason encoding derivation to ExecutionResult

db84075

Add controller test

7c8cb88

dottorblaster force-pushed the endpoint-results branch from e359999 to 7c8cb88 Compare October 5, 2022 12:49

dottorblaster merged commit 874a53b into main Oct 5, 2022

dottorblaster deleted the endpoint-results branch October 5, 2022 12:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serve the execution results through an endpoint #40

Serve the execution results through an endpoint #40

dottorblaster commented Oct 3, 2022

arbulu89 left a comment

arbulu89 Oct 4, 2022

fabriziosestito Oct 4, 2022

dottorblaster Oct 4, 2022

arbulu89 Oct 4, 2022

arbulu89 Oct 4, 2022

fabriziosestito Oct 4, 2022

rtorrero Oct 4, 2022

rtorrero Oct 4, 2022

fabriziosestito left a comment •

edited

Loading

nelsonkopliku Oct 4, 2022

nelsonkopliku Oct 4, 2022

dottorblaster Oct 4, 2022

nelsonkopliku Oct 4, 2022

nelsonkopliku Oct 4, 2022

dottorblaster Oct 4, 2022 •

edited

Loading

nelsonkopliku Oct 4, 2022

dottorblaster Oct 5, 2022

arbulu89 left a comment

arbulu89 Oct 4, 2022

dottorblaster Oct 4, 2022

fabriziosestito left a comment

Serve the execution results through an endpoint #40

Serve the execution results through an endpoint #40

Conversation

dottorblaster commented Oct 3, 2022

arbulu89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziosestito left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dottorblaster Oct 4, 2022 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arbulu89 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

fabriziosestito left a comment

Choose a reason for hiding this comment

fabriziosestito left a comment •

edited

Loading

dottorblaster Oct 4, 2022 •

edited

Loading