Handle saptune payload #1801

CDimonaco · 2023-09-11T09:26:11Z

Description

Handle Saptune payload in Host Policy.

How was this tested?

Automated tests

arbulu89

Hey @CDimonaco ,
Seeing the code, my major concern is this "transformation" of the payload in the policy. Not that it is done there. How it is done.
You are combining the updating the incoming message data.
Wouldn't be cleaner to get the data from the message and create a new map from scratch;
For example

%{packaget_version: package_version} = result = Map.get(payload, "result")
{}
|> Map.put("package_version", package_version)
|> Map.put("services", build_saptune_service(result)
|> Map.put("enabled_solutions", build_saptune_enabled_solutions(result)
    ...

Maybe it is a bit more verbose, but it you totally build the payload, without depending that much in the incoming data. In the current way it is really attached

arbulu89 · 2023-09-11T09:35:11Z

lib/trento/application/integration/discovery/payloads/saptune_discovery_payload.ex

+  deftype do
+    field :package_version, :string
+    field :configured_version, :string
+    field :tuning_state, :string


This could be an enum as we only have 3 options: compliant, not compliant and no tuning..
(yes, with spaces, we could change that as well to use snake_case I guess)

lib/trento/application/integration/discovery/payloads/saptune_discovery_payload.ex

lib/trento/application/integration/discovery/policies/host_policy.ex

test/fixtures/discovery/saptune_discovery.json

lib/trento/application/integration/discovery/payloads/test_data.text

lib/trento/application/integration/discovery/policies/host_policy.ex

arbulu89

Hey @CDimonaco,
The code looks good in general.
I dropped some comments.
Besides that, can we add moduledocs to all the files?

I guess once we have tests we will see if all the code is correct

lib/trento/application/integration/discovery/payloads/saptune_discovery_payload.ex

lib/trento/application/integration/discovery/policies/host_policy.ex

lib/trento/domain/value_objects/saptune_staging.ex

arbulu89 · 2023-09-12T10:04:13Z

lib/trento/domain/value_objects/saptune_staging.ex

+  deftype do
+    field :enabled, :boolean
+    field :notes, {:array, :string}
+    field :solutions_ids, {:array, :string}


I would call this solutions

idk solutions in other value object refer to the whole solution object, I prefer to stick to ids, to just not confuse the stuff 👀

well, but you have notes above, which is a list of note ids as well...
so we follow the same rule in both entries I guess

lib/trento/domain/value_objects/saptune_service_status.ex

lib/trento/application/integration/discovery/policies/host_policy.ex

arbulu89

Hey @CDimonaco , @EMaksy
All looking great.
Once we have the services entry sorted out we can merge

test/fixtures/discovery/saptune_discovery_empty_status.json

arbulu89 · 2023-09-13T09:26:58Z

lib/trento/domain/value_objects/saptune_staging.ex

+  deftype do
+    field :enabled, :boolean
+    field :notes, {:array, :string}
+    field :solutions_ids, {:array, :string}


well, but you have notes above, which is a list of note ids as well...
so we follow the same rule in both entries I guess

arbulu89 · 2023-09-13T09:30:56Z

lib/trento/application/integration/discovery/policies/host_policy.ex

+       }),
+       do: %{"id" => solution_id, "notes" => note_list, "partial" => partially_applied}
+
+  defp format_saptune_staging_informations(%{


Why not simply format_saptune_staging ?
In any case the final informations with a plural s looks wrong

lib/trento/application/integration/discovery/policies/host_policy.ex

arbulu89

Green light from my side!
Once the needed module docs are there, we can merge

arbulu89

@EMaksy Some suggestions on the moduledocs

lib/trento/domain/value_objects/saptune_note.ex

lib/trento/domain/value_objects/saptune_service_status.ex

lib/trento/domain/value_objects/saptune_solution.ex

CDimonaco added enhancement New feature or request elixir Pull requests that update Elixir code labels Sep 11, 2023

CDimonaco self-assigned this Sep 11, 2023

arbulu89 reviewed Sep 11, 2023

View reviewed changes

CDimonaco force-pushed the handle_saptune_payload branch from 7d3e8bb to ee4e4bd Compare September 12, 2023 09:55

arbulu89 reviewed Sep 12, 2023

View reviewed changes

CDimonaco force-pushed the handle_saptune_payload branch from f0d6884 to 9525e82 Compare September 13, 2023 08:52

arbulu89 reviewed Sep 13, 2023

View reviewed changes

arbulu89 approved these changes Sep 13, 2023

View reviewed changes

arbulu89 reviewed Sep 13, 2023

View reviewed changes

lib/trento/domain/value_objects/saptune_note.ex Outdated Show resolved Hide resolved

lib/trento/domain/value_objects/saptune_service_status.ex Outdated Show resolved Hide resolved

lib/trento/domain/value_objects/saptune_solution.ex Outdated Show resolved Hide resolved

EMaksy force-pushed the handle_saptune_payload branch 2 times, most recently from 9c2af10 to 14ece18 Compare September 13, 2023 12:39

EMaksy and others added 18 commits September 13, 2023 14:41

Add wip

1320f55

Add wip

d6adf96

Wip

8406a34

wip setup saptune discovery testing for host policy

5d44b3b

Wip on embedded schemas in SaptuneDiscoveryPayload

bf9ce66

Move saptune payload transformation to the host policy

7b34ff8

Fix casing of saptune result json object into host policy

44fb246

Add Staging field to SaptuneDiscoveryPayload

2e14338

Add saptune notes handling in host policy

e68badf

Refactored saptune status policy and command

c8e8bd5

Removed test data file

05ef580

Fix testing policy

51b029a

policy notes fix

3daf4a1

Add saptune discovery handler tests

426c56b

Change Saptune payload to reflect 1:1 the json payload from agent

7eb6327

Addressing review feedbacks

1651622

Refactor SaptuneServiceStatus value object

ba9e337

Removed old format saptune services list clause in host policy

065fc85

EMaksy added 2 commits September 13, 2023 14:41

Clean up lint messages

42a3526

Remove SaptunesServiceStatus required fields

4becd9d

EMaksy force-pushed the handle_saptune_payload branch 2 times, most recently from a1727c2 to 74724dc Compare September 13, 2023 12:59

Adress final comments

74724dc

CDimonaco marked this pull request as ready for review September 13, 2023 13:10

CDimonaco merged commit 7ea50d5 into main Sep 13, 2023

CDimonaco deleted the handle_saptune_payload branch September 13, 2023 13:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle saptune payload #1801

Handle saptune payload #1801

CDimonaco commented Sep 11, 2023

arbulu89 left a comment

arbulu89 Sep 11, 2023

arbulu89 left a comment

arbulu89 Sep 12, 2023

CDimonaco Sep 13, 2023

arbulu89 Sep 13, 2023

arbulu89 left a comment

arbulu89 Sep 13, 2023

arbulu89 Sep 13, 2023

arbulu89 left a comment •

edited

Loading

arbulu89 left a comment

Handle saptune payload #1801

Handle saptune payload #1801

Conversation

CDimonaco commented Sep 11, 2023

Description

How was this tested?

arbulu89 left a comment

Choose a reason for hiding this comment

arbulu89 Sep 11, 2023

Choose a reason for hiding this comment

arbulu89 left a comment

Choose a reason for hiding this comment

arbulu89 Sep 12, 2023

Choose a reason for hiding this comment

CDimonaco Sep 13, 2023

Choose a reason for hiding this comment

arbulu89 Sep 13, 2023

Choose a reason for hiding this comment

arbulu89 left a comment

Choose a reason for hiding this comment

arbulu89 Sep 13, 2023

Choose a reason for hiding this comment

arbulu89 Sep 13, 2023

Choose a reason for hiding this comment

arbulu89 left a comment • edited Loading

Choose a reason for hiding this comment

arbulu89 left a comment

Choose a reason for hiding this comment

arbulu89 left a comment •

edited

Loading