-
Notifications
You must be signed in to change notification settings - Fork 885
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
Target Provisioning #25426
Target Provisioning #25426
Conversation
Pull Request Test Coverage Report for Build 8340151881Warning: This coverage report may be inaccurate.This pull request's base commit is no longer the HEAD commit of its target branch. This means it includes changes from outside the original pull request, including, potentially, unrelated coverage changes.
Details
💛 - Coveralls |
...nsions/sql-migration/src/dialog/skuRecommendationResults/GenerateProvisioningScriptDialog.ts
Outdated
Show resolved
Hide resolved
|
||
this._saveTemplateLink.onDidClick(async () => { | ||
if (hasRecommendations(this.migrationStateModel)) { | ||
const generateProvisioningScriptDialog = new GenerateProvisioningScriptDialog(this.migrationStateModel, this.migrationTargetType); |
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.
Use Try catch here to catch any error, and log it. It will be helpful while debugging.
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.
done
extensions/sql-migration/src/wizard/assessmentDetailsPage/assessmentDetailsHeader.ts
Outdated
Show resolved
Hide resolved
containerName, | ||
blobName, | ||
permissions: BlobSASPermissions.parse("racwd"), | ||
expiresOn: new Date(new Date().valueOf() + 86400), |
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.
are we expecting 24 Hours expiration?
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.
I just used some random value, can change based on suggestion.
extensions/sql-migration/src/dialog/skuRecommendationResults/SelectStorageAccountDialog.ts
Outdated
Show resolved
Hide resolved
azdata.window.openDialog(this.dialog); | ||
await Promise.all(dialogSetupPromises); | ||
|
||
await this.model.getArmTemplate(this._targetType); |
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 must be under try catch block. and log error if it fails.
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.
there is a try catch block in the getArmTemplate function and error is also logged .
...nsions/sql-migration/src/dialog/skuRecommendationResults/GenerateProvisioningScriptDialog.ts
Outdated
Show resolved
Hide resolved
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.
We have SaveTemplate string outside dialog and also one inside dialog but both has different function. one opens a dialog, other saves in file explorer. May be we should separate the strings? Abhishek can comment on this.
...nsions/sql-migration/src/dialog/skuRecommendationResults/GenerateProvisioningScriptDialog.ts
Outdated
Show resolved
Hide resolved
...nsions/sql-migration/src/dialog/skuRecommendationResults/GenerateProvisioningScriptDialog.ts
Outdated
Show resolved
Hide resolved
extensions/sql-migration/src/dialog/skuRecommendationResults/SelectStorageAccountDialog.ts
Outdated
Show resolved
Hide resolved
extensions/sql-migration/src/wizard/assessmentDetailsPage/assessmentDetailsHeader.ts
Show resolved
Hide resolved
extensions/sql-migration/src/wizard/assessmentDetailsPage/assessmentDetailsHeader.ts
Show resolved
Hide resolved
|
||
// source instance same would be same across all recommendations MI/DB/VM. | ||
let instanceName = model._skuRecommendationResults.recommendations?.sqlMiRecommendationResults[0].sqlInstanceName; | ||
let fileName = `ARMTemplate-${targetType}-${instanceName}-${date}-${time}.json`; |
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 to me, hope name is recommended or approved by PM, as this customer facing.
...nsions/sql-migration/src/dialog/skuRecommendationResults/GenerateProvisioningScriptDialog.ts
Outdated
Show resolved
Hide resolved
Update config.json under sql-migration extension with latest sqltoolsservice build version. |
extensions/sql-migration/src/wizard/skuRecommendation/assessmentSummaryCard.ts
Outdated
Show resolved
Hide resolved
extensions/sql-migration/src/wizard/assessmentDetailsPage/assessmentDetailsHeader.ts
Outdated
Show resolved
Hide resolved
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 to me, just take remaining comments.
This PR contains following changes
UI changes for target provisioning experience as per below mocks.
Mocks - https://www.figma.com/file/UaD2zc58Y2iGKxnqDFQY3Q/End-to-End-Design-GA---SQL-Migration-ADS?type=design&node-id=3298-119337&mode=design&t=v0BLyTpg5zdjtnlu-0
Added RPC Request(GetSqlMigrationGenerateArmTemplateRequest) for the SQL tools service GenerateProvsioningScript API which generates the ARM Template.
Reference code for generating SAS URI to upload ARM template.
https://github.com/Azure-Samples/AzureStorageSnippets/blob/master/blobs/howto/JavaScript/NodeJS-v12/dev-guide/upload-blob-from-blob-sas-token.js
Screenshots of changes
1.