Add subscriptions projection #138
Conversation
dottorblaster
changed the title
dottorblaster
force-pushed
the
add-subscriptions-projector
branch
from
2d8fe2d to
b4cb1c1
Compare
dottorblaster
requested review from
fabriziosestito,
arbulu89,
nelsonkopliku,
rtorrero and
stefanotorresi
There was a problem hiding this comment.
@dottorblaster So far so good!
I have done some comments, and maybe let's wait until somebody else reviews it, but I like the code at least!
| @@ -0,0 +1,21 @@ | |||
| defmodule Tronto.Monitoring.Domain.Commands.UpdateSlesSubscriptions do | |||
| @moduledoc """ | |||
| Updated data relative to subscriptions. | |||
| @@ -0,0 +1,17 @@ | |||
| defmodule Tronto.Monitoring.Domain.Events.SlesSubscriptionsUpdated do | |||
| @moduledoc """ | |||
| Subscription updated event | |||
There was a problem hiding this comment.
Add final s maybe to Subscription in the docstring
|
|
||
| @derive Jason.Encoder | ||
| typedstruct do | ||
| @typedoc "Subscription value object" |
There was a problem hiding this comment.
Add Sles to the docstring maybe
| typedstruct do | ||
| @typedoc "Subscription value object" | ||
|
|
||
| field :host_id, String.t(), enforce: true |
There was a problem hiding this comment.
Is this host_id field ever used?
There was a problem hiding this comment.
I like to have it for completion, I don't have a strong opinion about it 😅
| @@ -109,4 +123,42 @@ defmodule Tronto.Monitoring.Integration.Discovery do | |||
| nil | |||
| end) | |||
| end | |||
|
|
|||
| defp parse_subscription_data(host_id, %{ | |||
There was a problem hiding this comment.
I don't know if there is any other option to have just an unique function here, instead of 2 with different values. It would be nice
There was a problem hiding this comment.
This is the same function with two clauses, (parse_subscription_data/2). An alternative would be grabbing the optional fields with Map.get https://hexdocs.pm/elixir/1.12/Map.html#get/3, but I think this version is a bit cleaner and I like the declarative approach
| @primary_key false | ||
| schema "sles_subscriptions" do | ||
| field :host_id, Ecto.UUID, primary_key: true | ||
| field :identifier, :string, primary_key: true |
There was a problem hiding this comment.
actually @arbulu89 helped me on this one 😄
dottorblaster
force-pushed
the
add-subscriptions-projector
branch
from
6deb1a5 to
f3dbd7f
Compare
dottorblaster
force-pushed
the
add-subscriptions-projector
branch
from
f2890f2 to
d35e7dc
Compare
As above, we need the subscriptions discovery to be projected in order to have a proper about page and host detail