From 31629d6d2247e40728366f3cae5d92b8afc4f23e Mon Sep 17 00:00:00 2001 From: pa-lem Date: Wed, 28 Aug 2024 11:57:14 +0200 Subject: [PATCH 01/29] add cardnality in query --- .../queries/proposed-changes/getProposedChangesDiffTree.ts | 1 + 1 file changed, 1 insertion(+) diff --git a/frontend/app/src/graphql/queries/proposed-changes/getProposedChangesDiffTree.ts b/frontend/app/src/graphql/queries/proposed-changes/getProposedChangesDiffTree.ts index 6acd44d6c4..b9bcd1a6d1 100644 --- a/frontend/app/src/graphql/queries/proposed-changes/getProposedChangesDiffTree.ts +++ b/frontend/app/src/graphql/queries/proposed-changes/getProposedChangesDiffTree.ts @@ -9,6 +9,7 @@ export const getProposedChangesDiffTree = gql` label status contains_conflict + cardinality elements { conflict { base_branch_action From 6efb82e14082bfa3e3aa4e394bda0ae301a135ca Mon Sep 17 00:00:00 2001 From: pa-lem Date: Wed, 28 Aug 2024 15:50:54 +0200 Subject: [PATCH 02/29] use node attribute for relationship one --- .../screens/diff/node-diff/node-attribute.tsx | 22 +++++--- .../app/src/screens/diff/node-diff/node.tsx | 55 ++++++++++++++++--- .../app/src/screens/diff/node-diff/types.ts | 1 + 3 files changed, 63 insertions(+), 15 deletions(-) diff --git a/frontend/app/src/screens/diff/node-diff/node-attribute.tsx b/frontend/app/src/screens/diff/node-diff/node-attribute.tsx index a2d83414be..0a3656fa4a 100644 --- a/frontend/app/src/screens/diff/node-diff/node-attribute.tsx +++ b/frontend/app/src/screens/diff/node-diff/node-attribute.tsx @@ -1,21 +1,25 @@ import { useParams } from "react-router-dom"; -import { DiffRow, formatValue } from "@/screens/diff/node-diff/utils"; +import { DiffRow } from "@/screens/diff/node-diff/utils"; import { DiffAttribute, DiffStatus } from "@/screens/diff/node-diff/types"; import { DiffThread } from "@/screens/diff/node-diff/thread"; import { DiffNodeProperty } from "@/screens/diff/node-diff/node-property"; +import { Conflict } from "./conflict"; type DiffNodeAttributeProps = { attribute: DiffAttribute; status: DiffStatus; + previousValue?: string; + newValue?: string; }; -export const DiffNodeAttribute = ({ attribute, status }: DiffNodeAttributeProps) => { +export const DiffNodeAttribute = ({ + attribute, + previousValue, + newValue, + status, +}: DiffNodeAttributeProps) => { const { "*": branchName } = useParams(); - const valueProperty = attribute.properties.find( - ({ property_type }) => property_type === "HAS_VALUE" - ); - return ( } - left={valueProperty?.previous_value && formatValue(valueProperty?.previous_value)} - right={status !== "REMOVED" && formatValue(valueProperty?.new_value)}> + left={previousValue} + right={newValue}>
{attribute.properties.map((property, index: number) => ( ))} + + {attribute.conflict && }
); diff --git a/frontend/app/src/screens/diff/node-diff/node.tsx b/frontend/app/src/screens/diff/node-diff/node.tsx index f6720cd403..c906fa340e 100644 --- a/frontend/app/src/screens/diff/node-diff/node.tsx +++ b/frontend/app/src/screens/diff/node-diff/node.tsx @@ -6,7 +6,7 @@ import { DiffNodeAttribute } from "./node-attribute"; import { DiffThread } from "./thread"; import { Icon } from "@iconify-icon/react"; import { useLocation, useParams } from "react-router-dom"; -import { DiffBadge } from "@/screens/diff/node-diff/utils"; +import { DiffBadge, formatValue } from "@/screens/diff/node-diff/utils"; import { useAtomValue } from "jotai"; import { schemaKindNameState } from "@/state/atoms/schemaKindName.atom"; import type { DiffNode as DiffNodeType } from "@/screens/diff/node-diff/types"; @@ -64,12 +64,53 @@ export const DiffNode = ({ sourceBranch, destinationBranch, node }: DiffNodeProp } className="bg-gray-100 border rounded-md">
- {node.attributes.map((attribute: any, index: number) => ( - - ))} - {node.relationships.map((relationship: any, index: number) => ( - - ))} + {node.attributes.map((attribute: any, index: number) => { + const valueProperty = attribute.properties.find( + ({ property_type }) => property_type === "HAS_VALUE" + ); + + return ( + + ); + })} + + {node.relationships.map((relationship: any, index: number) => { + if (relationship.cardinality === "ONE") { + const element = relationship.elements[0]; + + const attribute = { + ...relationship, + path_identifier: relationship.path_identifier, + properties: element.properties, + }; + + return ( + + ); + } + + return ( + + ); + })}
)} diff --git a/frontend/app/src/screens/diff/node-diff/types.ts b/frontend/app/src/screens/diff/node-diff/types.ts index 77b7474b27..6d2316efde 100644 --- a/frontend/app/src/screens/diff/node-diff/types.ts +++ b/frontend/app/src/screens/diff/node-diff/types.ts @@ -32,6 +32,7 @@ export type DiffAttribute = { properties: Array; path_identifier: string | null; contains_conflict: boolean; + conflict: DiffConflict | null; }; export type DiffRelationshipElement = { From debecbed9225b0c9e72af5c2d4dc01da988d937c Mon Sep 17 00:00:00 2001 From: pa-lem Date: Thu, 29 Aug 2024 11:17:45 +0200 Subject: [PATCH 03/29] udpate relationshiip structure for attribute --- frontend/app/src/screens/diff/node-diff/node.tsx | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/frontend/app/src/screens/diff/node-diff/node.tsx b/frontend/app/src/screens/diff/node-diff/node.tsx index 8ed2bd8229..bca26e998b 100644 --- a/frontend/app/src/screens/diff/node-diff/node.tsx +++ b/frontend/app/src/screens/diff/node-diff/node.tsx @@ -85,8 +85,10 @@ export const DiffNode = ({ sourceBranch, destinationBranch, node }: DiffNodeProp const element = relationship.elements[0]; const attribute = { - ...relationship, - path_identifier: relationship.path_identifier, + name: relationship.name, + contains_conflict: relationship.contains_conflict, + conflict: element.conflict, + path_identifier: element.path_identifier, properties: element.properties, }; @@ -95,7 +97,7 @@ export const DiffNode = ({ sourceBranch, destinationBranch, node }: DiffNodeProp key={index} attribute={attribute} status={node.status} - previousValue={"-"} + previousValue={element.previous_peer_label} newValue={element.peer_label} /> ); From 08ce586f3fab4823797ea056a09be492d9eb6bec Mon Sep 17 00:00:00 2001 From: pa-lem Date: Tue, 3 Sep 2024 14:17:16 +0200 Subject: [PATCH 04/29] add more details in response --- .../proposed-changes/getProposedChangesDiffTree.ts | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/frontend/app/src/graphql/queries/proposed-changes/getProposedChangesDiffTree.ts b/frontend/app/src/graphql/queries/proposed-changes/getProposedChangesDiffTree.ts index 5b4e8c2725..d6fcecae96 100644 --- a/frontend/app/src/graphql/queries/proposed-changes/getProposedChangesDiffTree.ts +++ b/frontend/app/src/graphql/queries/proposed-changes/getProposedChangesDiffTree.ts @@ -12,9 +12,11 @@ export const getProposedChangesDiffTree = gql` cardinality elements { conflict { + base_branch_label base_branch_action base_branch_changed_at base_branch_value + diff_branch_label diff_branch_action diff_branch_changed_at diff_branch_value @@ -26,9 +28,11 @@ export const getProposedChangesDiffTree = gql` peer_id properties { conflict { + base_branch_label base_branch_action base_branch_changed_at base_branch_value + diff_branch_label diff_branch_action diff_branch_changed_at diff_branch_value @@ -51,9 +55,11 @@ export const getProposedChangesDiffTree = gql` path_identifier } conflict { + base_branch_label base_branch_action base_branch_changed_at diff_branch_action + diff_branch_label base_branch_value diff_branch_changed_at diff_branch_value @@ -66,9 +72,11 @@ export const getProposedChangesDiffTree = gql` name properties { conflict { + base_branch_label base_branch_action base_branch_changed_at base_branch_value + diff_branch_label diff_branch_action diff_branch_changed_at diff_branch_value From 8de8644a51400c3ca42419977b51e5a9d94357a0 Mon Sep 17 00:00:00 2001 From: pa-lem Date: Tue, 3 Sep 2024 14:19:45 +0200 Subject: [PATCH 05/29] make relationships match attribute node diff --- .../screens/diff/node-diff/node-attribute.tsx | 3 --- .../screens/diff/node-diff/node-property.tsx | 8 ++++++-- .../app/src/screens/diff/node-diff/node.tsx | 19 +++++++++++++------ .../app/src/screens/diff/node-diff/utils.tsx | 2 +- 4 files changed, 20 insertions(+), 12 deletions(-) diff --git a/frontend/app/src/screens/diff/node-diff/node-attribute.tsx b/frontend/app/src/screens/diff/node-diff/node-attribute.tsx index 0a3656fa4a..f6ddae8e2c 100644 --- a/frontend/app/src/screens/diff/node-diff/node-attribute.tsx +++ b/frontend/app/src/screens/diff/node-diff/node-attribute.tsx @@ -3,7 +3,6 @@ import { DiffRow } from "@/screens/diff/node-diff/utils"; import { DiffAttribute, DiffStatus } from "@/screens/diff/node-diff/types"; import { DiffThread } from "@/screens/diff/node-diff/thread"; import { DiffNodeProperty } from "@/screens/diff/node-diff/node-property"; -import { Conflict } from "./conflict"; type DiffNodeAttributeProps = { attribute: DiffAttribute; @@ -39,8 +38,6 @@ export const DiffNodeAttribute = ({ {attribute.properties.map((property, index: number) => ( ))} - - {attribute.conflict && } ); diff --git a/frontend/app/src/screens/diff/node-diff/node-property.tsx b/frontend/app/src/screens/diff/node-diff/node-property.tsx index bbe1799ee8..7a89275839 100644 --- a/frontend/app/src/screens/diff/node-diff/node-property.tsx +++ b/frontend/app/src/screens/diff/node-diff/node-property.tsx @@ -22,7 +22,9 @@ const getPreviousValue = (property: DiffProperty) => { return previousValue; } - const conflictValue = formatValue(property.conflict.base_branch_value); + const conflictValue = formatValue( + property.conflict.base_branch_label || property.conflict.base_branch_value + ); return (
{previousValue} @@ -42,7 +44,9 @@ const getNewValue = (property: DiffProperty) => { return newValue; } - const conflictValue = formatValue(property.conflict.diff_branch_value); + const conflictValue = formatValue( + property.conflict.diff_branch_label || property.conflict.diff_branch_value + ); return ( {conflictValue} diff --git a/frontend/app/src/screens/diff/node-diff/node.tsx b/frontend/app/src/screens/diff/node-diff/node.tsx index bca26e998b..04bce7efac 100644 --- a/frontend/app/src/screens/diff/node-diff/node.tsx +++ b/frontend/app/src/screens/diff/node-diff/node.tsx @@ -51,8 +51,8 @@ export const DiffNode = ({ sourceBranch, destinationBranch, node }: DiffNodeProp
} className="bg-gray-100 border rounded-md"> -
-
+
+
{sourceBranch} @@ -87,9 +87,16 @@ export const DiffNode = ({ sourceBranch, destinationBranch, node }: DiffNodeProp const attribute = { name: relationship.name, contains_conflict: relationship.contains_conflict, - conflict: element.conflict, - path_identifier: element.path_identifier, - properties: element.properties, + properties: [ + { + conflict: element.conflict, + new_value: element.peer_label, + path_identifier: element.path_identifier, + previous_value: element.conflict?.base_branch_label, + property_type: "HAS_VALUE", + }, + ...element.properties, + ], }; return ( @@ -97,7 +104,7 @@ export const DiffNode = ({ sourceBranch, destinationBranch, node }: DiffNodeProp key={index} attribute={attribute} status={node.status} - previousValue={element.previous_peer_label} + previousValue={element.conflict?.base_branch_label} newValue={element.peer_label} /> ); diff --git a/frontend/app/src/screens/diff/node-diff/utils.tsx b/frontend/app/src/screens/diff/node-diff/utils.tsx index 681396132f..a791c9e483 100644 --- a/frontend/app/src/screens/diff/node-diff/utils.tsx +++ b/frontend/app/src/screens/diff/node-diff/utils.tsx @@ -61,7 +61,7 @@ export const DiffRow = ({ status, }: DiffRowProps) => { return ( -
+
{hasConflicts &&
} Date: Tue, 3 Sep 2024 14:40:45 +0200 Subject: [PATCH 06/29] fix test with new UI --- ...al-1_object-create-update-diff-and-merge.spec.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/frontend/app/tests/e2e/tutorial/tutorial-1_object-create-update-diff-and-merge.spec.ts b/frontend/app/tests/e2e/tutorial/tutorial-1_object-create-update-diff-and-merge.spec.ts index bf5be6b385..3ff6bba79b 100644 --- a/frontend/app/tests/e2e/tutorial/tutorial-1_object-create-update-diff-and-merge.spec.ts +++ b/frontend/app/tests/e2e/tutorial/tutorial-1_object-create-update-diff-and-merge.spec.ts @@ -101,16 +101,23 @@ test.describe("Getting started with Infrahub - Object and branch creation, updat await expect(page.locator("dl")).toContainText("cr1234"); }); + await test.step("trigger the diff update", async () => { + await page.getByText("Data").click(); + await expect(page.getByRole("button", { name: "Refresh diff" })).toBeVisible(); + await expect(page.getByText("No diff to display. Try to")).toBeVisible(); + await page.getByRole("button", { name: "Refresh diff" }).first().click(); + await expect(page.getByText("Diff updated!")).toBeVisible(); + }); + await test.step("View branch diff", async () => { - await page.getByText("Diff").click(); - await page.getByText("my-first-Tenant").click(); + await page.getByText("UpdatedTenantmy-first-Tenant").click(); await expect(page.getByText("Testing Infrahub")).toBeVisible(); await saveScreenshotForDocs(page, "tutorial_1_branch_diff"); await expect(page.getByText("Changes from branch cr1234")).toBeVisible(); }); await test.step("Merge branch cr1234 into main", async () => { - await page.getByRole("button", { name: "Details" }).click(); + await page.getByText("Details", { exact: true }).click(); const mergeButton = page.getByRole("button", { name: "Merge" }); await expect(mergeButton).toBeVisible(); await saveScreenshotForDocs(page, "tutorial_1_branch_details"); From 61ff4d8273e8fae2aa5410225007b8b99328c707 Mon Sep 17 00:00:00 2001 From: pa-lem Date: Tue, 3 Sep 2024 16:49:27 +0200 Subject: [PATCH 07/29] default open for items with conflicts --- frontend/app/src/screens/diff/node-diff/utils.tsx | 1 + 1 file changed, 1 insertion(+) diff --git a/frontend/app/src/screens/diff/node-diff/utils.tsx b/frontend/app/src/screens/diff/node-diff/utils.tsx index a791c9e483..c604ead555 100644 --- a/frontend/app/src/screens/diff/node-diff/utils.tsx +++ b/frontend/app/src/screens/diff/node-diff/utils.tsx @@ -65,6 +65,7 @@ export const DiffRow = ({ {hasConflicts &&
} Date: Tue, 3 Sep 2024 14:40:45 +0200 Subject: [PATCH 08/29] fix test with new UI --- ...al-1_object-create-update-diff-and-merge.spec.ts | 13 ++++++++++--- 1 file changed, 10 insertions(+), 3 deletions(-) diff --git a/frontend/app/tests/e2e/tutorial/tutorial-1_object-create-update-diff-and-merge.spec.ts b/frontend/app/tests/e2e/tutorial/tutorial-1_object-create-update-diff-and-merge.spec.ts index bf5be6b385..3ff6bba79b 100644 --- a/frontend/app/tests/e2e/tutorial/tutorial-1_object-create-update-diff-and-merge.spec.ts +++ b/frontend/app/tests/e2e/tutorial/tutorial-1_object-create-update-diff-and-merge.spec.ts @@ -101,16 +101,23 @@ test.describe("Getting started with Infrahub - Object and branch creation, updat await expect(page.locator("dl")).toContainText("cr1234"); }); + await test.step("trigger the diff update", async () => { + await page.getByText("Data").click(); + await expect(page.getByRole("button", { name: "Refresh diff" })).toBeVisible(); + await expect(page.getByText("No diff to display. Try to")).toBeVisible(); + await page.getByRole("button", { name: "Refresh diff" }).first().click(); + await expect(page.getByText("Diff updated!")).toBeVisible(); + }); + await test.step("View branch diff", async () => { - await page.getByText("Diff").click(); - await page.getByText("my-first-Tenant").click(); + await page.getByText("UpdatedTenantmy-first-Tenant").click(); await expect(page.getByText("Testing Infrahub")).toBeVisible(); await saveScreenshotForDocs(page, "tutorial_1_branch_diff"); await expect(page.getByText("Changes from branch cr1234")).toBeVisible(); }); await test.step("Merge branch cr1234 into main", async () => { - await page.getByRole("button", { name: "Details" }).click(); + await page.getByText("Details", { exact: true }).click(); const mergeButton = page.getByRole("button", { name: "Merge" }); await expect(mergeButton).toBeVisible(); await saveScreenshotForDocs(page, "tutorial_1_branch_details"); From 102e5a7a7d5834b149ecc4b78c36f98a88b37d53 Mon Sep 17 00:00:00 2001 From: bilalabbad Date: Tue, 3 Sep 2024 17:12:23 +0200 Subject: [PATCH 09/29] fix test after rebase --- .../tutorial-1_object-create-update-diff-and-merge.spec.ts | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/frontend/app/tests/e2e/tutorial/tutorial-1_object-create-update-diff-and-merge.spec.ts b/frontend/app/tests/e2e/tutorial/tutorial-1_object-create-update-diff-and-merge.spec.ts index 3ff6bba79b..225e93d836 100644 --- a/frontend/app/tests/e2e/tutorial/tutorial-1_object-create-update-diff-and-merge.spec.ts +++ b/frontend/app/tests/e2e/tutorial/tutorial-1_object-create-update-diff-and-merge.spec.ts @@ -103,9 +103,8 @@ test.describe("Getting started with Infrahub - Object and branch creation, updat await test.step("trigger the diff update", async () => { await page.getByText("Data").click(); - await expect(page.getByRole("button", { name: "Refresh diff" })).toBeVisible(); - await expect(page.getByText("No diff to display. Try to")).toBeVisible(); - await page.getByRole("button", { name: "Refresh diff" }).first().click(); + await expect(page.getByText("We are computing the diff")).toBeVisible(); + await page.getByRole("button", { name: "Refresh" }).click(); await expect(page.getByText("Diff updated!")).toBeVisible(); }); From 184ec41df1108a4b71f6f2a0d2cffc8385d85786 Mon Sep 17 00:00:00 2001 From: Aaron McCarty Date: Tue, 3 Sep 2024 16:17:48 -0700 Subject: [PATCH 10/29] add diff conflict at the attribute level for value conflicts --- backend/infrahub/graphql/queries/diff/tree.py | 7 ++++++ .../unit/graphql/test_diff_tree_query.py | 24 +++++++++++++++++++ 2 files changed, 31 insertions(+) diff --git a/backend/infrahub/graphql/queries/diff/tree.py b/backend/infrahub/graphql/queries/diff/tree.py index d7de246295..24585afbb6 100644 --- a/backend/infrahub/graphql/queries/diff/tree.py +++ b/backend/infrahub/graphql/queries/diff/tree.py @@ -8,6 +8,7 @@ from infrahub.core import registry from infrahub.core.constants import DiffAction, RelationshipCardinality +from infrahub.core.constants.database import DatabaseEdgeType from infrahub.core.diff.model.path import NameTrackingId from infrahub.core.diff.repository.repository import DiffRepository from infrahub.core.query.diff import DiffCountChanges @@ -75,6 +76,7 @@ class DiffAttribute(DiffSummaryCounts): path_identifier = String(required=True) properties = List(DiffProperty) contains_conflict = Boolean(required=True) + conflict = Field(ConflictDetails, required=False) class DiffSingleRelationship(DiffSummaryCounts): @@ -207,6 +209,10 @@ def to_diff_attribute( diff_properties = [ self.to_diff_property(enriched_property=e_prop, context=context) for e_prop in enriched_attribute.properties ] + conflict = None + for diff_prop in diff_properties: + if diff_prop.property_type == DatabaseEdgeType.HAS_VALUE.value and diff_prop.conflict: + conflict = diff_prop.conflict return DiffAttribute( name=enriched_attribute.name, last_changed_at=enriched_attribute.changed_at.obj, @@ -214,6 +220,7 @@ def to_diff_attribute( path_identifier=enriched_attribute.path_identifier, properties=diff_properties, contains_conflict=enriched_attribute.contains_conflict, + conflict=conflict, num_added=enriched_attribute.num_added, num_updated=enriched_attribute.num_updated, num_removed=enriched_attribute.num_removed, diff --git a/backend/tests/unit/graphql/test_diff_tree_query.py b/backend/tests/unit/graphql/test_diff_tree_query.py index 31c65fc0df..39aec40f3f 100644 --- a/backend/tests/unit/graphql/test_diff_tree_query.py +++ b/backend/tests/unit/graphql/test_diff_tree_query.py @@ -69,6 +69,18 @@ num_updated num_conflicts contains_conflict + conflict { + uuid + base_branch_action + base_branch_value + base_branch_changed_at + base_branch_label + diff_branch_action + diff_branch_value + diff_branch_changed_at + diff_branch_label + selected_branch + } properties { property_type last_changed_at @@ -353,6 +365,18 @@ async def test_diff_tree_one_attr_change( "num_conflicts": 1, "status": UPDATED_ACTION, "contains_conflict": True, + "conflict": { + "uuid": enriched_conflict.uuid, + "base_branch_action": UPDATED_ACTION, + "base_branch_value": "#fedcba", + "base_branch_changed_at": enriched_conflict.base_branch_changed_at.to_string(with_z=False), + "base_branch_label": None, + "diff_branch_action": UPDATED_ACTION, + "diff_branch_value": "#abcdef", + "diff_branch_changed_at": enriched_conflict.diff_branch_changed_at.to_string(with_z=False), + "diff_branch_label": None, + "selected_branch": GraphQLConfictSelection.DIFF_BRANCH.name, + }, "properties": [ { "property_type": "HAS_VALUE", From cbd60750f706e4b78718edd717a24f32e0e612f4 Mon Sep 17 00:00:00 2001 From: Aaron McCarty Date: Tue, 3 Sep 2024 16:27:35 -0700 Subject: [PATCH 11/29] remove HAS_VALUE conflict from the property --- backend/infrahub/graphql/queries/diff/tree.py | 1 + .../tests/unit/graphql/test_diff_tree_query.py | 17 +---------------- 2 files changed, 2 insertions(+), 16 deletions(-) diff --git a/backend/infrahub/graphql/queries/diff/tree.py b/backend/infrahub/graphql/queries/diff/tree.py index 24585afbb6..8e4512f29f 100644 --- a/backend/infrahub/graphql/queries/diff/tree.py +++ b/backend/infrahub/graphql/queries/diff/tree.py @@ -213,6 +213,7 @@ def to_diff_attribute( for diff_prop in diff_properties: if diff_prop.property_type == DatabaseEdgeType.HAS_VALUE.value and diff_prop.conflict: conflict = diff_prop.conflict + diff_prop.conflict = None return DiffAttribute( name=enriched_attribute.name, last_changed_at=enriched_attribute.changed_at.obj, diff --git a/backend/tests/unit/graphql/test_diff_tree_query.py b/backend/tests/unit/graphql/test_diff_tree_query.py index 39aec40f3f..b8159b38f9 100644 --- a/backend/tests/unit/graphql/test_diff_tree_query.py +++ b/backend/tests/unit/graphql/test_diff_tree_query.py @@ -386,22 +386,7 @@ async def test_diff_tree_one_attr_change( "previous_label": None, "new_label": None, "status": UPDATED_ACTION, - "conflict": { - "uuid": enriched_conflict.uuid, - "base_branch_action": UPDATED_ACTION, - "base_branch_value": "#fedcba", - "base_branch_changed_at": enriched_conflict.base_branch_changed_at.to_string( - with_z=False - ), - "base_branch_label": None, - "diff_branch_action": UPDATED_ACTION, - "diff_branch_value": "#abcdef", - "diff_branch_changed_at": enriched_conflict.diff_branch_changed_at.to_string( - with_z=False - ), - "diff_branch_label": None, - "selected_branch": GraphQLConfictSelection.DIFF_BRANCH.name, - }, + "conflict": None, } ], } From 6106cce6a4fa046f9bc5ef041061817acac3ffa5 Mon Sep 17 00:00:00 2001 From: pa-lem Date: Wed, 4 Sep 2024 10:44:40 +0200 Subject: [PATCH 12/29] fix test --- .../tests/e2e/proposed-changes/proposed-changes_diff.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/app/tests/e2e/proposed-changes/proposed-changes_diff.spec.ts b/frontend/app/tests/e2e/proposed-changes/proposed-changes_diff.spec.ts index d2c875458a..b81bf2ec1a 100644 --- a/frontend/app/tests/e2e/proposed-changes/proposed-changes_diff.spec.ts +++ b/frontend/app/tests/e2e/proposed-changes/proposed-changes_diff.spec.ts @@ -39,7 +39,7 @@ test.describe("/proposed-changes diff data", () => { await expect(page.getByText("UpdatedInterface L3Ethernet1")).toBeVisible(); await page.getByText("UpdatedDeviceden1-edge1").click(); await expect(page.getByText("status")).toBeVisible(); - await expect(page.getByText("active")).toBeVisible(); + await expect(page.getByText("active", { exact: true })).toBeVisible(); await expect(page.getByText("maintenance", { exact: true })).toBeVisible(); await page .locator("div") From 4c52436c3d44d28408fec1a3a0683cafe4e81ae9 Mon Sep 17 00:00:00 2001 From: pa-lem Date: Wed, 4 Sep 2024 11:42:57 +0200 Subject: [PATCH 13/29] update test --- .../e2e/proposed-changes/proposed-changes_diff.spec.ts | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/frontend/app/tests/e2e/proposed-changes/proposed-changes_diff.spec.ts b/frontend/app/tests/e2e/proposed-changes/proposed-changes_diff.spec.ts index b81bf2ec1a..52c32b8c6e 100644 --- a/frontend/app/tests/e2e/proposed-changes/proposed-changes_diff.spec.ts +++ b/frontend/app/tests/e2e/proposed-changes/proposed-changes_diff.spec.ts @@ -40,14 +40,8 @@ test.describe("/proposed-changes diff data", () => { await page.getByText("UpdatedDeviceden1-edge1").click(); await expect(page.getByText("status")).toBeVisible(); await expect(page.getByText("active", { exact: true })).toBeVisible(); - await expect(page.getByText("maintenance", { exact: true })).toBeVisible(); - await page - .locator("div") - .filter({ hasText: /^status$/ }) - .first() - .click(); + await expect(page.getByText("maintenance", { exact: true }).first()).toBeVisible(); await expect(page.getByText("valueConflict")).toBeVisible(); - await page.getByText("valueConflict").click(); await expect( page .locator("div") From 26947206ac7d298b5e8c9c60851bae08b4b0bd7e Mon Sep 17 00:00:00 2001 From: pa-lem Date: Wed, 4 Sep 2024 13:54:17 +0200 Subject: [PATCH 14/29] avoid retry failure --- .../tests/e2e/proposed-changes/proposed-changes_diff.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/app/tests/e2e/proposed-changes/proposed-changes_diff.spec.ts b/frontend/app/tests/e2e/proposed-changes/proposed-changes_diff.spec.ts index 52c32b8c6e..c0a3ad4d3d 100644 --- a/frontend/app/tests/e2e/proposed-changes/proposed-changes_diff.spec.ts +++ b/frontend/app/tests/e2e/proposed-changes/proposed-changes_diff.spec.ts @@ -67,7 +67,7 @@ test.describe("/proposed-changes diff data", () => { test("should approve a proposed changes", async ({ page }) => { await test.step("got to the proposed changes data tab", async () => { await page.goto("/proposed-changes"); - await page.getByRole("link", { name: "conflict-test" }).click(); + await page.getByRole("link", { name: "conflict-test" }).first().click(); await expect(page.getByRole("cell", { name: "A", exact: true }).first()).toBeVisible(); await expect(page.getByRole("cell", { name: "A", exact: true }).nth(1)).toBeVisible(); await page.getByRole("button", { name: "Approve" }).click(); From 8bd9e8580af8c18ae47936a76863d0248fddd429 Mon Sep 17 00:00:00 2001 From: pa-lem Date: Wed, 4 Sep 2024 14:00:13 +0200 Subject: [PATCH 15/29] increase timeout for diff update query --- .../tests/e2e/proposed-changes/proposed-changes_diff.spec.ts | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/frontend/app/tests/e2e/proposed-changes/proposed-changes_diff.spec.ts b/frontend/app/tests/e2e/proposed-changes/proposed-changes_diff.spec.ts index c0a3ad4d3d..559fec5879 100644 --- a/frontend/app/tests/e2e/proposed-changes/proposed-changes_diff.spec.ts +++ b/frontend/app/tests/e2e/proposed-changes/proposed-changes_diff.spec.ts @@ -31,7 +31,7 @@ test.describe("/proposed-changes diff data", () => { await test.step("trigger the diff update", async () => { await expect(page.getByText("We are computing the diff")).toBeVisible(); await page.getByRole("button", { name: "Refresh" }).click(); - await expect(page.getByText("Diff updated!")).toBeVisible(); + await expect(page.getByText("Diff updated!")).toBeVisible({ timeout: 5 * 60 * 1000 }); }); await test.step("check diff data", async () => { From 584649bb1ec35d950257086eb595b9a30c2c1f6f Mon Sep 17 00:00:00 2001 From: Aaron McCarty Date: Wed, 4 Sep 2024 16:07:58 -0700 Subject: [PATCH 16/29] IFC-623 diff locking enhancements --- backend/infrahub/core/diff/coordinator.py | 172 +++++++++++++--------- backend/infrahub/lock.py | 11 ++ 2 files changed, 112 insertions(+), 71 deletions(-) diff --git a/backend/infrahub/core/diff/coordinator.py b/backend/infrahub/core/diff/coordinator.py index 01d92040c4..600a06ef05 100644 --- a/backend/infrahub/core/diff/coordinator.py +++ b/backend/infrahub/core/diff/coordinator.py @@ -35,6 +35,8 @@ def __hash__(self) -> int: class DiffCoordinator: + lock_namespace = "diff-update" + def __init__( self, diff_repo: DiffRepository, @@ -54,6 +56,7 @@ def __init__( self.labels_enricher = labels_enricher self.summary_counts_enricher = summary_counts_enricher self.data_check_synchronizer = data_check_synchronizer + self.lock_registry = lock.registry self._enriched_diff_cache: dict[EnrichedDiffRequest, EnrichedDiffRoot] = {} async def run_update( @@ -84,17 +87,41 @@ async def run_update( name=name, ) + def _get_lock_name(self, base_branch_name: str, diff_branch_name: str, is_incremental: bool) -> str: + lock_name = f"{base_branch_name}__{diff_branch_name}" + if is_incremental: + lock_name += "__incremental" + return lock_name + async def update_branch_diff(self, base_branch: Branch, diff_branch: Branch) -> EnrichedDiffRoot: + incremental_lock_name = self._get_lock_name( + base_branch_name=base_branch.name, diff_branch_name=diff_branch.name, is_incremental=True + ) + existing_incremental_lock = self.lock_registry.get_existing( + name=incremental_lock_name, namespace=self.lock_namespace + ) + if existing_incremental_lock and await existing_incremental_lock.locked(): + async with self.lock_registry.get(name=incremental_lock_name, namespace=self.lock_namespace): + return await self.diff_repo.get_one( + tracking_id=BranchTrackingId(name=diff_branch.name), diff_branch_name=diff_branch.name + ) + general_lock_name = self._get_lock_name( + base_branch_name=base_branch.name, diff_branch_name=diff_branch.name, is_incremental=False + ) from_time = Timestamp(diff_branch.get_created_at()) to_time = Timestamp() tracking_id = BranchTrackingId(name=diff_branch.name) - return await self._update_diffs( - base_branch=base_branch, - diff_branch=diff_branch, - from_time=from_time, - to_time=to_time, - tracking_id=tracking_id, - ) + async with ( + self.lock_registry.get(name=general_lock_name, namespace=self.lock_namespace), + self.lock_registry.get(name=incremental_lock_name, namespace=self.lock_namespace), + ): + return await self._update_diffs( + base_branch=base_branch, + diff_branch=diff_branch, + from_time=from_time, + to_time=to_time, + tracking_id=tracking_id, + ) async def create_or_update_arbitrary_timeframe_diff( self, @@ -107,13 +134,17 @@ async def create_or_update_arbitrary_timeframe_diff( tracking_id = None if name: tracking_id = NameTrackingId(name=name) - return await self._update_diffs( - base_branch=base_branch, - diff_branch=diff_branch, - from_time=from_time, - to_time=to_time, - tracking_id=tracking_id, + general_lock_name = self._get_lock_name( + base_branch_name=base_branch.name, diff_branch_name=diff_branch.name, is_incremental=False ) + async with self.lock_registry.get(name=general_lock_name, namespace=self.lock_namespace): + return await self._update_diffs( + base_branch=base_branch, + diff_branch=diff_branch, + from_time=from_time, + to_time=to_time, + tracking_id=tracking_id, + ) async def _update_diffs( self, @@ -123,67 +154,66 @@ async def _update_diffs( to_time: Timestamp, tracking_id: TrackingId | None = None, ) -> EnrichedDiffRoot: - async with lock.registry.get(name=f"{base_branch.name}__{diff_branch.name}", namespace="branch-diff"): - requested_diff_branches = {base_branch.name: base_branch, diff_branch.name: diff_branch} - aggregated_diffs_by_branch_name: dict[str, EnrichedDiffRoot] = {} - diff_uuids_to_delete = [] - for branch in requested_diff_branches.values(): - retrieved_enriched_diffs = await self.diff_repo.get( - base_branch_name=base_branch.name, - diff_branch_names=[branch.name], - from_time=from_time, - to_time=to_time, - tracking_id=tracking_id, - include_empty=True, - ) - if tracking_id: - diff_uuids_to_delete += [ - diff.uuid for diff in retrieved_enriched_diffs if diff.tracking_id == tracking_id - ] - covered_time_ranges = [ - TimeRange(from_time=enriched_diff.from_time, to_time=enriched_diff.to_time) - for enriched_diff in retrieved_enriched_diffs + requested_diff_branches = {base_branch.name: base_branch, diff_branch.name: diff_branch} + aggregated_diffs_by_branch_name: dict[str, EnrichedDiffRoot] = {} + diff_uuids_to_delete = [] + for branch in requested_diff_branches.values(): + retrieved_enriched_diffs = await self.diff_repo.get( + base_branch_name=base_branch.name, + diff_branch_names=[branch.name], + from_time=from_time, + to_time=to_time, + tracking_id=tracking_id, + include_empty=True, + ) + if tracking_id: + diff_uuids_to_delete += [ + diff.uuid for diff in retrieved_enriched_diffs if diff.tracking_id == tracking_id ] - missing_time_ranges = self._get_missing_time_ranges( - time_ranges=covered_time_ranges, from_time=from_time, to_time=to_time - ) - all_enriched_diffs = list(retrieved_enriched_diffs) - for missing_time_range in missing_time_ranges: - diff_request = EnrichedDiffRequest( - base_branch=base_branch, - diff_branch=branch, - from_time=missing_time_range.from_time, - to_time=missing_time_range.to_time, - ) - enriched_diff = await self._get_enriched_diff(diff_request=diff_request) - all_enriched_diffs.append(enriched_diff) - all_enriched_diffs.sort(key=lambda e_diff: e_diff.from_time) - combined_diff = all_enriched_diffs[0] - for next_diff in all_enriched_diffs[1:]: - combined_diff = await self.diff_combiner.combine(earlier_diff=combined_diff, later_diff=next_diff) - aggregated_diffs_by_branch_name[branch.name] = combined_diff - - if len(aggregated_diffs_by_branch_name) > 1: - await self.conflicts_enricher.add_conflicts_to_branch_diff( - base_diff_root=aggregated_diffs_by_branch_name[base_branch.name], - branch_diff_root=aggregated_diffs_by_branch_name[diff_branch.name], + covered_time_ranges = [ + TimeRange(from_time=enriched_diff.from_time, to_time=enriched_diff.to_time) + for enriched_diff in retrieved_enriched_diffs + ] + missing_time_ranges = self._get_missing_time_ranges( + time_ranges=covered_time_ranges, from_time=from_time, to_time=to_time + ) + all_enriched_diffs = list(retrieved_enriched_diffs) + for missing_time_range in missing_time_ranges: + diff_request = EnrichedDiffRequest( + base_branch=base_branch, + diff_branch=branch, + from_time=missing_time_range.from_time, + to_time=missing_time_range.to_time, ) - await self.labels_enricher.enrich( - enriched_diff_root=aggregated_diffs_by_branch_name[diff_branch.name], conflicts_only=True - ) - - if tracking_id: - for enriched_diff in aggregated_diffs_by_branch_name.values(): - enriched_diff.tracking_id = tracking_id - if diff_uuids_to_delete: - await self.diff_repo.delete_diff_roots(diff_root_uuids=diff_uuids_to_delete) - + enriched_diff = await self._get_enriched_diff(diff_request=diff_request) + all_enriched_diffs.append(enriched_diff) + all_enriched_diffs.sort(key=lambda e_diff: e_diff.from_time) + combined_diff = all_enriched_diffs[0] + for next_diff in all_enriched_diffs[1:]: + combined_diff = await self.diff_combiner.combine(earlier_diff=combined_diff, later_diff=next_diff) + aggregated_diffs_by_branch_name[branch.name] = combined_diff + + if len(aggregated_diffs_by_branch_name) > 1: + await self.conflicts_enricher.add_conflicts_to_branch_diff( + base_diff_root=aggregated_diffs_by_branch_name[base_branch.name], + branch_diff_root=aggregated_diffs_by_branch_name[diff_branch.name], + ) + await self.labels_enricher.enrich( + enriched_diff_root=aggregated_diffs_by_branch_name[diff_branch.name], conflicts_only=True + ) + + if tracking_id: for enriched_diff in aggregated_diffs_by_branch_name.values(): - await self.summary_counts_enricher.enrich(enriched_diff_root=enriched_diff) - await self.diff_repo.save(enriched_diff=enriched_diff) - branch_enriched_diff = aggregated_diffs_by_branch_name[diff_branch.name] - await self._update_core_data_checks(enriched_diff=enriched_diff) - return branch_enriched_diff + enriched_diff.tracking_id = tracking_id + if diff_uuids_to_delete: + await self.diff_repo.delete_diff_roots(diff_root_uuids=diff_uuids_to_delete) + + for enriched_diff in aggregated_diffs_by_branch_name.values(): + await self.summary_counts_enricher.enrich(enriched_diff_root=enriched_diff) + await self.diff_repo.save(enriched_diff=enriched_diff) + branch_enriched_diff = aggregated_diffs_by_branch_name[diff_branch.name] + await self._update_core_data_checks(enriched_diff=enriched_diff) + return branch_enriched_diff async def _update_core_data_checks(self, enriched_diff: EnrichedDiffRoot) -> list[Node]: return await self.data_check_synchronizer.synchronize(enriched_diff=enriched_diff) diff --git a/backend/infrahub/lock.py b/backend/infrahub/lock.py index 7b22202f5b..39aa54651b 100644 --- a/backend/infrahub/lock.py +++ b/backend/infrahub/lock.py @@ -209,6 +209,17 @@ def _generate_name(cls, name: str, namespace: Optional[str] = None, local: Optio return new_name + def get_existing( + self, + name: str, + namespace: str | None, + local: Optional[bool] = None, + ) -> InfrahubLock | None: + lock_name = self._generate_name(name=name, namespace=namespace, local=local) + if lock_name not in self.locks: + return None + return self.locks[lock_name] + def get( self, name: str, namespace: Optional[str] = None, local: Optional[bool] = None, in_multi: bool = False ) -> InfrahubLock: From 037a0883835a5de6368959ba6ce4ce44cf6e5bac Mon Sep 17 00:00:00 2001 From: Aaron McCarty Date: Wed, 4 Sep 2024 18:23:34 -0700 Subject: [PATCH 17/29] add locked method to NATSLock --- backend/infrahub/lock.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/backend/infrahub/lock.py b/backend/infrahub/lock.py index 39aa54651b..787a867920 100644 --- a/backend/infrahub/lock.py +++ b/backend/infrahub/lock.py @@ -102,6 +102,9 @@ async def do_acquire(self, token: str) -> bool: async def release(self) -> None: await self.service.cache.delete(key=self.name) + async def locked(self) -> bool: + return await self.service.cache.get(key=self.name) is not None + class InfrahubLock: """InfrahubLock object to provide a unified interface for both Local and Distributed locks. From 2d0d6acfba273dbdddd234a0f70efbea848b1bfd Mon Sep 17 00:00:00 2001 From: Aaron McCarty Date: Wed, 4 Sep 2024 18:24:05 -0700 Subject: [PATCH 18/29] add unit test --- .../unit/core/diff/test_coordinator_lock.py | 57 +++++++++++++++++++ 1 file changed, 57 insertions(+) create mode 100644 backend/tests/unit/core/diff/test_coordinator_lock.py diff --git a/backend/tests/unit/core/diff/test_coordinator_lock.py b/backend/tests/unit/core/diff/test_coordinator_lock.py new file mode 100644 index 0000000000..73af4c1c78 --- /dev/null +++ b/backend/tests/unit/core/diff/test_coordinator_lock.py @@ -0,0 +1,57 @@ +import asyncio +from unittest.mock import AsyncMock +from uuid import uuid4 + +import pytest + +from infrahub import lock +from infrahub.core.branch import Branch +from infrahub.core.diff.coordinator import DiffCoordinator +from infrahub.core.diff.data_check_synchronizer import DiffDataCheckSynchronizer +from infrahub.core.initialization import create_branch +from infrahub.core.node import Node +from infrahub.database import InfrahubDatabase +from infrahub.dependencies.registry import get_component_registry + + +class TestDiffCoordinatorLocks: + @pytest.fixture + async def branch_data(self, db: InfrahubDatabase, default_branch: Branch, car_person_schema): + lock.initialize_lock(local_only=True) + branch_1 = await create_branch(branch_name="branch_1", db=db) + for _ in range(10): + person = await Node.init(db=db, schema="TestPerson", branch=default_branch) + await person.new(db=db, name=str(uuid4), height=180) + await person.save(db=db) + for _ in range(10): + person = await Node.init(db=db, schema="TestPerson", branch=branch_1) + await person.new(db=db, name=str(uuid4), height=180) + await person.save(db=db) + + return branch_1 + + async def get_diff_coordinator(self, db: InfrahubDatabase, diff_branch: Branch) -> DiffCoordinator: + component_registry = get_component_registry() + diff_coordinator = await component_registry.get_component(DiffCoordinator, db=db, branch=diff_branch) + mock_synchronizer = AsyncMock(spec=DiffDataCheckSynchronizer) + diff_coordinator.data_check_synchronizer = mock_synchronizer + wrapped_repo = AsyncMock(wraps=diff_coordinator.diff_repo) + diff_coordinator.diff_repo = wrapped_repo + wrapped_calculator = AsyncMock(wraps=diff_coordinator.diff_calculator) + diff_coordinator.diff_calculator = wrapped_calculator + return diff_coordinator + + async def test_incremental_locks_do_not_queue_up(self, db: InfrahubDatabase, default_branch: Branch, branch_data): + diff_branch = branch_data + diff_coordinator = await self.get_diff_coordinator(db=db, diff_branch=diff_branch) + + results = await asyncio.gather( + diff_coordinator.update_branch_diff(base_branch=default_branch, diff_branch=diff_branch), + diff_coordinator.update_branch_diff(base_branch=default_branch, diff_branch=diff_branch), + ) + assert len(results) == 2 + assert results[0] == results[1] + # called once to calculate diff on main and once to calculate diff on the branch + assert len(diff_coordinator.diff_calculator.calculate_diff.call_args_list) == 2 + # called instead of calculating the diff again + diff_coordinator.diff_repo.get_one.assert_awaited_once() From aef7e5a963143eb93a75c805035cc7981b958692 Mon Sep 17 00:00:00 2001 From: Aaron McCarty Date: Wed, 4 Sep 2024 18:30:28 -0700 Subject: [PATCH 19/29] add some debug logging statements --- backend/infrahub/core/diff/coordinator.py | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/backend/infrahub/core/diff/coordinator.py b/backend/infrahub/core/diff/coordinator.py index 600a06ef05..06289511f6 100644 --- a/backend/infrahub/core/diff/coordinator.py +++ b/backend/infrahub/core/diff/coordinator.py @@ -5,6 +5,7 @@ from infrahub import lock from infrahub.core.timestamp import Timestamp +from infrahub.log import get_logger from .model.path import BranchTrackingId, EnrichedDiffRoot, NameTrackingId, TimeRange, TrackingId @@ -22,6 +23,9 @@ from .repository.repository import DiffRepository +log = get_logger() + + @dataclass class EnrichedDiffRequest: base_branch: Branch @@ -94,6 +98,7 @@ def _get_lock_name(self, base_branch_name: str, diff_branch_name: str, is_increm return lock_name async def update_branch_diff(self, base_branch: Branch, diff_branch: Branch) -> EnrichedDiffRoot: + log.debug(f"Received request to update branch diff for {base_branch.name} - {diff_branch.name}") incremental_lock_name = self._get_lock_name( base_branch_name=base_branch.name, diff_branch_name=diff_branch.name, is_incremental=True ) @@ -101,7 +106,9 @@ async def update_branch_diff(self, base_branch: Branch, diff_branch: Branch) -> name=incremental_lock_name, namespace=self.lock_namespace ) if existing_incremental_lock and await existing_incremental_lock.locked(): + log.debug(f"Branch diff update for {base_branch.name} - {diff_branch.name} already in progress") async with self.lock_registry.get(name=incremental_lock_name, namespace=self.lock_namespace): + log.debug(f"Existing branch diff update for {base_branch.name} - {diff_branch.name} complete") return await self.diff_repo.get_one( tracking_id=BranchTrackingId(name=diff_branch.name), diff_branch_name=diff_branch.name ) @@ -115,6 +122,7 @@ async def update_branch_diff(self, base_branch: Branch, diff_branch: Branch) -> self.lock_registry.get(name=general_lock_name, namespace=self.lock_namespace), self.lock_registry.get(name=incremental_lock_name, namespace=self.lock_namespace), ): + log.debug(f"Acquired lock to run branch diff update for {base_branch.name} - {diff_branch.name}") return await self._update_diffs( base_branch=base_branch, diff_branch=diff_branch, @@ -138,6 +146,7 @@ async def create_or_update_arbitrary_timeframe_diff( base_branch_name=base_branch.name, diff_branch_name=diff_branch.name, is_incremental=False ) async with self.lock_registry.get(name=general_lock_name, namespace=self.lock_namespace): + log.debug(f"Acquired lock to run arbitrary diff update for {base_branch.name} - {diff_branch.name}") return await self._update_diffs( base_branch=base_branch, diff_branch=diff_branch, From 47951cebd79b59365f8fcc3482fa499381bb89a9 Mon Sep 17 00:00:00 2001 From: Aaron McCarty Date: Wed, 4 Sep 2024 19:13:55 -0700 Subject: [PATCH 20/29] add exception handling for deleted proposed change --- .../integrity/object_conflict/conflict_recorder.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/backend/infrahub/core/integrity/object_conflict/conflict_recorder.py b/backend/infrahub/core/integrity/object_conflict/conflict_recorder.py index a50c280890..ad7a6c9e81 100644 --- a/backend/infrahub/core/integrity/object_conflict/conflict_recorder.py +++ b/backend/infrahub/core/integrity/object_conflict/conflict_recorder.py @@ -8,6 +8,7 @@ from infrahub.core.protocols import CoreProposedChange from infrahub.core.timestamp import Timestamp from infrahub.database import InfrahubDatabase +from infrahub.exceptions import NodeNotFoundError class ObjectConflictValidatorRecorder: @@ -18,9 +19,12 @@ def __init__(self, db: InfrahubDatabase, validator_kind: str, validator_label: s self.check_schema_kind = check_schema_kind async def record_conflicts(self, proposed_change_id: str, conflicts: Sequence[ObjectConflict]) -> list[Node]: - proposed_change = await NodeManager.get_one_by_id_or_default_filter( - id=proposed_change_id, kind=InfrahubKind.PROPOSEDCHANGE, db=self.db - ) + try: + proposed_change = await NodeManager.get_one_by_id_or_default_filter( + id=proposed_change_id, kind=InfrahubKind.PROPOSEDCHANGE, db=self.db + ) + except NodeNotFoundError: + return [] proposed_change = cast(CoreProposedChange, proposed_change) validator = await self.get_or_create_validator(proposed_change) await self.initialize_validator(validator) From 8d5e8ad7a27dca90c0eb7199857c2c38da3cbd85 Mon Sep 17 00:00:00 2001 From: Aaron McCarty Date: Wed, 4 Sep 2024 19:26:34 -0700 Subject: [PATCH 21/29] pylint exclusion --- .../core/integrity/object_conflict/conflict_recorder.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/backend/infrahub/core/integrity/object_conflict/conflict_recorder.py b/backend/infrahub/core/integrity/object_conflict/conflict_recorder.py index ad7a6c9e81..39381b85f5 100644 --- a/backend/infrahub/core/integrity/object_conflict/conflict_recorder.py +++ b/backend/infrahub/core/integrity/object_conflict/conflict_recorder.py @@ -18,7 +18,7 @@ def __init__(self, db: InfrahubDatabase, validator_kind: str, validator_label: s self.validator_label = validator_label self.check_schema_kind = check_schema_kind - async def record_conflicts(self, proposed_change_id: str, conflicts: Sequence[ObjectConflict]) -> list[Node]: + async def record_conflicts(self, proposed_change_id: str, conflicts: Sequence[ObjectConflict]) -> list[Node]: # pylint: disable=too-many-branches try: proposed_change = await NodeManager.get_one_by_id_or_default_filter( id=proposed_change_id, kind=InfrahubKind.PROPOSEDCHANGE, db=self.db From bd0f1509006d79856cee9ff7f795dd9b7d97d787 Mon Sep 17 00:00:00 2001 From: pa-lem Date: Thu, 5 Sep 2024 11:10:24 +0200 Subject: [PATCH 22/29] add conflict at top level if needed --- .../getProposedChangesDiffTree.ts | 12 +++++ .../screens/diff/node-diff/node-attribute.tsx | 4 ++ .../screens/diff/node-diff/node-property.tsx | 4 +- .../app/src/screens/diff/node-diff/node.tsx | 44 ++++++++++++------- 4 files changed, 45 insertions(+), 19 deletions(-) diff --git a/frontend/app/src/graphql/queries/proposed-changes/getProposedChangesDiffTree.ts b/frontend/app/src/graphql/queries/proposed-changes/getProposedChangesDiffTree.ts index d6fcecae96..589f29145b 100644 --- a/frontend/app/src/graphql/queries/proposed-changes/getProposedChangesDiffTree.ts +++ b/frontend/app/src/graphql/queries/proposed-changes/getProposedChangesDiffTree.ts @@ -70,6 +70,18 @@ export const getProposedChangesDiffTree = gql` contains_conflict last_changed_at name + conflict { + base_branch_label + base_branch_action + base_branch_changed_at + base_branch_value + diff_branch_label + diff_branch_action + diff_branch_changed_at + diff_branch_value + selected_branch + uuid + } properties { conflict { base_branch_label diff --git a/frontend/app/src/screens/diff/node-diff/node-attribute.tsx b/frontend/app/src/screens/diff/node-diff/node-attribute.tsx index f6ddae8e2c..31e095deae 100644 --- a/frontend/app/src/screens/diff/node-diff/node-attribute.tsx +++ b/frontend/app/src/screens/diff/node-diff/node-attribute.tsx @@ -3,6 +3,7 @@ import { DiffRow } from "@/screens/diff/node-diff/utils"; import { DiffAttribute, DiffStatus } from "@/screens/diff/node-diff/types"; import { DiffThread } from "@/screens/diff/node-diff/thread"; import { DiffNodeProperty } from "@/screens/diff/node-diff/node-property"; +import { Conflict } from "./conflict"; type DiffNodeAttributeProps = { attribute: DiffAttribute; @@ -18,6 +19,7 @@ export const DiffNodeAttribute = ({ status, }: DiffNodeAttributeProps) => { const { "*": branchName } = useParams(); + console.log("attribute: ", attribute); return (
+ {attribute.conflict && } + {attribute.properties.map((property, index: number) => ( ))} diff --git a/frontend/app/src/screens/diff/node-diff/node-property.tsx b/frontend/app/src/screens/diff/node-diff/node-property.tsx index 7a89275839..f10b7b9f49 100644 --- a/frontend/app/src/screens/diff/node-diff/node-property.tsx +++ b/frontend/app/src/screens/diff/node-diff/node-property.tsx @@ -14,7 +14,7 @@ type DiffNodePropertyProps = { status: DiffStatus; }; -const getPreviousValue = (property: DiffProperty) => { +export const getPreviousValue = (property: DiffProperty) => { const previousValue = formatValue(property.previous_value); if (previousValue === null) return null; @@ -36,7 +36,7 @@ const getPreviousValue = (property: DiffProperty) => { ); }; -const getNewValue = (property: DiffProperty) => { +export const getNewValue = (property: DiffProperty) => { const newValue = formatValue(property.new_value); if (newValue === null) return null; diff --git a/frontend/app/src/screens/diff/node-diff/node.tsx b/frontend/app/src/screens/diff/node-diff/node.tsx index 04bce7efac..dd48c0f23c 100644 --- a/frontend/app/src/screens/diff/node-diff/node.tsx +++ b/frontend/app/src/screens/diff/node-diff/node.tsx @@ -5,13 +5,14 @@ import { DiffNodeRelationship } from "./node-relationship"; import { DiffNodeAttribute } from "./node-attribute"; import { DiffThread } from "./thread"; import { useLocation, useParams } from "react-router-dom"; -import { DiffBadge, formatValue } from "@/screens/diff/node-diff/utils"; +import { DiffBadge } from "@/screens/diff/node-diff/utils"; import { useAtomValue } from "jotai"; import { schemaKindNameState } from "@/state/atoms/schemaKindName.atom"; import type { DiffNode as DiffNodeType } from "@/screens/diff/node-diff/types"; import { classNames } from "@/utils/common"; import { useEffect, useRef } from "react"; import { Icon } from "@iconify-icon/react"; +import { getNewValue, getPreviousValue } from "./node-property"; type DiffNodeProps = { node: DiffNodeType; @@ -72,10 +73,14 @@ export const DiffNode = ({ sourceBranch, destinationBranch, node }: DiffNodeProp key={index} attribute={attribute} status={node.status} - previousValue={ - valueProperty?.previous_value && formatValue(valueProperty?.previous_value) - } - newValue={status !== "REMOVED" && formatValue(valueProperty?.new_value)} + previousValue={getPreviousValue({ + ...valueProperty, + conflict: attribute.conflict, + })} + newValue={getNewValue({ + ...valueProperty, + conflict: attribute.conflict, + })} /> ); })} @@ -87,16 +92,15 @@ export const DiffNode = ({ sourceBranch, destinationBranch, node }: DiffNodeProp const attribute = { name: relationship.name, contains_conflict: relationship.contains_conflict, - properties: [ - { - conflict: element.conflict, - new_value: element.peer_label, - path_identifier: element.path_identifier, - previous_value: element.conflict?.base_branch_label, - property_type: "HAS_VALUE", - }, - ...element.properties, - ], + properties: element.properties, + conflict: element.conflict, + }; + + const valueProperty = { + conflict: element.conflict, + new_value: element.peer_label, + path_identifier: element.path_identifier, + previous_value: element.conflict?.base_branch_label, }; return ( @@ -104,8 +108,14 @@ export const DiffNode = ({ sourceBranch, destinationBranch, node }: DiffNodeProp key={index} attribute={attribute} status={node.status} - previousValue={element.conflict?.base_branch_label} - newValue={element.peer_label} + previousValue={getPreviousValue({ + ...valueProperty, + conflict: attribute.conflict, + })} + newValue={getNewValue({ + ...valueProperty, + conflict: attribute.conflict, + })} /> ); } From 13c3e30eaafafb7c995e6b902c131b76a7421738 Mon Sep 17 00:00:00 2001 From: pa-lem Date: Thu, 5 Sep 2024 11:11:44 +0200 Subject: [PATCH 23/29] add conflict badge in title for node attribute --- frontend/app/src/screens/diff/node-diff/node-attribute.tsx | 7 +++++-- 1 file changed, 5 insertions(+), 2 deletions(-) diff --git a/frontend/app/src/screens/diff/node-diff/node-attribute.tsx b/frontend/app/src/screens/diff/node-diff/node-attribute.tsx index 31e095deae..2f646e4891 100644 --- a/frontend/app/src/screens/diff/node-diff/node-attribute.tsx +++ b/frontend/app/src/screens/diff/node-diff/node-attribute.tsx @@ -4,6 +4,7 @@ import { DiffAttribute, DiffStatus } from "@/screens/diff/node-diff/types"; import { DiffThread } from "@/screens/diff/node-diff/thread"; import { DiffNodeProperty } from "@/screens/diff/node-diff/node-property"; import { Conflict } from "./conflict"; +import { BadgeConflict } from "../diff-badge"; type DiffNodeAttributeProps = { attribute: DiffAttribute; @@ -19,7 +20,6 @@ export const DiffNodeAttribute = ({ status, }: DiffNodeAttributeProps) => { const { "*": branchName } = useParams(); - console.log("attribute: ", attribute); return ( -
{attribute.name}
+
+ {attribute.name} + {attribute.conflict && Conflict} +
{!branchName && attribute.path_identifier && ( From 9e554a12aebded0b7fe38fc990ebb2fa8a5c54fd Mon Sep 17 00:00:00 2001 From: pa-lem Date: Thu, 5 Sep 2024 11:16:27 +0200 Subject: [PATCH 24/29] update types and details --- frontend/app/src/screens/diff/node-diff/node.tsx | 7 ++++++- frontend/app/src/screens/diff/node-diff/types.ts | 16 +++++++++------- 2 files changed, 15 insertions(+), 8 deletions(-) diff --git a/frontend/app/src/screens/diff/node-diff/node.tsx b/frontend/app/src/screens/diff/node-diff/node.tsx index dd48c0f23c..7c737e5780 100644 --- a/frontend/app/src/screens/diff/node-diff/node.tsx +++ b/frontend/app/src/screens/diff/node-diff/node.tsx @@ -8,7 +8,7 @@ import { useLocation, useParams } from "react-router-dom"; import { DiffBadge } from "@/screens/diff/node-diff/utils"; import { useAtomValue } from "jotai"; import { schemaKindNameState } from "@/state/atoms/schemaKindName.atom"; -import type { DiffNode as DiffNodeType } from "@/screens/diff/node-diff/types"; +import type { DiffNode as DiffNodeType, PropertyType } from "@/screens/diff/node-diff/types"; import { classNames } from "@/utils/common"; import { useEffect, useRef } from "react"; import { Icon } from "@iconify-icon/react"; @@ -94,6 +94,8 @@ export const DiffNode = ({ sourceBranch, destinationBranch, node }: DiffNodeProp contains_conflict: relationship.contains_conflict, properties: element.properties, conflict: element.conflict, + path_identifier: element.path_identifier, + uuid: element.uuid, }; const valueProperty = { @@ -101,6 +103,9 @@ export const DiffNode = ({ sourceBranch, destinationBranch, node }: DiffNodeProp new_value: element.peer_label, path_identifier: element.path_identifier, previous_value: element.conflict?.base_branch_label, + property_type: "HAS_VALUE" as PropertyType, + last_changed_at: element.last_changed_at, + status: element.status, }; return ( diff --git a/frontend/app/src/screens/diff/node-diff/types.ts b/frontend/app/src/screens/diff/node-diff/types.ts index 6d2316efde..d27bbb259f 100644 --- a/frontend/app/src/screens/diff/node-diff/types.ts +++ b/frontend/app/src/screens/diff/node-diff/types.ts @@ -1,3 +1,11 @@ +export type PropertyType = + | "HAS_VALUE" + | "HAS_OWNER" + | "HAS_SOURCE" + | "IS_VISIBLE" + | "IS_PROTECTED" + | "IS_RELATED"; + export type DiffStatus = "ADDED" | "UPDATED" | "REMOVED" | "UNCHANGED"; export type DiffConflict = { @@ -15,13 +23,7 @@ export type DiffProperty = { conflict: DiffConflict | null; new_value: any; previous_value: any; - property_type: - | "HAS_VALUE" - | "HAS_OWNER" - | "HAS_SOURCE" - | "IS_VISIBLE" - | "IS_PROTECTED" - | "IS_RELATED"; + property_type: PropertyType; path_identifier: string | null; status: DiffStatus; }; From 39a260ad98c3e534b853e4497f5553a371b69f63 Mon Sep 17 00:00:00 2001 From: pa-lem Date: Thu, 5 Sep 2024 11:55:10 +0200 Subject: [PATCH 25/29] update toggle button ui --- frontend/app/src/components/buttons/toggle-buttons.tsx | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/frontend/app/src/components/buttons/toggle-buttons.tsx b/frontend/app/src/components/buttons/toggle-buttons.tsx index a3d587b693..0c7b204dc6 100644 --- a/frontend/app/src/components/buttons/toggle-buttons.tsx +++ b/frontend/app/src/components/buttons/toggle-buttons.tsx @@ -20,7 +20,8 @@ export const ToggleButtons = ({ tabs, ...props }: TabsProps) => {