Add `managed` field in Cluster resources #2366

rtorrero · 2024-02-26T11:51:34Z

Description

This PR adds:

The managed field for all resources in the clusters API
The managed column in the cluster resources (node details) view
An 'unmanaged' counter as a tooltip on the site details table
Small refactor of existing code to adapt the API for v1 (adapt_v1 function)

Frontend Preview:

Checkout storybook "Unmanaged Node Resources"

Add count of unmanaged resources when a node is online but has unmanaged resources

Add Managed column in Node details view

How was this tested?

Updated existing frontend tests
Added cypress test to check tooltip message
Updated view tests for API backwards compatibility
Updated existing policy tests to include managed field

arbulu89

Some initial comments.
PD: Add UT tests for the frontend and e2e if possible

arbulu89 · 2024-02-27T07:50:01Z

lib/trento/discovery/policies/cluster_policy.ex

@@ -715,6 +716,9 @@ defmodule Trento.Discovery.Policies.ClusterPolicy do
    end)
  end

+  defp parse_managed(%{managed: true}), do: true


Wouldn't be better to include managed as required field in the payload and here simply passing that value, as we would always heve it?

arbulu89 · 2024-02-27T07:51:02Z

lib/trento_web/openapi/v1/schema/cluster.ex

@@ -20,7 +20,8 @@ defmodule TrentoWeb.OpenApi.V1.Schema.Cluster do
        type: %Schema{type: :string},
        role: %Schema{type: :string},
        status: %Schema{type: :string},
-        fail_count: %Schema{type: :integer}
+        fail_count: %Schema{type: :integer},
+        managed: %Schema{type: :boolean}


We don't want to include this field in the v1.
We want to include it in the v2.
In fact, we need to remove it in the v1 in the view code

arbulu89 · 2024-02-27T07:51:52Z

test/support/factory.ex

@@ -450,7 +450,8 @@ defmodule Trento.Factory do
          id: Faker.Pokemon.name(),
          role: "Stopped",
          status: Faker.Pokemon.name(),
-          type: "ocf::heartbeat:Dummy"
+          type: "ocf::heartbeat:Dummy",
+          managed: Enum.random([false, true])


This looks like a good moment to create the cluster_resource factory and use it.
What do you think?

You don't need this change. Here what you need to do is to use the factory itself

assets/js/pages/ClusterDetails/AttributesDetails.jsx

arbulu89

Hey @EMaksy
Some comments. We need to rethink some new things you did

assets/js/pages/ClusterDetails/AttributesDetails.jsx

assets/js/pages/ClusterDetails/ClusterNodeName.jsx

assets/js/pages/ClusterDetails/HanaClusterDetails.test.jsx

assets/js/pages/ClusterDetails/AscsErsClusterDetails.test.jsx

assets/js/pages/ClusterDetails/AttributesDetails.jsx

test/e2e/cypress/e2e/hana_cluster_details.cy.js

arbulu89

@rtorrero @EMaksy
The backend part is ready for me. I did some comments, and I think it could be improved, but well, at least it is correct.
@EMaksy The frontend part could get some additional work

arbulu89 · 2024-03-04T07:46:03Z

lib/trento_web/views/v1/cluster_view.ex

+    |> Map.put(:stopped_resources, adapt_resources(stopped_resources))
+  end
+
+  defp adapt_details(%{nodes: nodes} = details) do


@rtorrero Is this new entry needed?
I mean, the stopped_resources would be always there, so adapt_resources would return an empty list in any case.
Or do I miss something?
If you remove this, you could remove the adapt_nodes function as it is used only in one place, and we don't want to hide a simple Enum.map into a function

arbulu89 · 2024-03-04T07:48:21Z

lib/trento_web/views/v1/cluster_view.ex

+  end
+
+  defp adapt_node(node) do
+    adapted_resources = adapt_resources(node[:resources] || [])


@rtorrero Mostly the same here. Is it not the resources field always coming?
You could get it in the function signature, and not use this ugly thing.
If it is compulsory, use Map.get(node, :resources, []) at least

arbulu89 · 2024-03-04T07:49:21Z

test/support/factory.ex

@@ -450,7 +450,8 @@ defmodule Trento.Factory do
          id: Faker.Pokemon.name(),
          role: "Stopped",
          status: Faker.Pokemon.name(),
-          type: "ocf::heartbeat:Dummy"
+          type: "ocf::heartbeat:Dummy",
+          managed: Enum.random([false, true])


You don't need this change. Here what you need to do is to use the factory itself

arbulu89 · 2024-03-04T07:50:04Z

test/trento_web/views/v1/cluster_view_test.exs

@@ -40,6 +41,14 @@ defmodule TrentoWeb.V1.ClusterViewTest do
      refute Access.get(node, :nameserver_actual_role)
      refute Access.get(node, :indexserver_actual_role)
      refute Access.get(node, :status)
+
+      Enum.each(details.stopped_resources, fn stopped_resource ->
+        refute Map.has_key?(stopped_resource, :managed)


Why not Access.get here? Just for consistency

couldn't Access.get return a falsy boolean? (causing the test to pass when it shouldn't)

A Map.has_key? seems also semantically more correct to me.

It was more about consistency, not having Access above and Map.has_key? here.
I would be fine having has_key? in both.
Anyway, it was a detail

assets/js/pages/ClusterDetails/AttributesDetails.jsx

arbulu89 · 2024-03-04T07:54:31Z

assets/js/pages/ClusterDetails/ClusterNodeName.test.jsx

+    },
+    {
+      status: 'Online',
+      resources: [{ managed: true }, { managed: false }],


I think we should use the clusterResourceFactory here

arbulu89 · 2024-03-04T07:57:38Z

assets/js/pages/ClusterDetails/HanaClusterDetails.stories.jsx

@@ -177,3 +192,10 @@ export const WithRunningExecution = {
    lastExecution: { data: checksExecutionRunningFactory.build() },
  },
 };
+
+export const WithUnmanagedResources = {


Not really nice with the Host currently not registered, but hey...

assets/js/pages/ClusterDetails/AscsErsClusterDetails.test.jsx

assets/js/pages/ClusterDetails/HanaClusterDetails.test.jsx

test/e2e/cypress/e2e/hana_cluster_details.cy.js

EMaksy · 2024-03-04T21:47:00Z

assets/js/pages/ClusterDetails/AttributeDetails.test.jsx

+    expectedResource.forEach(({ key, title }) => {
+      expect(screen.getByText(title)).toBeInTheDocument();
+      resources.forEach((resource) => {
+        let value;


I put a let instead of const, as otherwise eslint complains about nested ternary operator. Maybe there is a better way ?

EMaksy · 2024-03-04T21:51:54Z

assets/js/pages/ClusterDetails/AttributeDetails.test.jsx

+    { attributes: hanaClusterAttributes, resources: hanaClusterResources },
+  ] = hanaClusterDetailsNodesFactory.buildList(1);
+
+  const scenarios = [


Any more ideas for additional scenarios?

arbulu89

Thank you @EMaksy @rtorrero
Ready to merge from my side 🚀

arbulu89 reviewed Feb 27, 2024

View reviewed changes

EMaksy force-pushed the resource-managed branch 2 times, most recently from 45a0207 to 25438ca Compare February 27, 2024 09:54

EMaksy self-assigned this Feb 27, 2024

EMaksy added enhancement New feature or request javascript Pull requests that update Javascript code elixir Pull requests that update Elixir code labels Feb 27, 2024

EMaksy force-pushed the resource-managed branch from 25438ca to 824c447 Compare February 27, 2024 14:33

arbulu89 reviewed Feb 27, 2024

View reviewed changes

assets/js/pages/ClusterDetails/AttributesDetails.jsx Outdated Show resolved Hide resolved

assets/js/pages/ClusterDetails/ClusterNodeName.jsx Outdated Show resolved Hide resolved

assets/js/pages/ClusterDetails/ClusterNodeName.jsx Outdated Show resolved Hide resolved

EMaksy reviewed Feb 27, 2024

View reviewed changes

assets/js/pages/ClusterDetails/ClusterNodeName.jsx Outdated Show resolved Hide resolved

EMaksy force-pushed the resource-managed branch from fe5f2d3 to 1700dff Compare February 29, 2024 15:52

EMaksy reviewed Feb 29, 2024

View reviewed changes

assets/js/pages/ClusterDetails/HanaClusterDetails.test.jsx Outdated Show resolved Hide resolved

EMaksy force-pushed the resource-managed branch from 2cc6ad7 to 8cc7b33 Compare March 1, 2024 10:22

EMaksy reviewed Mar 1, 2024

View reviewed changes

assets/js/pages/ClusterDetails/AscsErsClusterDetails.test.jsx Outdated Show resolved Hide resolved

EMaksy assigned rtorrero Mar 1, 2024

EMaksy reviewed Mar 1, 2024

View reviewed changes

assets/js/pages/ClusterDetails/AttributesDetails.jsx Outdated Show resolved Hide resolved

EMaksy force-pushed the resource-managed branch from 4ee353d to eba43d2 Compare March 1, 2024 15:51

EMaksy reviewed Mar 1, 2024

View reviewed changes

test/e2e/cypress/e2e/hana_cluster_details.cy.js Outdated Show resolved Hide resolved

rtorrero marked this pull request as ready for review March 1, 2024 16:39

arbulu89 requested changes Mar 4, 2024

View reviewed changes

EMaksy force-pushed the resource-managed branch 3 times, most recently from e782f8e to aa047e9 Compare March 4, 2024 21:43

EMaksy reviewed Mar 4, 2024

View reviewed changes

arbulu89 approved these changes Mar 5, 2024

View reviewed changes

EMaksy force-pushed the resource-managed branch from 36f4cbc to e8626fa Compare March 5, 2024 09:02

rtorrero and others added 3 commits March 5, 2024 10:03

Add managed field for cluster resource

8b767ac

prepare resources for table rendering

79f23d6

Refactor and fix es lint complains

64e90d2

EMaksy and others added 28 commits March 5, 2024 10:03

Refactor resourcesTableConfig to be more explicit and add managed column

581c75b

Adjust tests to include managed resource field

53c8e64

Adjust factory to include the managed field in resources

9f6f0cc

Add tooltip with count of unmanaged resources

7ef51a5

Add unmanaged resource count in tooltip

9df9d4e

Change resource table config

5313616

Refactor ClusterNodeName component

02972f3

use lodash capitalize instead of tenery operator

8a4459b

Use switch case for node status icons

e8733aa

Use cluster resource factory instead of literal

74db2db

Refactor ClusterNodeName

a08292a

Update test for ClusterNodeName

8bc19ff

Adapt resources managed

eda7e2b

Make managed a required field

fada62e

Add formatting

0ddec8e

Address mix credo

7b01a23

Refactor existing test for displaying node details

e33c22e

Add unmanaged resource node story

a0e2c5a

Add managed key value to fixtures

c18196c

Add e2e test to check tooltip icon message

efff0bb

Use cluster_resource factory for cluster_details

bced77c

Apply suggestions for parsing resources and nodes

399b82c

Address further code readability suggestions

a97eb3c

Remove e2e test for tooltip message

5abe3c4

Use factory for resources

293fe3f

Add new AttributeDetails test

2071af7

Refactor AttributeDetails.test.jsx

313791b

Refactor the AttributesDetails test

e8626fa

EMaksy merged commit 8d0ff11 into main Mar 5, 2024
24 checks passed

EMaksy deleted the resource-managed branch March 5, 2024 09:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `managed` field in Cluster resources #2366

Add `managed` field in Cluster resources #2366

rtorrero commented Feb 26, 2024 •

edited

Loading

arbulu89 left a comment

arbulu89 Feb 27, 2024

arbulu89 Feb 27, 2024

arbulu89 Feb 27, 2024

arbulu89 Mar 4, 2024

arbulu89 left a comment

arbulu89 left a comment

arbulu89 Mar 4, 2024

arbulu89 Mar 4, 2024

arbulu89 Mar 4, 2024

arbulu89 Mar 4, 2024

rtorrero Mar 4, 2024

arbulu89 Mar 4, 2024

arbulu89 Mar 4, 2024

arbulu89 Mar 4, 2024

EMaksy Mar 4, 2024

EMaksy Mar 4, 2024

arbulu89 left a comment

Add managed field in Cluster resources #2366

Add managed field in Cluster resources #2366

Conversation

rtorrero commented Feb 26, 2024 • edited Loading

Description

Frontend Preview:

Add count of unmanaged resources when a node is online but has unmanaged resources

Add Managed column in Node details view

How was this tested?

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

arbulu89 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

arbulu89 left a comment

Choose a reason for hiding this comment

Add `managed` field in Cluster resources #2366

Add `managed` field in Cluster resources #2366

rtorrero commented Feb 26, 2024 •

edited

Loading