(Outdated) Showing Fleets in the Treeview #1220

JunyuQian · 2025-02-05T01:35:26Z

Showing Fleets in the Treeview

This PR adds the Fleet resource to the treeview, based on @serbrech's initial work. The image below shows the final result of the treeview.

Due to the special characteristics that a Fleet can have member clusters from different subscriptions, the initial treeview design, which classifies clusters based on subscriptions, does not quite fit Fleets. The current design can be summarized into three cases below:

Key Changes

New Files

fleet-tree-icon.png: The fleet icon displayed in the treeview.
fleetTreeItem.ts: The class responsible for the Fleet node in the treeview. It includes the createFleetTreeNode function and all necessary properties.

Modified Files

azure-api-util.ts: Modified the parseResource function to enable parsing a member cluster to a cluster resource.
azureResources.ts: Defined the DefinedFleetMemberWithGroup type, added Fleet-related resource types, and added functions to retrieve Fleet members and convert a FleetMember to a DefinedFleetMemberWithGroup.
config.ts: Refactored and added the cluster filtering function implemented in this PR #1208.
aksClusterTreeItem.ts: Deleted the subscriptionId field and used the clusterResource's id to get the subscriptionId for more concise code. Modified the parent node type so that the parent node of a cluster can be either a subscription or a Fleet.
subscriptionTreeItem.ts: Merged the cluster filter into the current PR by refactoring and moving the related code into config.ts. Added the process of fetching fleets and mapping members to their corresponding fleets before loading.

Conflict Resolution (with cluster-filter PR #1208)

This PR also resolves conflicts with the cluster-filter PR by refactoring the filter-related code (mentioned in config.ts and subscriptionTreeItem.ts in the Modified Files section above). However, further integration testing is needed to ensure both functionalities work well together.

Some known issues are:

When the user clicks the filter button next to a subscription, the cluster pool only contains clusters from the current subscription. However, as mentioned above, a Fleet can have member clusters from different subscriptions. In this case, member clusters from other subscriptions will never be visible to the user. The user must go to the corresponding subscription to see the member clusters. It also makes sense to leave it as it is since it is a filter based on the subscription and there might not be a need to have members from another subscription.
Once the user sets some filter conditions, there is no way to clear the filter. This issue has been discussed, and later the user will be able to clear the conditions by unselecting all the filtering conditions.

Next Steps

Further clean up the loadMoreChildrenImpl function in subscriptionTreeItem.ts, as mentioned in this comment #1219.
Implement the hasMoreChildren function in fleetTreeItems.ts, so that the resources will be loaded gradually (e.g., load 10 lines, and display the "load more" button), rather than load them all at once. Currently, in the testing environment, the number of resources is not quite big. But in reality, it should have the ability to handle a large amount of resources.
Implement more integration tests after fixing the issue in the cluster filter to ensure the cluster-filter and fleet-treeview functionalities work well together.

Last but not least, a huge thank-you to @serbrech, who provided immense assistance to this PR and spent hours peer-programming with me. Also, a big thank-you to @Tatsinnit for your support and the discussion this afternoon, where we could finalize this PR ❤️

Looking forward to moving on to the next feature, where we can add a member to a Fleet!

serbrech · 2025-02-05T08:44:38Z

@Tatsinnit please review carefully the merge commit 7259407

probably best evaluated by reading the overall code in subscriptionTreeItem.ts

in short, most of the work in this PR was to change the resource fetching in the subscriptionTree implementation.
Your filter was conflicting, and was moved to a func that encapsulates fetching clusters and fleets.

There is still a gap to apply the same filtering on members under the fleet items.
I suggested to @JunyuQian to apply it without caring too much about duplication for now.
I'll send a little refactor PR after that.

serbrech · 2025-02-05T08:54:08Z

src/azure-api-utils.ts

    const name = bits[bits.length - 1];
-    return { resourceGroupName, name };
+    const parentResourceId = `/${bits.slice(0, bits.length - 2).join("/")}`;
+    return { parentResourceId, subscriptionId, resourceGroupName, name };
 }

 export function parseSubId(armId: string): { subId: string } {


this func was added in your PR @Tatsinnit
follow-up refactoring might be to delete it, and reuse the parseResource that allows extracting more parts.

src/tree/subscriptionTreeItem.ts

src/tree/fleetTreeItem.ts

src/tree/subscriptionTreeItem.ts

Tatsinnit

💡 First pass looks fine, share away the VSIX, we can all shake down a little and if all looks good, lets enable Junyu for his intern work and we can all chip in.

Thanks heaps, fyi @tejhan and @ReinierCC as well please ❤️

eslint.config.mjs

src/azure-api-utils.ts

Tatsinnit · 2025-02-06T20:49:23Z

src/commands/utils/clusterfilter.ts

-                ...validClusters.map((c) => ({ label: c.name!, name: c.name!, id: c.id!, location: c.location! })),
-            );
-        }
+        const aksClusters = await fetchAksClusters(graphServiceClient, subscriptionNode.result.subscriptionId);


💡 Note to other readers, this is Graph API use, I've added this last night and its way more faster. In case anyone else ponders this, after nice cahtup with Junyu and Stephane, Thanks heaps

Tatsinnit · 2025-02-06T20:50:46Z

src/tree/aksClusterTreeItem.ts


        this.iconPath = assetUri("resources/aks-tools.png");
-        this.subscriptionTreeNode = parent;
+        if (parent.nodeType === "subscription") {


💡Note only please: This part of the code is something we can nicely verify in our quick shakedown please @tejhan

src/tree/subscriptionTreeItem.ts

JunyuQian · 2025-02-06T23:18:08Z

Hi @Tatsinnit @tejhan @ReinierCC, sorry for the delay and here is the VSIX file :)

vscode-aks-tools-1.5.5-fleet-treeview.vsix.zip

dvadas

Stephane's commits are not verified. I remember this was a problem before, will it make it hard to merge again now?

src/commands/utils/azureResources.ts

dvadas · 2025-02-07T06:42:23Z

src/commands/utils/azureResources.ts

+    try {
+        const response = await client.resources(query);
+
+        const aksClusters: AksCluster[] = response.data.map((resource: AksCluster) => ({


I don't get what this is doing? It looks like it's mapping over a list of AksClusters to produce the exact same list of AksClusters. Why can't you just pass response.data directly into the errmap constructor?

dvadas · 2025-02-07T06:46:13Z

src/commands/utils/azureResources.ts

+
+function asFleetMemberWithGroup(member: FleetMember): DefinedFleetMemberWithGroup {
+    return {
+        resourceGroup: parseResource(member.id!).resourceGroupName!,


Rather than using the non-null assertions here, it'd be safer to check for nulls explicitly. You can pass null back to the caller in that case, and in getFleetMembers, handle the error properly. You already use Errorable there, so it'll fit in to the type there.

dvadas · 2025-02-07T06:55:15Z

src/commands/utils/azureResources.ts

+            subscriptionId: resource.subscriptionId,
+            type: resource.type,
+        }));
+        return errmap({ succeeded: true, result: aksClusters }, (resources) => resources);


You're not doing any mapping here. You could just return success(aksClusters).

src/commands/utils/clusterfilter.ts

dvadas · 2025-02-07T07:09:24Z

src/commands/utils/azureResources.ts

+): Promise<Errorable<AksCluster[]>> {
+    const client = getGraphResourceClient(sessionProvider);
+    const query = {
+        query: `Resources | where type =~ '${clusterResourceType}' or type =~ '${fleetResourceType}' | project id, name, location, resourceGroup, subscriptionId, type`,


You retrieve Fleets here as well, but you return a list of AksClusters. Maybe rename that AksCluster type so it's clear that it encompasses multiple Azure resource types.

dvadas · 2025-02-07T07:18:45Z

src/commands/utils/clusterfilter.ts

@@ -79,6 +73,34 @@ export default async function aksClusterFilter(_context: IActionContext, target:
    await setFilteredClusters(newFilteredClusters);
 }

+async function fetchAksClusters(


This looks the same as getClusterAndFleetResourcesFromGraphAPI in azureResources.ts. Can you factor them out, e.g. by having 1 function that takes in the query string?

❤️ most of these comments are catered for your PR @JunyuQian here: https://github.com/JunyuQian/vscode-aks-tools/pull/4/files

Thank you!

* Use arm resrource graph api, which is way faster then iterator api. * Remove unused import types. * Optimise mulitple call to 1 call and add getClusterAndFleetResourcesFromGraphAPI. * Help Junyu for recommended cahnges. * prettier fix.

Tatsinnit · 2025-02-12T02:11:03Z

👮 Closing this in favour of #1237 because its a more re-work, with fixes rebase issues. Thanks, (If this is a mistake we can always reopen)