fix: Resources on graph based on name AND apiVersion #307
Conversation
Hubert-Szczepanski-SAP
changed the title
There was a problem hiding this comment.
Pull Request Overview
This PR changes the resource identification strategy in the graph component from using only the resource name to using a combination of name and API version. This prevents collisions when resources have the same name but different API versions.
Key changes:
- Modified resource ID generation to include both name and API version
- Updated reference resolution to consistently use the new ID format
- Restructured code to apply the new ID format to both parent and extra references
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
src/components/Graphs/useGraph.ts
Outdated
| const id = item?.metadata?.name; | ||
| const name = item?.metadata?.name; | ||
| const apiVersion = item?.apiVersion ?? ''; | ||
| const id = `${name}-${apiVersion}`; |
There was a problem hiding this comment.
The ID generation could create collisions when resource names contain hyphens. For example, 'my-resource' with apiVersion 'v1' would have the same ID as 'my' with apiVersion 'resource-v1'. Consider using a different separator like '::' or implement a more robust encoding scheme.
Copilot uses AI. Check for mistakes.
src/components/Graphs/useGraph.ts
Outdated
| ].filter(Boolean) as string[]; | ||
| const createReferenceIdWithApiVersion = (referenceName: string | undefined) => { | ||
| if (!referenceName) return undefined; | ||
| return `${referenceName}-${apiVersion}`; |
There was a problem hiding this comment.
The reference ID generation uses the current resource's apiVersion for all references, but referenced resources may have different API versions. This could create incorrect references to non-existent nodes in the graph.
Copilot uses AI. Check for mistakes.
There was a problem hiding this comment.
You can check Copilot's suggestions, to me it seems fine. 🙏
One thought: since this was a bugfix, it might make sense to add some unit tests. We could extract the transformation logic (providerConfigsList + managedResources → treeData, lines 100-184) into a separate function and test it properly. What do you think? Could also do it as a separate backlog item.
Hubert-Szczepanski-SAP
deleted the
fix/visualise-resources-on-graph-by-name-and-version
branch
2 participants
What this PR does / why we need it:
Changing the way we map resources from only name to name + apiVersion (
${name}-${apiVersion})After change:
Which issue(s) this PR fixes:
Fixes #
Special notes for your reviewer: