Conversation
There was a problem hiding this comment.
Pull request overview
This PR refactors the AKS project import functionality to support importing managed namespace projects when users don't have full cluster access. The implementation switches from iterative namespace discovery to a more efficient Azure Resource Graph query approach and adds support for namespace-scoped credentials during cluster registration.
Changes:
- Refactored namespace discovery to use Azure Resource Graph queries instead of iterating through subscriptions
- Added
managedNamespaceparameter to cluster registration for scoped credential support - Uncommented and enabled the AKS project import feature in the plugin registration
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| plugins/aks-desktop/src/utils/shared/isAksProject.tsx | Added cleanup for API endpoint subscription to prevent memory leaks |
| plugins/aks-desktop/src/utils/azure/aks.ts | Added managedNamespace parameter to registerAKSCluster and removed cluster info validation step |
| plugins/aks-desktop/src/index.tsx | Enabled the previously commented-out AKS project import feature |
| plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx | Complete refactor from subscription-based discovery to Azure Resource Graph queries with simplified UI |
| headlamp | Updated subproject commit reference |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Outdated
Show resolved
Hide resolved
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Outdated
Show resolved
Hide resolved
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Show resolved
Hide resolved
8c1e203 to
fe47b4f
Compare
illume
left a comment
There was a problem hiding this comment.
Looks like a CI check is failing.
Can you please add the changed module to the git commit context part? (Like ImportAKSProjects.tsx)
9e4f19d to
6ac9ea7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Outdated
Show resolved
Hide resolved
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Show resolved
Hide resolved
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Outdated
Show resolved
Hide resolved
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Outdated
Show resolved
Hide resolved
|
Please address the open copilot convos? I'm not sure how to validate this works, and I don't see tests. Can you please add steps to test? |
illume
left a comment
There was a problem hiding this comment.
I checked the copilot comments.
I'm not sure how to validate this works, and I don't see tests. Can you please add steps to test?
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Outdated
Show resolved
Hide resolved
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Outdated
Show resolved
Hide resolved
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Outdated
Show resolved
Hide resolved
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Show resolved
Hide resolved
6ac9ea7 to
c1bc6a1
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 6 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Show resolved
Hide resolved
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Show resolved
Hide resolved
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Outdated
Show resolved
Hide resolved
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Outdated
Show resolved
Hide resolved
|
When it's ready, @vyncent-t will make a video for a11y team. The video should be made only after both PRs are merged (second one below): |
There was a problem hiding this comment.
Please check the commit messages have the relevant components in the context part of the message. If you could please add Why in the git body it's a bit easier to understand?
I think it would be good to add a whole bunch of // @todo notes in the code for things that need cleaning up. There's a lot, but it would be good to add the things found in this review into the code for later. We already have an issue to clean up this code.
If there's an issue for this can you please link it?
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Show resolved
Hide resolved
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Show resolved
Hide resolved
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Outdated
Show resolved
Hide resolved
c1bc6a1 to
29150ec
Compare
skoeva
left a comment
There was a problem hiding this comment.
minor nit - I think you meant "aksd" in this commit:
akds: Dont fetch cluster info when registering a cluster
29150ec to
00ad4b7
Compare
|
I've updated commit messages to include module names and also expanded commit messages to include more information about the changes |
00ad4b7 to
457e7f7
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Show resolved
Hide resolved
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Outdated
Show resolved
Hide resolved
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Show resolved
Hide resolved
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Outdated
Show resolved
Hide resolved
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Outdated
Show resolved
Hide resolved
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx
Show resolved
Hide resolved
illume
left a comment
There was a problem hiding this comment.
Can you please fix the git conflict?
ashu8912
left a comment
There was a problem hiding this comment.
Tested, and it works, thanks, once the comments and merge conflicts are fixed we can merge this.
…ject To allow users access a particular namespace without access to the cluster we are adding that namespace to the list of allowed namespaces
Pass additional information when registering cluster so we can use appropriate az cli command to retrieve credentials to that cluster
Since we can just look at the kubeconfig to see if we need to provide custom kubelogin script we don't need to fetch cluster information
457e7f7 to
205d1d9
Compare
|
Rebased, updated all the strings to be translated and regenerated translation files. |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 43 out of 43 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (4)
plugins/aks-desktop/src/utils/shared/isAksProject.tsx:1
- The watch cancellation only runs on the success callback. On the error callback path, the watch is never cancelled, which can leak the watcher and potentially trigger callbacks after the Promise has already resolved. Cancel the watch in both callbacks (or via a shared finally-like helper) so it always cleans up.
plugins/aks-desktop/src/utils/azure/aks.ts:1 - This hard-codes
isAzureRBACEnabledtofalseafter removing the call that previously retrieved the real value. If the backend registration logic depends on the actual RBAC mode, this will cause incorrect behavior for Azure RBAC-enabled clusters. Prefer passing the real value again (restore the details call), or adjust the IPC contract so the backend determines RBAC mode itself instead of relying on a hard-coded constant.
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx:1 - Treating any
stderroutput as a fatal error can break discovery if Azure CLI emits warnings or non-fatal messages on stderr while still returning valid JSON on stdout. Instead, base failure on the command exit code (preferred) or validate that stdout parses and contains expected fields before failing the operation.
plugins/aks-desktop/src/components/ImportAKSProjects/ImportAKSProjects.tsx:1 - Using
clusterNamealone in the localStorage key can collide when users have clusters with the same name in different subscriptions/resource groups (AKS cluster name is not globally unique). Use a stable unique identifier (e.g., subscriptionId + resourceGroup + clusterName, or the full Azure resource ID) for the storage key to avoid overwriting settings for a different cluster.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
To improve performance we use graph query to fetch all the namespaces with the required labels. This fetches all the namespace with just one request.
205d1d9 to
df40269
Compare
Changes to aks desktop plugin needed to make managed namespace project import work when user doesn't have access to the cluster itself
This PR is one of two PRs to make import work. Second one is here #109
Changes:
How to test: