Fleet Manager - createFleet #1150

JunyuQian · 2025-01-04T05:43:08Z

Initial work from Tatsat + createFleet panel setup

JunyuQian · 2025-01-04T06:00:52Z

@JunyuQian please read the following Contributor License Agreement(CLA). If you agree with the CLA, please reply with the following information.
@microsoft-github-policy-service agree [company="{your company}"]
Options:

(default - no company specified) I have sole ownership of intellectual property rights to my Submissions and I am not making Submissions in the course of work for my employer.
@microsoft-github-policy-service agree
(when company given) I am making Submissions in the course of work for my employer (or my employer has intellectual property rights in my Submissions by contract or applicable law). I have permission from my employer to make Submissions and enter into this Agreement on behalf of my employer. By signing below, the defined term “You” includes me and my employer.
@microsoft-github-policy-service agree company="Microsoft"
Contributor License Agreement

@microsoft-github-policy-service agree company="Microsoft"

JunyuQian · 2025-01-05T23:17:18Z

@serbrech @dvadas

serbrech · 2025-01-06T01:29:16Z

src/panels/CreateFleetPanel.ts

+    private readonly resourceManagementClient: ResourceManagementClient;
+    private readonly fleetClient: ContainerServiceFleetClient;
+    private readonly featureClient: FeatureClient;
+    // private readonly commandId: string;


what is the command id for? will it be used later?

serbrech · 2025-01-06T01:30:05Z

src/panels/CreateFleetPanel.ts

+            // args.isNewResourceGroup,
+            // args.resourceGroupName,
+            // args.location,
+            // args.name,


should these commented out lines be deleted?

serbrech · 2025-01-06T01:33:28Z

src/panels/CreateFleetPanel.ts

+    name: string,
+    resource: Fleet,
+    // sessionProvider: ReadyAzureSessionProvider,
+    // subscriptionId: string,


the client is created using the session and the subscriptionId, so I don't think that you would ever need these parameters here.
if the list of argument increases (probably, to support all the fleet options) then you should create a type to encapsulate them. That way, the function signature won't change when you add more options

I feel this PR is still undergoing changes, testing etc.. it could be still work in progress. Please reach out if you have any questions, Thank you.

It is absolutely work in progress, I'm commenting to help get to a mergeable state in a reasonable time :)
Thanks for reviewing as well @hsubramanianaks!

@serbrech agreed to your point above where it has to be a type for the function parameter.

serbrech · 2025-01-06T01:34:28Z

src/webview-contract/webviewDefinitions/createFleet.ts

+    portalUrl: string;
+};
+
+export interface CreateFleetParams {


isn't this the create fleet argument list?
should this be used as an argument to the createFleet func?

I'm not familiar with typescript, @Tatsinnit might be better guidance here to get to a good place.

Copilot

Copilot reviewed 6 out of 15 changed files in this pull request and generated 2 comments.

Files not reviewed (9)

package.json: Language not supported
webview-ui/src/manualTest/main.tsx: Evaluated as low risk
src/commands/aksFleet/aksFleetManager.ts: Evaluated as low risk
src/webview-contract/webviewTypes.ts: Evaluated as low risk
webview-ui/src/main.tsx: Evaluated as low risk
src/extension.ts: Evaluated as low risk
src/commands/utils/arm.ts: Evaluated as low risk
webview-ui/src/manualTest/createFleetTests.tsx: Evaluated as low risk
src/panels/CreateFleetPanel.ts: Evaluated as low risk

Comments suppressed due to low confidence (8)

webview-ui/src/CreateFleet/CreateFleetInput.tsx:60

[nitpick] The error message for the fleet name validation is too verbose. Consider simplifying it to: 'Fleet name can only contain letters, numbers, dashes, and underscores, and must start and end with a letter or number.'

The only allowed characters are letters, numbers, dashes, and underscore. The first and last character must be a letter or a number.

webview-ui/src/CreateFleet/helpers/state.ts:13

Enum values should be in uppercase. Rename 'InProgress' to 'IN_PROGRESS'.

    InProgress,

webview-ui/src/CreateFleet/helpers/state.ts:15

Enum values should be in uppercase. Rename 'Cancelled' to 'CANCELLED'.

    Cancelled,

webview-ui/src/CreateFleet/helpers/state.ts:17

Enum values should be in uppercase. Rename 'Failed' to 'FAILED'.

    Failed,

webview-ui/src/CreateFleet/helpers/state.ts:17

Enum values should be in uppercase. Rename 'Success' to 'SUCCESS'.

    Success,

webview-ui/src/CreateFleet/helpers/state.ts:83

[nitpick] Consider adding a success message for the 'ProgressEventType.SUCCESS' case.

            return { stage: Stage.Succeeded, message: null };

src/commands/utils/fleet.ts:12

Ensure that result.name is not null or undefined before using it. Consider adding a check to handle the case where result.name is null or undefined.

return { succeeded: true, result: result.name! };

src/webview-contract/webviewDefinitions/createFleet.ts:32

[nitpick] The enum values 'With' and 'Without' are ambiguous. They should be renamed to 'WithHubCluster' and 'WithoutHubCluster' to make it clear what they represent.

export enum HubClusterMode { With, Without }

Copilot · 2025-01-06T01:37:43Z

webview-ui/src/CreateFleet/CreateFleetInput.tsx

+        console.log("Location:", props.locations);
+        validate();
+        setLocation(missing<string>("somewhere"));
+        // const parameters = validate();


The handleSubmit function contains commented-out code and incomplete logic. Consider removing the commented-out code and ensuring the form submission logic is complete.

Suggested change

// const parameters = validate();

const parameters = validate(); if (parameters.isJust) { props.eventHandlers.onSetCreating({ parameters: parameters.value }); }

Copilot · 2025-01-06T01:37:44Z

webview-ui/src/CreateFleet/helpers/state.ts

+        case ProgressEventType.InProgress:
+            return { stage: Stage.Creating, message: operationDescription };
+        case ProgressEventType.Cancelled:
+            return { stage: Stage.Failed, message: "Fleet Creation is cancelled" };


The error message should end with a period.

Suggested change

return { stage: Stage.Failed, message: "Fleet Creation is cancelled" };

return { stage: Stage.Failed, message: "Fleet Creation is cancelled." };

serbrech · 2025-01-06T01:38:15Z

webview-ui/src/CreateFleet/helpers/state.ts

@@ -0,0 +1,91 @@
+import {


could you add comment at the top of this file to explain what state this file defines and encodes, where it's used, etc?

this state.ts file is required for communication between webview and extension. webview is for user experience (UI components) and actual extension handles everything with the vscode. this file is for state management of create fleet flow.

serbrech · 2025-01-06T01:40:20Z

webview-ui/src/CreateFleet/CreateFleetInput.tsx

+            isNewResourceGroup = false;
+            // } else if (newResourceGroupName) {
+            //     resourceGroupName = newResourceGroupName;
+            //     isNewResourceGroup = true;


can we delete all code related to "is new resource group" and just require the resource group to be pre-created at first? we can think about this create RG flow later if necessary

hsubramanianaks · 2025-01-06T02:49:26Z

src/panels/CreateFleetPanel.ts

+        this.fleetClient = getAksFleetClient(sessionProvider, this.subscriptionId);
+        this.featureClient = getFeatureClient(sessionProvider, this.subscriptionId);
+        // this.commandId = commandId;
+        console.log(this.resourceManagementClient, this.featureClient, this.sessionProvider);


this console.log was added for testing purpose? can this be removed ?

hsubramanianaks · 2025-01-06T02:53:21Z

src/panels/CreateFleetPanel.ts

+        try {
+            await createFleet(this.fleetClient, resourceGroupName, name, resource);
+        } catch (error) {
+            console.log(error);


Please throw the error message as notification to the user. Please refer other CreateClusterPanel.ts for error handling.

Tatsinnit and others added 10 commits December 13, 2024 13:05

Initial fleet commit for junyu help and create fleet functionality hook.

b2c7756

fleet-creat data panel setup

41cf70e

Merge branch 'Azure:main' into fleet-creation

3d37144

Merge branch 'Azure:main' into fleet-creation

080f197

testing files setup

df8f5f8

empty page loaded

9bf5e6a

resc group hardcoded dropdown

9840ffd

panel parameters setup

a20406d

formatting and webpage restructuring

5b9e42a

resolve conflict and merge

9aa6b3b

serbrech reviewed Jan 6, 2025

View reviewed changes

Tatsinnit added the enhancement 🚀 New feature or request or improvements on existing code. label Jan 6, 2025

Tatsinnit assigned Tatsinnit and JunyuQian Jan 6, 2025

Tatsinnit requested review from Tatsinnit, qpetraroia, ReinierCC, tejhan and hsubramanianaks January 6, 2025 01:33

serbrech reviewed Jan 6, 2025

View reviewed changes

Tatsinnit linked an issue Jan 6, 2025 that may be closed by this pull request

Early Day Idea: U/X Re-design estimate work for thinking for adding Fleet like concept in the VsCode Extension. #679

Open

Tatsinnit requested a review from Copilot January 6, 2025 01:36

Copilot AI reviewed Jan 6, 2025

View reviewed changes

serbrech reviewed Jan 6, 2025

View reviewed changes

hsubramanianaks reviewed Jan 6, 2025

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fleet Manager - createFleet #1150

Fleet Manager - createFleet #1150

JunyuQian commented Jan 4, 2025

JunyuQian commented Jan 4, 2025

JunyuQian commented Jan 5, 2025

serbrech Jan 6, 2025

serbrech Jan 6, 2025

serbrech Jan 6, 2025

hsubramanianaks Jan 6, 2025

serbrech Jan 6, 2025

hsubramanianaks Jan 6, 2025

serbrech Jan 6, 2025 •

edited

Loading

Copilot AI left a comment

Copilot AI Jan 6, 2025

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

Copilot AI Jan 6, 2025

Provide additional feedback

Please help us improve GitHub Copilot by sharing more details about this comment.

serbrech Jan 6, 2025

hsubramanianaks Jan 6, 2025 •

edited

Loading

serbrech Jan 6, 2025

hsubramanianaks Jan 6, 2025

hsubramanianaks Jan 6, 2025

	// const parameters = validate();
	const parameters = validate(); if (parameters.isJust) { props.eventHandlers.onSetCreating({ parameters: parameters.value }); }

	return { stage: Stage.Failed, message: "Fleet Creation is cancelled" };
	return { stage: Stage.Failed, message: "Fleet Creation is cancelled." };

Fleet Manager - createFleet #1150

Are you sure you want to change the base?

Fleet Manager - createFleet #1150

Conversation

JunyuQian commented Jan 4, 2025

JunyuQian commented Jan 4, 2025

JunyuQian commented Jan 5, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serbrech Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Copilot AI left a comment

Choose a reason for hiding this comment

Copilot AI Jan 6, 2025

Choose a reason for hiding this comment

Copilot AI Jan 6, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hsubramanianaks Jan 6, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

serbrech Jan 6, 2025 •

edited

Loading

hsubramanianaks Jan 6, 2025 •

edited

Loading