-
Notifications
You must be signed in to change notification settings - Fork 56
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Create Fleet: Full Cycle Completed #1179
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Copilot reviewed 5 out of 7 changed files in this pull request and generated no comments.
Files not reviewed (2)
- webview-ui/src/main.tsx: Evaluated as low risk
- src/extension.ts: Evaluated as low risk
Comments suppressed due to low confidence (2)
webview-ui/src/CreateFleet/CreateFleet.tsx:9
- Ensure that the state transitions and different stages (Uninitialized, Loading, CollectingInput, Creating, Failed, Succeeded) are adequately covered by tests.
const { state, eventHandlers } = useStateManagement(stateUpdater, initialState, vscode);
src/panels/CreateFleetPanel.ts:9
- [nitpick] The error message should be more specific. Consider changing it to: "No locations found for the Microsoft.ContainerService provider."
window.showErrorMessage("No locations found for fleets resource type.");
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good! Took a deep look but no obvious issues seen.
Excited to test out the functionality. On next PR, when command gets enabled, please also share the VSIX file of the built code to test on our end.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 🧋 Thank you so much for this, looks good.
Few things to note and also fyi:
- This PR in current state looks good as it not hooked yet (aka exposed to end-user) the U/X definitely need some touch, so the message here Create Fleet: Full Cycle Completed #1179 (review) (Contains information for how to do that
visx
share.- So, in next week catchup for U/X this will be a great point to show.
I will leave U/X specific and the test level kind help to @ReinierCC and @tejhan for taking a look as well, also Hari can eye-ball as well please.
Thanks heaps.
Thank you all for the comments and thank you @ReinierCC for sharing the docs! I've removed unnecessary code and space in the latest commit. Please let me know if there is anything else that needs attention :) |
Below is the VSIX file that enables the entry point for create-fleet, allowing reviewers to run and test the code locally. Please let me know if there are any issues :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❤️ Thanks heaps, and this looks good to me, again, given its not frontend hooked, we can treat this PR as moving forward.
- An important note for key aspect for this code: cc496a5#diff-158f5d880e339d6252e796c0f377110c33569087f850c46c772cea87dfd63fcbR147
Code:
if (!result.id) {
throw new Error("Fleet creation did not return an ID");
}
Please do think this through
Suggestions for Improvement:
-
Granular Error Context: Provide more context in the error message to assist with debugging. For instance, include the operation or function name and any other relevant details if any (e.g., input parameters).
- Example:
throw new Error(`Fleet creation failed: No ID returned. Input data: ${JSON.stringify(inputData)}`);
- Example:
-
Validation Scope: If
result
itself can be undefined or null, consider adding a null/undefined check before accessing itsid
property to avoid runtime errors.- Example:
if (!result || !result.id) { throw new Error("Fleet creation failed: No valid result or ID returned"); }
- Example:
-
Fallback Options: Depending on the criticality of this operation, consider implementing a fallback or retry mechanism before throwing the error. For instance:
- Retry the operation if it’s a transient issue.
- Provide a default ID or handle the absence of
result.id
gracefully if applicable.
above are just pure ideas which you can keep in mind especially I saw throw
from that error use-case.
Thanks heaps.
Thank you very much Tats! I can replace the value-checking part with not-null assertions in the next PR. I’ll also focus more on content, scope, and fallbacks when writing error messages in future development :) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍! States, messaging & functions look good.
Looking forward to testing out UI implementation
Create Fleet: Full Cycle Completed
This pull request completes the full cycle of creating a fleet. Starting from right-clicking on the subscription, then loading the front-end UI after clicking "Create Fleet," prompting the user to enter inputs, validating the inputs, and finally creating the fleet which is visible in the Azure portal.
Key Changes
New Files:
CreateFleetInput
to show the input form for the user to enter the fleet name, resource group, and location.CollectingInput
. It allows the user to input fleet information and provides an input validation method to ensure the inputs are legal before calling the API and creating the resource.Modified Files:
InProgress
,Success
, andFailed
states, and the transitions among these states. The URL for the user to click if the resources are created successfully is also added in this file.Note
The right-click option (the entry point) is still not enabled in this PR. To use the function, modifying
package.json
is required.Next Steps