-
Notifications
You must be signed in to change notification settings - Fork 542
Added dialog for Arm container error #20324
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
base: main
Are you sure you want to change the base?
Conversation
PR Changes
|
Codecov Report❌ Patch coverage is
❌ Your patch status has failed because the patch coverage (44.44%) is below the target coverage (70.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #20324 +/- ##
==========================================
+ Coverage 53.53% 54.52% +0.98%
==========================================
Files 192 200 +8
Lines 16854 17351 +497
Branches 1140 1154 +14
==========================================
+ Hits 9023 9460 +437
- Misses 7831 7891 +60
🚀 New features to boost your workflow:
|
| options: versions, | ||
| validate(_state, value) { | ||
| // Handle ARM64 architecture case where SQL Server 2025 latest is broken | ||
| const isArm64With2025 = arch() === "arm64" && value.toString().includes("2025"); |
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.
Aren't all containers broken in arm64 windows?
Isn't this error specific to arm64 Macs only?
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.
just 2025, and on any arm architecture
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.
If it's broken on any ARM machine, why does one of the error messages specify Apple silicon? I'm still confused about which parts of the [SQL version, OS, architecture] matrix are broken.
SQL Server 2025 is not yet supported on Apple Silicon (ARM).
|
Is the gif created from an actual windows arm machine? Or is it just for demonstarting the error message? |
just demonstrating the error message |
|
@croblesm please take a look, and also provide a link for the arm error docs |
| public static portPlaceholder = l10n.t("Enter port"); | ||
| public static hostnamePlaceholder = l10n.t("Enter hostname"); | ||
| public static sqlServer2025ArmError = l10n.t( | ||
| "SQL Server 2025 is not supported on ARM architecture. Please select a different SQL Server version.", |
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.
This reads a bit off grammatically to me. I think
SQL Server 2025 is not supported on ARM architectures.
(pluralizing "architectures") is correct. As would "on the ARM architecture", but I like the former better.
| arch() === "arm64" && | ||
| state.formState.version.includes("2025") | ||
| ) { | ||
| state.dialog = { |
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.
Can you create an issue for reverting this once CU1 is released (if one doesn't already exist), stick in the January 2026 milestone, and add a link to it in a comment here?
| options: versions, | ||
| validate(_state, value) { | ||
| // Handle ARM64 architecture case where SQL Server 2025 latest is broken | ||
| const isArm64With2025 = arch() === "arm64" && value.toString().includes("2025"); |
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.
If it's broken on any ARM machine, why does one of the error messages specify Apple silicon? I'm still confused about which parts of the [SQL version, OS, architecture] matrix are broken.
SQL Server 2025 is not yet supported on Apple Silicon (ARM).
| </Text> | ||
| <Text className={classes.contentItem}> | ||
| {Loc.localContainers.see}{" "} | ||
| <Link href="https://aka.ms/sql-container-arm"> |
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.
Nit: pull the URL out into a const if it's also used as the display string for the link.
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.
Pull Request Overview
This PR adds an error dialog to inform ARM-based system users (including Windows ARM and Apple Silicon) that SQL Server 2025 RTM containers are not compatible with their architecture. The dialog appears when users attempt to create or select a SQL Server 2025 container on ARM64 systems, with a link to documentation for more details.
Key Changes:
- Added a new warning dialog component for ARM SQL 2025 incompatibility
- Implemented architecture detection and validation to prevent SQL Server 2025 container selection on ARM64 systems
- Removed hardcoded workaround that downgraded SQL Server 2025 to CTP2.0 on ARM systems
Reviewed Changes
Copilot reviewed 12 out of 12 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/sharedInterfaces/localContainers.ts | Added reducer and context method for closing the ARM error dialog; removed unused platform property |
| src/sharedInterfaces/connectionDialog.ts | Extended dialog types to include the new armSql2025Error dialog |
| src/reactviews/pages/Deployment/deploymentStateProvider.tsx | Registered the closeArmSql2025ErrorDialog action in the deployment state provider |
| src/reactviews/pages/Deployment/LocalContainers/localContainersInputForm.tsx | Added conditional rendering for the ARM SQL 2025 error dialog |
| src/reactviews/pages/Deployment/LocalContainers/armSql2025ErrorDialog.tsx | New dialog component displaying ARM incompatibility warning with documentation link |
| src/reactviews/common/locConstants.ts | Added localized strings for the ARM error dialog content |
| src/deployment/localContainersHelpers.ts | Implemented ARM64 detection and dialog display logic during initialization and validation; added validation to prevent SQL Server 2025 selection on ARM64 |
| src/deployment/dockerUtils.ts | Removed Mac ARM workaround that forced SQL Server 2025 CTP2.0 version |
| src/deployment/deploymentWebviewController.ts | Updated controller to propagate dialog state from deployment type state |
| src/constants/locConstants.ts | Added localized error messages for SQL Server 2025 ARM incompatibility |
| localization/xliff/vscode-mssql.xlf | Added translation units for new ARM error messages |
| localization/l10n/bundle.l10n.json | Added localized strings to the bundle |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| {" "} | ||
| <DialogTitle className={classes.titleDiv}> | ||
| {" "} |
Copilot
AI
Oct 21, 2025
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.
Remove the unnecessary whitespace-only JSX expressions {\" \"} on lines 59 and 61. These are extraneous and serve no functional purpose.
| {" "} | |
| <DialogTitle className={classes.titleDiv}> | |
| {" "} | |
| <DialogTitle className={classes.titleDiv}> | |
Copilot uses AI. Check for mistakes.
| validate(_state, value) { | ||
| // Handle ARM64 architecture case where SQL Server 2025 latest is broken | ||
| const isArm64With2025 = arch() === "arm64" && value.toString().includes("2025"); |
Copilot
AI
Oct 21, 2025
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.
The arch() function from the os module is called every time this validation runs. Consider caching the result at module level or passing it as part of the state to avoid repeated system calls during form validation.
Copilot uses AI. Check for mistakes.
| arch() === "arm64" | ||
| ? LocalContainers.sqlServer2025ArmErrorTooltip | ||
| : LocalContainers.selectImageTooltip, |
Copilot
AI
Oct 21, 2025
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.
The arch() system call is made during form component setup. Consider caching the architecture value at module level to avoid repeated system calls, especially since the architecture won't change during the application's lifetime.
Copilot uses AI. Check for mistakes.
Pull Request Template – vscode-mssql
Description
Fixes #20316
Added an error dialog to give more context to arm users when they attempt to start a SQL 2025 container.
Code Changes Checklist
npm run test)Reviewers: Please read our reviewer guidelines