Project Import: Use allowed namespaces when importing#7
Project Import: Use allowed namespaces when importing#7sniok wants to merge 2 commits intoAzure:mainfrom
Conversation
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Show resolved
Hide resolved
illume
left a comment
There was a problem hiding this comment.
In the commit messages.. it would be good to have the components modified. eg. "ImportAKSProjects:"
ps. Are we going with aksd:? (or aks-plugin: btw, maybe we should move the folder from "plugins/aks-desktop" to "aks" then we can use the shorter "aks" and not have the plugins folder with the only plugin ever used in this repo?
There was a problem hiding this comment.
Pull request overview
This pull request implements allowed namespace support for AKS project imports and adds cleanup of WebSocket connections after checking if a project is an AKS-managed one.
Changes:
- Add cancellation of namespace WebSocket connection after verifying if a project is AKS-managed
- Configure allowed namespaces in localStorage during cluster import to restrict namespace visibility
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| plugins/aks-desktop/src/utils/shared/isAksProject.tsx | Adds WebSocket connection cancellation after checking if a project is AKS-managed |
| plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx | Configures allowed namespaces in cluster settings during import |
Comments suppressed due to low confidence (1)
plugins/aks-desktop/src/utils/shared/isAksProject.tsx:21
- The cancel function should also be called in the error handler to properly clean up the WebSocket connection. Currently, the WebSocket connection is only cancelled in the success callback but not when an error occurs, which could lead to resource leaks.
() => {
res(false);
},
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| localStorage.getItem(`cluster_settings.${clusterName}`) || '{}' | ||
| ); | ||
| settings.allowedNamespaces ??= []; | ||
| settings.allowedNamespaces.push(...namespacesInCluster.map(it => it.namespace.name)); |
There was a problem hiding this comment.
The allowedNamespaces array can contain duplicates if the import function is called multiple times for the same cluster. Consider checking if a namespace already exists in the array before adding it, or use a Set to ensure uniqueness. This could happen if a user imports the same namespaces again.
| settings.allowedNamespaces.push(...namespacesInCluster.map(it => it.namespace.name)); | |
| const allowedNamespacesSet = new Set<string>(settings.allowedNamespaces); | |
| for (const { namespace } of namespacesInCluster) { | |
| allowedNamespacesSet.add(namespace.name); | |
| } | |
| settings.allowedNamespaces = Array.from(allowedNamespacesSet); |
There was a problem hiding this comment.
I guess this can't hurt doing.
| const settings = JSON.parse( | ||
| localStorage.getItem(`cluster_settings.${clusterName}`) || '{}' | ||
| ); |
There was a problem hiding this comment.
The JSON.parse call could throw an exception if the localStorage value is not valid JSON. While the fallback of '{}' handles the case when the value doesn't exist, if the value exists but is corrupted or malformed, it will throw an error. Consider wrapping this in a try-catch block to handle potential parse errors gracefully.
| const settings = JSON.parse( | |
| localStorage.getItem(`cluster_settings.${clusterName}`) || '{}' | |
| ); | |
| const rawSettings = localStorage.getItem(`cluster_settings.${clusterName}`); | |
| let settings: any; | |
| try { | |
| settings = rawSettings ? JSON.parse(rawSettings) : {}; | |
| } catch { | |
| settings = {}; | |
| } |
There was a problem hiding this comment.
Same here. It's a valid point IMHO.
|
For the commit message, can you please add the component to the context? ImportAKSProjects.tsx |
Requires #8
Use allowed namespaces when importing
Cancel namespace websocket connection after checking if the project is aks one