Process execution completed

[arbulu89]

Description

Process ExecutionCompleted event coming from wanda. The execution_id is set as correlation_id of the health changes command, so we can easily match the event with the execution data.

I have the feeling that I have over engineered the process part, to make the functions more pure and testable. If you want, have a look removing the last commit (basically, handling the command dispatch in process instead of using the policy)

Fixes # (issue)

Did you add the right label?

Remember to add the right labels to this PR.

How was this tested?

Tests added

[rtorrero]

LGTM but maybe others want to take a look at this too before merge 😅

[nelsonkopliku]

Generally looks going in the right direction.

Main points are:

• not clear about the commanded adapter, maybe I am missing something
• different approach in transitioning from runner to wanda (new wanda specific command)

[nelsonkopliku]

Considering the option of relying on the existing Trento.Domain.Commands.CompleteChecksExecution this could be refactored by matching a CompleteChecksExecution with a health property, to differenciate.

No hard feelings, I understand the temporary approach, so proceed as you wish.

The fact is that, personally, in this case I would find enough altering the domain by just adjusting an already stable concept by tweaking a bit its shape rather than adding another temporary command, which anyway is temporary 😄

Different ways to achieve the same 😄

[dottorblaster]

@nelsonkopliku I think this is a good case where common sense can help us instead, I like very much introducing a new command just for the time being and phasing that out as things evolve. I think that commands are really ephemeral and I wouldn't feel comfortable adapting something old to a new usecase. But I mean, by the time we end this discussion, we'll have a newer solution knocking at the door 😄

[arbulu89]

For me it was the easiest way of tracking the transition and knowing in the future, what we can safely remove and what not. This is not a temporary solution. This is the final solution simply with a Wanda suffix at the end, as a reminder that the command is used with the wanda version.

[nelsonkopliku]

Let's go with what we have, as said, I am fine with it 🚀

For me to better understand: we're gonna end up with only a command named Trento.Domain.Commands.CompleteChecksExecution (without Wanda) which will have these fields

defcommand do
  field :cluster_id, Ecto.UUID
  field :health, Ecto.Enum, values: Health.values()
end

instead of these

defcommand do
  field :cluster_id, Ecto.UUID
  embeds_many :hosts_executions, HostExecution
end

Got it right?

[arbulu89]

Yes

[nelsonkopliku]

I was initially thinking would emitting a ClusterHealthChanged be enough?

Probably not, I see that this new event gives a higher granularity of what is happening in the system and it becomes interesting when we want to react upon change of the aggregated result of a check execution.

So far only the UI would be interested in reacting to such an event, right?

[arbulu89]

At the end, we need to change the aggregate with this new checks health value to compute the final value, so an event must exist for that (we cannot change the aggregate otherwise).
So yes, it is simply an intermediate event (the same way we had the same event for the discovered values from the agent).

Maybe there is some better way to handle everything only with the ClusterHealthChanged, but at the end, this event would become more complicated, as it would need to take care of changing the aggregate in different scenarios and with different events.

[dottorblaster]

Mega LGTM

[dottorblaster]

@nelsonkopliku I think this is a good case where common sense can help us instead, I like very much introducing a new command just for the time being and phasing that out as things evolve. I think that commands are really ephemeral and I wouldn't feel comfortable adapting something old to a new usecase. But I mean, by the time we end this discussion, we'll have a newer solution knocking at the door 😄

[fabriziosestito]

LGTM, i like how the cluster aggregate is getting easier to follow

[nelsonkopliku]

Let's go!