Add projector for host checks selection #1542

EMaksy · 2023-06-19T09:54:44Z

Description

This pull request introduces a new projector function, Trento.HostProjector, which handles host checks selection. Additionally, broadcasting functionality has been implemented to notify subscribed clients of any changes.

The database read model and migration have been updated to include a new column, selected_checks, in the hosts table.

How was this tested?

Automated test was added + manually tested

dottorblaster

Just a nitpick then we can merge

lib/trento/application/projectors/host_projector.ex

arbulu89

Some more nitpicks

priv/repo/migrations/20230616114036_add_selected_checks_to_hosts.exs

test/trento/application/projectors/host_projector_test.exs

EMaksy · 2023-06-21T15:43:25Z

lib/trento/application/projectors/host_projector.ex

+      changeset =
+        HostReadModel
+        |> Repo.get(id)
+        # TODO: couldn't make it work with Ecto.Multi


So, about this, i don't like it, but like in the cluster projector, but i spend multiple hours to make it work "right" but i could not find a way for it.
See:

web/lib/trento/application/projectors/cluster_projector.ex

Line 92 in 32b8064

# TODO: couldn't make it work with Ecto.Multi

So i'm open for suggestions or we track it as a tech debt, what do you think?

@EMaksy There are some reasons why this is happening. The main one is that you have default: [] for selected_checks in the read model. With this, ecto has some emtpy_fields logic which if the values is empty, it will default it, so in this case, as the default and the new empty value are the same, there is no changeset.

https://hexdocs.pm/ecto/Ecto.Changeset.html#cast/4

With this default, you are forced to have something different. Something like HostReadModel.changeset(%HostReadModel{id: "240f96b1-8d26-53b7-9e99-ffb0f2e735bf", selected_checks: [nil]}, %{selected_checks: []}) would work, but in the past we agreed to not use it, as it it looks like a trick.

The same happens when you want to set nil to values, as most of the times nil is the default value.

There are other options like setting force_changes in the cast function within our changeset, but this would require to change the changeset function to allow additional options, which looks an overkill.

I would go with the current solution.
In any case, i would remove the comment above, or at least the reference to ClusterReadModel

Bottom line, same as @arbulu89 said. We already had this debate a couple times, and this is just the right way to do that. We should actually remove that comment in some places 😬

Okay guys thanks for the additional information, then let's stick with this implementation, i will remove the comment :D

arbulu89 · 2023-06-22T06:27:28Z

lib/trento/application/projectors/host_projector.ex

+    message =
+      HostView.render(
+        "host_checks_updated.json",
+        %{host: %HostReadModel{id: id, selected_checks: checks}}


Don't use HostReadModel here as the view doesn't expect that.
It simply expects a map with id and selected_checks.
%{id: id, selected_checks: checks} should be it

arbulu89 · 2023-06-22T06:36:31Z

lib/trento/application/projectors/host_projector.ex

+        %{host: %HostReadModel{id: id, selected_checks: checks}}
+      )
+
+    TrentoWeb.Endpoint.broadcast("monitoring:hosts", "host_checks_updated", message)


I wouldn't create a new event for this. We can use host_details_updated I guess.
This way, we already handle this event and we don't need to touch the frontend redux state.

Agreed. Let's keep it consistent with what happens in the cluster.

Yeah sounds good to me i won't add the host_checks_updated event and stick with host_details_updated

arbulu89 · 2023-06-22T06:52:41Z

lib/trento/application/projectors/host_projector.ex

+      changeset =
+        HostReadModel
+        |> Repo.get(id)
+        # TODO: couldn't make it work with Ecto.Multi


@EMaksy There are some reasons why this is happening. The main one is that you have default: [] for selected_checks in the read model. With this, ecto has some emtpy_fields logic which if the values is empty, it will default it, so in this case, as the default and the new empty value are the same, there is no changeset.

https://hexdocs.pm/ecto/Ecto.Changeset.html#cast/4

With this default, you are forced to have something different. Something like HostReadModel.changeset(%HostReadModel{id: "240f96b1-8d26-53b7-9e99-ffb0f2e735bf", selected_checks: [nil]}, %{selected_checks: []}) would work, but in the past we agreed to not use it, as it it looks like a trick.

The same happens when you want to set nil to values, as most of the times nil is the default value.

There are other options like setting force_changes in the cast function within our changeset, but this would require to change the changeset function to allow additional options, which looks an overkill.

I would go with the current solution.
In any case, i would remove the comment above, or at least the reference to ClusterReadModel

test/trento/application/projectors/host_projector_test.exs

arbulu89 · 2023-06-22T07:06:02Z

test/trento/application/projectors/host_projector_test.exs

+      %{checks: []}
+    ]
+
+    Enum.each(test_checks_data, fn %{checks: checks} ->


I don't know if people is aware of the common for in elixir hehe
for %{checks: checks} <- tests_checks_data do

Oki fair point, Enum.each was just the first thing which shot in my mind :D

arbulu89 · 2023-06-22T07:07:36Z

test/trento/application/projectors/host_projector_test.exs

+  test "should update the selected_checks field when event is received" do
+    %{id: host_id} = insert(:host, id: UUID.uuid4())
+
+    test_checks_data = [


Not a big fan of this name. I would go with cases or scenarios.
Anyway, not strong opinions at all

Fair enough :D

nelsonkopliku

Great @EMaksy! Welcome to the es/cqrs wonders 😄
Nothing to add besides what already shared.

dottorblaster

Once you adress @arbulu89's comments LGTM

EMaksy added enhancement New feature or request elixir Pull requests that update Elixir code labels Jun 19, 2023

EMaksy self-assigned this Jun 19, 2023

EMaksy force-pushed the host-checks-projector branch 6 times, most recently from 71ca1f9 to 45a3a62 Compare June 19, 2023 14:20

EMaksy requested review from dottorblaster, nelsonkopliku and arbulu89 June 19, 2023 14:21

dottorblaster requested changes Jun 19, 2023

View reviewed changes

lib/trento/application/projectors/host_projector.ex Outdated Show resolved Hide resolved

arbulu89 requested changes Jun 19, 2023

View reviewed changes

priv/repo/migrations/20230616114036_add_selected_checks_to_hosts.exs Outdated Show resolved Hide resolved

test/trento/application/projectors/host_projector_test.exs Outdated Show resolved Hide resolved

test/trento/application/projectors/host_projector_test.exs Outdated Show resolved Hide resolved

EMaksy force-pushed the host-checks-projector branch 3 times, most recently from 8fdf446 to c99ecfc Compare June 21, 2023 15:33

EMaksy commented Jun 21, 2023

View reviewed changes

EMaksy force-pushed the host-checks-projector branch from 32b8064 to fd868a2 Compare June 21, 2023 15:56

EMaksy marked this pull request as ready for review June 21, 2023 16:00

arbulu89 requested changes Jun 22, 2023

View reviewed changes

nelsonkopliku reviewed Jun 22, 2023

View reviewed changes

dottorblaster approved these changes Jun 22, 2023

View reviewed changes

arbulu89 approved these changes Jun 22, 2023

View reviewed changes

EMaksy force-pushed the host-checks-projector branch from eb2364d to d770eaf Compare June 22, 2023 11:44

nelsonkopliku approved these changes Jun 22, 2023

View reviewed changes

EMaksy force-pushed the host-checks-projector branch from d770eaf to 9441dd1 Compare June 22, 2023 11:45

EMaksy added 3 commits June 22, 2023 13:45

Add host selection projector

5231cad

Update host read model and add migration

0e4b525

Add host selected checks view

610d756

EMaksy force-pushed the host-checks-projector branch 2 times, most recently from 8820908 to 29bc275 Compare June 22, 2023 11:53

Use host_details_updated view and event

29bc275

EMaksy merged commit 5d04a50 into main Jun 22, 2023

EMaksy deleted the host-checks-projector branch June 22, 2023 12:05

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add projector for host checks selection #1542

Add projector for host checks selection #1542

EMaksy commented Jun 19, 2023 •

edited

Loading

dottorblaster left a comment

arbulu89 left a comment

EMaksy Jun 21, 2023

arbulu89 Jun 22, 2023

dottorblaster Jun 22, 2023

EMaksy Jun 22, 2023

arbulu89 Jun 22, 2023

dottorblaster Jun 22, 2023

arbulu89 Jun 22, 2023

nelsonkopliku Jun 22, 2023

EMaksy Jun 22, 2023

arbulu89 Jun 22, 2023

arbulu89 Jun 22, 2023

EMaksy Jun 22, 2023

arbulu89 Jun 22, 2023

EMaksy Jun 22, 2023

nelsonkopliku left a comment

dottorblaster left a comment

Add projector for host checks selection #1542

Add projector for host checks selection #1542

Conversation

EMaksy commented Jun 19, 2023 • edited Loading

Description

How was this tested?

dottorblaster left a 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

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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nelsonkopliku left a comment

Choose a reason for hiding this comment

dottorblaster left a comment

Choose a reason for hiding this comment

EMaksy commented Jun 19, 2023 •

edited

Loading