Skip to content

Changes for Microsoft.DataMigration GA stable version 2018-04-19.#2936

Merged
hovsepm merged 10 commits intoAzure:masterfrom
binuj:master
May 2, 2018
Merged

Changes for Microsoft.DataMigration GA stable version 2018-04-19.#2936
hovsepm merged 10 commits intoAzure:masterfrom
binuj:master

Conversation

@binuj
Copy link
Contributor

@binuj binuj commented Apr 24, 2018

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

  • The title of the PR is clear and informative.
  • There are a small number of commits, each of which have an informative message. This means that previously merged commits do not appear in the history of the PR. For information on cleaning up the commits in your pull request, see this page.
  • Except for special cases involving multiple contributors, the PR is started from a fork of the main repository, not a branch.
  • If applicable, the PR references the bug/issue that it fixes.
  • Swagger files are correctly named (e.g. the api-version in the path should match the api-version in the spec).

Quality of Swagger

binuj added 2 commits April 23, 2018 09:53
…review/2018-03-31-preview'. Next commit will have the actual changes for version 2018-04-19.
@binuj binuj requested a review from hitenjava April 24, 2018 01:25
@AutorestCI
Copy link

AutorestCI commented Apr 24, 2018

Automation for azure-libraries-for-java

Nothing to generate for azure-libraries-for-java

@AutorestCI
Copy link

AutorestCI commented Apr 24, 2018

Automation for azure-sdk-for-node

Nothing to generate for azure-sdk-for-node

@azuresdkciprbot
Copy link

AutoRest linter results for ARM Related Validation Errors/Warnings

These errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns.

File: specification/datamigration/resource-manager/readme.md

⚠️0 new Warnings.(17 total)
0 new Errors.(6 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

AutoRest linter results for SDK Related Validation Errors/Warnings

These errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns.

File: specification/datamigration/resource-manager/readme.md

⚠️0 new Warnings.(29 total)
0 new Errors.(3 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@AutorestCI
Copy link

AutorestCI commented Apr 24, 2018

Automation for azure-sdk-for-python

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-python#2521

@AutorestCI
Copy link

AutorestCI commented Apr 24, 2018

Automation for azure-sdk-for-go

The initial PR has been merged into your service PR:
Azure/azure-sdk-for-go#1768

@azuresdkciprbot
Copy link

AutoRest linter results for ARM Related Validation Errors/Warnings

These errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns.

File: specification/datamigration/resource-manager/readme.md

⚠️0 new Warnings.(17 total)
0 new Errors.(6 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

AutoRest linter results for SDK Related Validation Errors/Warnings

These errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns.

File: specification/datamigration/resource-manager/readme.md

⚠️0 new Warnings.(29 total)
0 new Errors.(3 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

- Microsoft.DataMigration/preview/2018-03-31-preview/definitions/MigrationValidation.json
- Microsoft.DataMigration/preview/2018-03-31-preview/definitions/ValidateMigrationInputSqlServerSqlMITask.json
```

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Line 31 in readme.md has default as "tag: package-2018-03-31-preview".
Change this to the GA version. The public repo sdk picks up the version number from here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, done.

@azuresdkciprbot
Copy link

AutoRest linter results for SDK Related Validation Errors/Warnings

These errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns.

File: specification/datamigration/resource-manager/readme.md

⚠️0 new Warnings.(21 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

AutoRest linter results for ARM Related Validation Errors/Warnings

These errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns.

File: specification/datamigration/resource-manager/readme.md

⚠️0 new Warnings.(16 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

- Microsoft.DataMigration/preview/2018-03-31-preview/definitions/ValidateMigrationInputSqlServerSqlMITask.json
```

### Tag: package-2018-04-19
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

please reorganize readme.md to have most resent tag at the top.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

],
"x-ms-enum": {
"name": "NameCheckFailureReason",
"modelAsString": true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"modelAsString": true [](start = 12, length = 21)

just a reminder - if enum is not modeled as string any time you will need to add a new enum member to it will be a breaking change and will require an API version bump. I'd strongly recommend marking enums as string

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just confirming - There is no change needed right? The usage here follows the recommended way of defining enums, correct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Currently all the enums are marked as "modelAsString": true

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No changes asked for now, just a warning for a possible breaking change in the future.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My comment rather came from already existing enums in Tasks.json (e.g. ProjectTaskProperties.State). Your PR has all the new enums marked as string types which is exactly what I was recommending :) sorry for the confusion.

"description": "The current value of the quota. If null or missing, the current value cannot be determined in the context of the request."
},
"id": {
"type": "string",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't this field read-only? if so should be marked as "readOnly": true,

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general any property of a type that is supposed to be sent only in one direction from Server to Client should be marked as "readOnly": true,


In reply to: 183830418 [](ancestors = 183830418)

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

@hovsepm hovsepm added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Apr 24, 2018
@hovsepm
Copy link
Contributor

hovsepm commented Apr 24, 2018

@ravbhatnagar could you check if this was approved?

"type": "object",
"description": "Information about a single database",
"properties": {
"id": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

id [](start = 9, length = 2)

Readonly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

"type": "string",
"description": "Unique identifier for the database"
},
"name": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

name [](start = 9, length = 4)

Readonly?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

Ordered readme.md contents from latest to oldest version.
Marked response properties as readonly.
@azuresdkciprbot
Copy link

AutoRest linter results for ARM Related Validation Errors/Warnings

These errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns.

File: specification/datamigration/resource-manager/readme.md

⚠️0 new Warnings.(16 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

AutoRest linter results for SDK Related Validation Errors/Warnings

These errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns.

File: specification/datamigration/resource-manager/readme.md

⚠️0 new Warnings.(21 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

"discriminator": "resultType"
},
"MigrateSqlServerSqlDbTaskOutputMigrationLevel": {
"x-ms-discriminator-value": "MigrationLevelOutput",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add "description" for this type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

"StartMigrationScenarioServerRoleResult": {
"type": "object",
"description": "",
"properties": {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please add "description" for this type

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

"type": "string"
"type": "string",
"readOnly": true
},
Copy link
Contributor

@hovsepm hovsepm Apr 26, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if this type is marked as readonly it cannot be also marked as required. Required means that client has to send it to server each time, readonly means client only receives this type and cannot send it back to server. Please fix.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed readonly.

@ravbhatnagar
Copy link
Contributor

The only big change from ARM standpoint is making some properties readonly. Since this change is made in a new api version, and the older version was preview, its fine. signing off!

@ravbhatnagar ravbhatnagar added ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review and removed WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required labels Apr 26, 2018
@azuresdkciprbot
Copy link

AutoRest linter results for SDK Related Validation Errors/Warnings

These errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns.

File: specification/datamigration/resource-manager/readme.md

⚠️0 new Warnings.(15 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

AutoRest linter results for ARM Related Validation Errors/Warnings

These errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns.

File: specification/datamigration/resource-manager/readme.md

⚠️0 new Warnings.(16 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@hovsepm
Copy link
Contributor

hovsepm commented Apr 27, 2018

@binuj could you fix Linter errors? - https://travis-ci.org/Azure/azure-rest-api-specs/jobs/371826015
Look for "type":"Error".

They have pretty good documentation. e.g.

{
  "type": "Error",
  "code": "DefinitionsPropertiesNamesCamelCase",
  "message": "Property named: 'Logins', for definition: 'ConnectToTargetSqlMITaskOutput' must follow camelCase style. Example: 'logins'.",
  "id": "R3016",
  "validationCategory": "RPCViolation",
  "providerNamespace": null,
  "resourceType": null,
  "sources": [
    "file:///home/travis/build/Azure/azure-rest-api-specs/specification/datamigration/resource-manager/Microsoft.DataMigration/preview/2018-03-31-preview/definitions/ConnectToTargetSqlMITask.json:52:8 ($.definitions.ConnectToTargetSqlMITaskOutput.properties.Logins)"
  ],
  "jsonref": "file:///home/travis/build/Azure/azure-rest-api-specs/specification/datamigration/resource-manager/Microsoft.DataMigration/preview/2018-03-31-preview/definitions/ConnectToTargetSqlMITask.json:52:8 ($.definitions.ConnectToTargetSqlMITaskOutput.properties.Logins)",
  "json-path": "file:///home/travis/build/Azure/azure-rest-api-specs/specification/datamigration/resource-manager/Microsoft.DataMigration/preview/2018-03-31-preview/definitions/ConnectToTargetSqlMITask.json:52:8 ($.definitions.ConnectToTargetSqlMITaskOutput.properties.Logins)"
}

@azuresdkciprbot
Copy link

AutoRest linter results for SDK Related Validation Errors/Warnings

These errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns.

File: specification/datamigration/resource-manager/readme.md

⚠️0 new Warnings.(15 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

AutoRest linter results for ARM Related Validation Errors/Warnings

These errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns.

File: specification/datamigration/resource-manager/readme.md

⚠️0 new Warnings.(16 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

AutoRest linter results for ARM Related Validation Errors/Warnings

These errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns.

File: specification/datamigration/resource-manager/readme.md

⚠️0 new Warnings.(16 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

AutoRest linter results for SDK Related Validation Errors/Warnings

These errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns.

File: specification/datamigration/resource-manager/readme.md

⚠️0 new Warnings.(15 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@hovsepm
Copy link
Contributor

hovsepm commented Apr 28, 2018

one left

Executing: npx autorest@2.0.4152 --validation --azure-validator --message-format=json specification/datamigration/resource-manager/readme.md --tag=package-2017-11-15-preview
{
  "type": "Error",
  "code": "RequiredReadOnlyProperties",
  "message": "Property 'taskType' is a required property. It should not be marked as 'readonly'.",
  "id": "R2056",
  "validationCategory": "SDKViolation",
  "providerNamespace": null,
  "resourceType": null,
  "sources": [
    "file:///home/travis/build/Azure/azure-rest-api-specs/specification/datamigration/resource-manager/Microsoft.DataMigration/preview/2017-11-15-preview/definitions/Tasks.json:25:8 ($.definitions.ProjectTaskProperties.properties.taskType)"
  ],
  "jsonref": "file:///home/travis/build/Azure/azure-rest-api-specs/specification/datamigration/resource-manager/Microsoft.DataMigration/preview/2017-11-15-preview/definitions/Tasks.json:25:8 ($.definitions.ProjectTaskProperties.properties.taskType)",
  "json-path": "file:///home/travis/build/Azure/azure-rest-api-specs/specification/datamigration/resource-manager/Microsoft.DataMigration/preview/2017-11-15-preview/definitions/Tasks.json:25:8 ($.definitions.ProjectTaskProperties.properties.taskType)"
}

@hovsepm
Copy link
Contributor

hovsepm commented Apr 28, 2018

There are also bunch of missing documentation fields. They will be used in all the auto-generated languages as hints for developers so it is highly suggested to have human readable documentation text. You can see all the doc issues in this build https://travis-ci.org/Azure/azure-rest-api-specs/jobs/372255731. They start with

{
  "type": "Warning",
  "code": "DescriptionAndTitleMissing",

@azuresdkciprbot
Copy link

AutoRest linter results for SDK Related Validation Errors/Warnings

These errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns.

File: specification/datamigration/resource-manager/readme.md

⚠️0 new Warnings.(11 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

AutoRest linter results for ARM Related Validation Errors/Warnings

These errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns.

File: specification/datamigration/resource-manager/readme.md

⚠️0 new Warnings.(16 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@hovsepm
Copy link
Contributor

hovsepm commented Apr 30, 2018

@binuj please revert changes to the specs of the previous versions - no need to fix those issues in this PR.

…only focused on stable 2018-04-19.

Marked some properties as readonly in  '2018-04-19\definitions\TasksCommon.json'
@azuresdkciprbot
Copy link

AutoRest linter results for SDK Related Validation Errors/Warnings

These errors are reported by the SDK team's validation tools, reachout to ADX Swagger Reviewers directly for any questions or concerns.

File: specification/datamigration/resource-manager/readme.md

⚠️0 new Warnings.(11 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@azuresdkciprbot
Copy link

AutoRest linter results for ARM Related Validation Errors/Warnings

These errors are reported by the ARM team's validation tools, reachout to ARM RP API Review directly for any questions or concerns.

File: specification/datamigration/resource-manager/readme.md

⚠️0 new Warnings.(16 total)
0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

@hovsepm
Copy link
Contributor

hovsepm commented May 2, 2018

@binuj please fix Linter errors like "Property named: 'Logins', for definition: 'ConnectToTargetSqlMITaskOutput' must follow camelCase style. Example: 'logins'.", here https://travis-ci.org/Azure/azure-rest-api-specs/jobs/373778210

@hovsepm
Copy link
Contributor

hovsepm commented May 2, 2018

ignore my last comment - errors are in previous preview specs and were marked by Linter due to description change in your current pr.

@hovsepm hovsepm merged commit deee6ca into Azure:master May 2, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants