-
Notifications
You must be signed in to change notification settings - Fork 5.1k
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
Expose DMS online migration scenarios for public preview #3567
Conversation
Can one of the admins verify this patch? |
Automation for azure-sdk-for-pythonThe initial PR has been merged into your service PR: |
Automation for azure-sdk-for-rubyNothing to generate for azure-sdk-for-ruby |
@@ -2,7 +2,7 @@ | |||
"swagger": "2.0", | |||
"info": { | |||
"title": "Azure Data Migration Service Resource Provider", |
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 update the terminology to 'Database Migration Service' instead of 'Data Migration Service'. The GA swagger already has this.
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.
@@ -994,6 +1046,52 @@ | |||
} | |||
} | |||
}, | |||
"/subscriptions/{subscriptionId}/resourceGroups/{groupName}/providers/Microsoft.DataMigration/services/{serviceName}/projects/{projectName}/accessArtifacts": { | |||
"post": { |
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.
Not required for public preview.
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.
Removed.
@@ -0,0 +1,16 @@ | |||
{ | |||
"parameters": { |
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 file not required for public preview.
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.
Removed.
Automation for azure-sdk-for-nodeNothing to generate for azure-sdk-for-node |
Automation for azure-sdk-for-goNothing to generate for azure-sdk-for-go |
Automation for azure-sdk-for-javaNothing to generate for azure-sdk-for-java |
@@ -50,6 +50,39 @@ input-file: | |||
- Microsoft.DataMigration/stable/2018-04-19/definitions/MigrationValidation.json | |||
``` | |||
|
|||
### Tag: package-2018-07-15-preview |
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.
You probably want to change line 31 as well, to put package-2018-07-15-preview as the default tag to generate
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.
"MySqlConnectionInfo": { | ||
"x-ms-discriminator-value": "MySqlConnectionInfo", | ||
"type": "object", | ||
"description": "Information for connecting to MySQL source", |
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.
Should this be MySQL server? As it is used for both source and target
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.
"MigrationFromSqlServerToAzureDB", | ||
"MigrationFromSqlServerToAzureMI", | ||
"MigrationFromMySQLToSQL", | ||
"MigrationFromMySQLToAzureDB", |
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.
Is "MigrationFromMySQLToAzureDB" supported? As in MySqlTargetPlatform we only have SqlServer and AzureDbForMySQL
"description": "Input for the task that validates connection to SQL DB and target server requirements", | ||
"properties": { | ||
"sourceConnectionInfo": { | ||
"description": "Connection information for Source SQL Server", |
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: source instead of Source, as below we say target. Both should be consistent
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.
"$ref": "./Common.json#/definitions/SqlConnectionInfo" | ||
}, | ||
"targetConnectionInfo": { | ||
"description": "Connection information for SQL Server", |
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.
SQL DB instead of SQL Server?
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.
"description": "Connection information for source MySQL", | ||
"$ref": "./Common.json#/definitions/MySqlConnectionInfo" | ||
} | ||
}, |
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: Should the order be sourceConnectionInfo, targetConnectionInfo and then selectedDatabases?
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.
"fullLoadEstFinishTime": { | ||
"type": "string", | ||
"format": "date-time", | ||
"description": "Estimate to finish FullLoad", |
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.
full load*
that is what we are using everywhere.
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.
"ConnectToTarget.AzureSqlDbMI", | ||
"ConnectToTarget.SqlDb", | ||
"Migrate.SqlServer.SqlDb", | ||
"ConnectToTarget.SqlServer", | ||
"DataMigration.RunAssessment", |
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.
Isnt DataMigration.RunAssessment a private scenario? I don't think it is exposed.
Also I dont see DataMigration.RunSqlManagedInstanceMigration, DataMigration.SelectTargetServerForSqlManagedInstanceScenario, DataMigration.SelectSourceForSqlManagedInstanceScenario, DataMigration.PrepareSqlManagedInstanceRestore in our AdvisorScenarioConstants.cs
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.
"ValidateMigrationInput.SqlServer.AzureSqlDbMI" | ||
"Migrate.SqlServer.SqlDb", | ||
"DataMigration.PrepareSqlManagedInstanceRestore", | ||
"Migrate.SqlServer.SqlServer", |
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 am not sure if Migrate.SqlServer.SqlServer is public yet. It is still in preview
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.
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 think it looks good now 👍
"type": "string", | ||
"description": "An enumeration of possible target types when migrating from MySQL", | ||
"enum": [ | ||
"SqlServer", |
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?
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
"Default", | ||
"MigrationFromSqlServerToAzureDB", | ||
"MigrationFromSqlServerToAzureMI", | ||
"MigrationFromMySQLToSQL", |
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?
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.
Should these be removed? I think the list should be
"Default",
"MigrationFromSqlServerToAzureDB",
"MigrationFromSqlServerToAzureMI",
"MigrationFromMySQLToAzureDBForMySQL"
These are the scenarios we support though I'm not sure what "ServerLevelPermissionsGroup" represents.
} | ||
] | ||
}, | ||
"ProjectArtifactsResponse": { |
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 ProjectArtifactsResponse
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.
"$ref": "#/definitions/DatabaseInfo" | ||
} | ||
}, | ||
"provisioningState": { |
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.
Shouldn't there be something about online/offline migration in project properties?
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.
Not anymore. Online/Offline is not associated with project but rather activity.
{ | ||
"swagger": "2.0", | ||
"info": { | ||
"title": "Azure Data Migration Service Resource Provider", |
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.
Search for 'Azure Data Migration Service' and replace with 'Azure Database Migration Service
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.
@@ -0,0 +1,46 @@ | |||
{ |
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 don't think this file is required. It is used for the private preview MySQL scenario 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.
Yeah, the objects here don't look like they are referenced anywhere.
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.
My latest commit should have removed this file from the PR.
"type": "string", | ||
"format": "date-time", | ||
"readOnly": true, | ||
"description": "UTC Date and time when the AgentJob was last executed." |
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.
Minor - Can you replace 'AgentJob' in descriptions with 'Agent Job'. Thanks!
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.
"info": { | ||
"title": "Azure Database Migration Service Resource Provider", | ||
"version": "2018-07-15-preview", | ||
"description": "The Data Migration Service helps people migrate their data from on-premise database servers to Azure, or from older database software to newer software. The service manages one or more workers that are joined to a customer's virtual network, which is assumed to provide connectivity to their databases. To avoid frequent updates to the resource provider, data migration tasks are implemented by the resource provider in a generic way as task resources, each of which has a task type (which identifies the type of work to run), input, and output. The client is responsible for providing appropriate task type and inputs, which will be passed through unexamined to the machines that implement the functionality, and for understanding the output, which is passed back unexamined to the client.", |
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.
Search for 'Data Migration Service' and replace with 'Database Migration Service'. You can do this in VS using replace all and limiting search to folder '...\preview\2018-07-15-preview' and replace all files in this 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.
Done.
Has this gone through Azure API review board and ARM API review board? If not, please set something up with armapireview and azureapirbcore |
Adding PostgreSql models to swagger
Fix errors for PostgreSql Task file
Update readme.md to fix path for PostgreSql task json
ping, please set up a meeting as noted in the email. |
reviewed in person. Signing off from ARM side. |
This PR contains changes for our public contract to enable the new online migration scenarios as well as updates to our existing scenarios.
This checklist is used to make sure that common issues in a pull request are addressed. This will expedite the process of getting your pull request merged and avoid extra work on your part to fix issues discovered during the review process.
PR information
api-version
in the path should match theapi-version
in the spec).Quality of Swagger