opsmill / infrahub

Infrahub - A new approach to Infrastructure Management
https://opsmill.com/
GNU Affero General Public License v3.0
196 stars 11 forks source link

bug: failing schema integrity checks are missing a message and have a confusing title #2796

Closed wvandeun closed 5 months ago

wvandeun commented 6 months ago

Component

API Server / GraphQL

Current Behavior

When you create a proposed change with a data change that violates the schema, you get one (or more) failing schema integrity checks with the title "Data Conflict" and no additional message.

The Data conflict title for the check is confusing. As we also have a data conflict check for the data integrity tests. Also there is not really a conflict as such, but a data change that violates the constraints defined in the schema.

image

Expected Behavior

We should have a better name/title for this Schema integrity "Data conflict" test that better describes the goal of the test. If the test fails, sufficient information should be provided to point the user in the direction of the issue so that it can be fixed.

Steps to Reproduce

Additional Information

No response

ogenstad commented 5 months ago

At the moment any relevant information about the actual conflict is hidden within a JSON blob in the "conflicts" key. Looking at a conflict similar to the one above can look like this:

{
  "data": {
    "CoreSchemaCheck": {
      "edges": [
        {
          "node": {
            "display_label": "Data Conflict",
            "conflicts": {
              "value": [
                {
                  "name": "schema/BuiltinTag/name/unique",
                  "type": "attribute.unique.update",
                  "kind": "BuiltinTag",
                  "id": "17c88d06-a070-6778-58d9-17468a21d7fa",
                  "path": "schema/BuiltinTag/name/unique",
                  "branch": "placeholder",
                  "value": "NA"
                }
              ]
            }
          }
        },
        {
          "node": {
            "display_label": "Data Conflict",
            "conflicts": {
              "value": [
                {
                  "name": "schema/BuiltinTag/name/unique",
                  "type": "attribute.unique.update",
                  "kind": "BuiltinTag",
                  "id": "17c88d09-dcfc-5078-58df-17468af9b2f3",
                  "path": "schema/BuiltinTag/name/unique",
                  "branch": "placeholder",
                  "value": "NA"
                }
              ]
            }
          }
        }
      ]
    }
  }
}

I don't think it makes sense for the frontend to parse this JSON blob to figure out a title. I'd suggest that as a quick fix to highlight what the problem is we change the label from "Data Integrity" to the conflict name so it looks like this:

Screenshot 2024-04-22 at 14 07 22

Perhaps it doesn't look great but at least there should be a chance for users to figure out what the problem is even if it is not easy. A problem here is that the values are marked as "NA" so it's hard to figure out which one the problematic node is. Also I'd view the above as one conflict and not two.

This can be changed after #3014 is merged.

diff --git a/backend/infrahub/core/integrity/object_conflict/conflict_recorder.py b/backend/infrahub/core/integrity/object_conflict/conflict_recorder.py
index 1587cde48..bab678926 100644
--- a/backend/infrahub/core/integrity/object_conflict/conflict_recorder.py
+++ b/backend/infrahub/core/integrity/object_conflict/conflict_recorder.py
@@ -64,7 +64,7 @@ class ObjectConflictValidatorRecorder:

                 await conflict_obj.new(
                     db=self.db,
-                    label="Data Conflict",
+                    label=conflict.name,
                     origin="internal",
ogenstad commented 5 months ago

An alternative could look like this where we have the actual ids (I was looking at the wrong branch previously and thought the IDs were something else but we have them for the objects at least):

Screenshot 2024-04-22 at 14 38 34

BRBCoffeeDebuff commented 5 months ago

Adding IDs at least gives users a starting point. In the future I would love to be able to link to the appropriate objects from the error