Handle unreachable checks execution scenario #304
Conversation
arbulu89
requested review from
dottorblaster,
fabriziosestito,
rtorrero,
stefanotorresi and
nelsonkopliku
|
Hey @trento-project/developers , |
arbulu89
force-pushed
the
handle-unreachable-execution-state
branch
from
b161dbf to
c9d22ae
Compare
arbulu89
force-pushed
the
handle-unreachable-execution-state
branch
from
c9d22ae to
615045c
Compare
|
|
||
| @derive {Jason.Encoder, except: [:__meta__, :__struct__]} | ||
| @primary_key false | ||
| schema "hosts_checks_results" do |
There was a problem hiding this comment.
hosts checks results or hosts checks execution?
There was a problem hiding this comment.
Between the two I'd prefer HostsChecksExecution.
Just as a discussion triggering point:
I cannot also think about the possibility of it being at the moment just ChecksExecution 🤔
anyway it is bounded to the context of a host, right?
We can refine also on the language and see whether some extra differentiation is needed.
There was a problem hiding this comment.
Yep, HostsChecksExecutions looks more appropriate. I think the Hosts prefix helps to identify that the data is Host related (even though it is still a bit abstract to understand what is it about...(
| @@ -24,6 +27,7 @@ defmodule Trento.ClusterReadModel do | |||
| field :details, :map | |||
| field :checks_execution, Ecto.Enum, values: [:not_running, :requested, :running] | |||
|
|
|||
| has_many :hosts_executions, HostChecksExecutionsReadModel, foreign_key: :cluster_id | |||
There was a problem hiding this comment.
is there a reason why we are not nesting check results under checks execution?
is it more ergonomic on the frontend side?
There was a problem hiding this comment.
It was much more hassle to do so, and without big advantages. It forces you to do more hierarchical operations to get the checks (loop through hosts, and them loop through the checks), and as we were already storing the host_id for each check, I thought that it was good enough. In fact, as you commented, it is easier to handle the 2 lists independently in the frontend
There was a problem hiding this comment.
Perfect, i was just curious about the rationale. at the end read models are for, well, reading, so it make sense to make them closer to the usecase.
priv/repo/migrations/20220405142819_create_host_checks_results.exs
Outdated
Show resolved
Hide resolved
|
Woh lots of stuff, great! Could you rewrite the history to make the review easier? Thanks! |
There was a problem hiding this comment.
Great job!
No big thing from my side, @fabriziosestito already pointed out good comments.
Naming is always difficult and good design too 😄
|
|
||
| @derive {Jason.Encoder, except: [:__meta__, :__struct__]} | ||
| @primary_key false | ||
| schema "hosts_checks_results" do |
There was a problem hiding this comment.
Between the two I'd prefer HostsChecksExecution.
Just as a discussion triggering point:
I cannot also think about the possibility of it being at the moment just ChecksExecution 🤔
anyway it is bounded to the context of a host, right?
We can refine also on the language and see whether some extra differentiation is needed.
Handle the unreachable checks execution scenario.
The implementation was not that simple.
Initially, I thought of adding a new
NotReachedevent or similar (actually, some intermediate commits have this code), but eventually I saw that the code was being duplicated, and the only real difference was the name of the event. I thought that it is not enough the hassle.Please, don't judge the UI, I didn't have enough time to work on it. But I wanted to open the PR on draft so you can have a view while I'm away.
PD: The tests are still missing
Here how it looks like:
