Ecto decode clusters discovery

[rtorrero]

This big PR basically continues the effort to use Ecto to define and validate the payloads for the different discovery policies, already started by PRs #602, #595, #582.

This time it's the turn for the cluster discovery. Some notes/clarifications:

• Sometimes I use nested / inline deftype's such as:

  deftype do
   embeds_one :configuration, Configuration do
     embeds_one :resources, CibResources
   end
 end

Because it feels "wrong" to have all the boilerplate to create a new deftype just for one or few fields. The line on when to do this vs using a deftype is a bit blurry to me, so suggestions accepted on that.

• Sometimes I had to use cast(attrs, []) (cast with nothing to cast) to be able to define deftypes that are pure nested structs (with no fields). Thanks to @fabriziosestito for poiting this out, looked weird initially but it makes sense now:

  def changeset(cib, attrs) do
   cib
   |> cast(attrs, [])
   ...

• Sometimes, the agent sends null on a field that should have a list. This causes the payload validation to fail, and for such cases, the "workaround" that @arbulu89 came up with was to convert these fields to lists before the changeset get's called, passing a filtered_attrs to it.

   defp transform_nil_lists(%{"groups" => groups} = attrs) do
     attrs
     |> Map.put("groups", ListHelper.to_list(groups))
   end
   
   def to_list(list) when is_list(list), do: list
   def to_list(nil), do: []

Comments appreciated!

[nelsonkopliku]

Unfortunately the pipeline is failing here

I ran cypress locally and I get failures, did you try that locally, too?

Not sure if this helps, but I noticed quite some (many duplication) discarded_discovery_events.
Are those expected to be discarded? 🤔

EDIT: here's how it fails locally, seems the same failure of the pipeline.

[rtorrero]

Unfortunately the pipeline is failing here

I ran cypress locally and I get failures, did you try that locally, too?

Not sure if this helps, but I noticed quite some (many duplication) discarded_discovery_events. Are those expected to be discarded? thinking

EDIT: here's how it fails locally, seems the same failure of the pipeline.

Yep, I was just testing and indeed I get the same failures locally (e.g. cluster names are missing from some places, etc). Looking into it now.

[rtorrero]

I'd guess that there are some payloads being sent during the E2e tests that include some additional arrays receiving null instead of []. If that's the case, should we apply the filtering to change it to [] on all fields that expect an array?

[arbulu89]

Hi @rtorrero ,
I did an initial review.
Couldn't check all the changes in the cluster_policy.ex file.
In any case, as long as the unit tests and the E2E tests are failling, some changes look required

[arbulu89]

We could maybe define this field as a map. I think that sbd has more options than these ones, and as we don't really do any real validation, a map would work.
This is basically the information coming from the sbd configuration file, which has a key=value format, and mapping all the flags here looks a bit an overkill

[nelsonkopliku]

I tend to agree with Xabi.

What I can say is that if we mapped ALL the fields and we're sure about that, we could also keep it like this.
Otherwise, if we are not fully confident we could be a bit less rigid as xabi is suggesting.

[rtorrero]

Ok, I just checked a bit and there is indeed at least one more field that we are not tracking right now. Let's just switch it to a map.

[nelsonkopliku]

I'd guess that there are some payloads being sent during the E2e tests that include some additional arrays receiving null instead of []. If that's the case, should we apply the filtering to change it to [] on all fields that expect an array?

First thing I'd say let's clearly identify the cause, which might be very well what you mention, and if that's the case it makes sense to make sure all lists fields, if received as null are transformed to an empty list.

[rtorrero]

It should be passing now; I wasn't properly handling cases where there is no SID on the payload.

[arbulu89]

Hi @rtorrero ,
A long journey hehe

I find the PR really good, and I'm approving it, but still adding some comments.
I find having the to_list usage in the cluster policy code is a pretty big smell. The lists should already be sanitazed in the payload.

Besides that, we should open a tech-debt ticket in our backlog to change the agent to send empty lists properly instead of null values. I find that at any time this code might explode because we receive some null where we have the embeds_many option. And I think we should work on this sooner than later.

I couldn't really review the policy side, I hope that having the tests passing is our best review here, as the code is just an adaptation rather than new changes

Congratulations for the great work!

[nelsonkopliku]

[...] I find having the to_list usage in the cluster policy code is a pretty big smell. [...]

[...] tech-debt ticket in our backlog to change the agent to send empty lists properly instead of null values. [...]
at any time this code might explode [...]

I embrace @arbulu89 comments, we can improve code smells (not by making them smell more ) and make the involved parties - agent and server - to properly adhere to the contracts.

Great effort nevertheless! Thanks!

Approving, hope tests cover enough 😅
Let's be ready to act if anything comes up