Service Bus: added Standard to Premium namespace migration API by v-Ajnava · Pull Request #2854 · Azure/azure-rest-api-specs

v-Ajnava · 2018-04-11T23:09:09Z

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

I have read the contribution guidelines.
My spec meets the review criteria:
- The spec conforms to the Swagger 2.0 specification.
- The spec follows the guidelines described in the Swagger checklist document.
- Validation tools were run on swagger spec(s) and have all been fixed in this PR.

AutorestCI · 2018-04-11T23:10:24Z

Automation for azure-sdk-for-python

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-sdk-for-python#2037

AutorestCI · 2018-04-11T23:11:09Z

Automation for azure-sdk-for-node

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

AutorestCI · 2018-04-11T23:11:26Z

Automation for azure-libraries-for-java

Encountered a Subprocess error: (azure-libraries-for-java)

Command: ['/usr/local/bin/autorest', '/tmp/tmpouo3q1gn/rest/specification/servicebus/resource-manager/readme.md', '--azure-libraries-for-java-folder=/tmp/tmpouo3q1gn/sdk', '--fluent', '--java', '--multiapi', '--verbose']
Finished with return code 1
and output:

AutoRest code generation utility [version: 2.0.4262; node: v7.10.1]
(C) 2018 Microsoft Corporation.
https://aka.ms/autorest
   Loading AutoRest core      '/root/.autorest/@microsoft.azure_autorest-core@2.0.4272/node_modules/@microsoft.azure/autorest-core/dist' (2.0.4272)
   Loading AutoRest extension '@microsoft.azure/autorest.java' (~2.1.32->2.1.49)
   Loading AutoRest extension '@microsoft.azure/autorest.modeler' (2.3.38->2.3.38)
FATAL: System.Exception: Duplicate File Generation: src/main/java/com/microsoft/azure/management/servicebus/implementation/PremiumMessagingRegionsInner.java
   at AutoRest.Core.CodeGenerator.<Write>d__13.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at AutoRest.Core.CodeGenerator.<Write>d__12.MoveNext()
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at AutoRest.Java.Azure.Fluent.CodeGeneratorJvaf.<Generate>d__6.MoveNext() in C:\Users\autorest-ci\AppData\Local\Temp\2\PUBLISHedxhk\164_20180307T193002\autorest.java\src\azurefluent\CodeGeneratorJvaf.cs:line 74
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at AutoRest.Java.Program.<ProcessInternal>d__3.MoveNext() in C:\Users\autorest-ci\AppData\Local\Temp\2\PUBLISHedxhk\164_20180307T193002\autorest.java\src\Program.cs:line 115
--- End of stack trace from previous location where exception was thrown ---
   at System.Runtime.ExceptionServices.ExceptionDispatchInfo.Throw()
   at System.Runtime.CompilerServices.TaskAwaiter.HandleNonSuccessAndDebuggerNotification(Task task)
   at NewPlugin.<Process>d__15.MoveNext()
FATAL: java/generate - FAILED
FATAL: Error: Plugin java reported failure.
Process() cancelled due to exception : Plugin java reported failure.
  Error: Plugin java reported failure.

AutorestCI · 2018-04-11T23:12:23Z

Automation for azure-sdk-for-go

A PR has been created for you based on this PR content.

Once this PR will be merged, content will be added to your service PR:
Azure/azure-sdk-for-go#1670

anuchandy · 2018-04-12T23:45:59Z

...ification/servicebus/resource-manager/Microsoft.ServiceBus/stable/2017-04-01/servicebus.json

+        }
+      }
+    },
+    "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.ServiceBus/namespaces/{namespaceName}/migrationConfigurations/$default": {


I understand that $default is the only valid value now but right thing to do here is model it as a parameter with single-value enum instead of hardcoding.

/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName} /providers/Microsoft.ServiceBus/namespaces/{namespaceName} /migrationConfigurations/{configName}

and model {configName} as:

{ "name": "configName", "in": "path", "description": "The configuration name. Should always be \"$default\".", "required": true, "type": "string", "enum": [ "$default" ], "x-ms-enum": { "name": "MigrationConfigurationName", "modelAsString": true } }

you can also move this definition to parameters section and refer it from eligible operations in migrationConfigurations operation group

Since this is an enum with ONE value ($default) and is required there will not be an enum type generated in SDK instead method impl will uses a const with value $default, hence it is not exposed to user i.e. generated methods will not have a "configName" parameter, which is exactly what you want here.

hovsepm · 2018-04-13T20:16:19Z

...ification/servicebus/resource-manager/Microsoft.ServiceBus/stable/2017-04-01/servicebus.json

+          "migrationConfigurations"
+        ],
+        "operationId": "MigrationConfig_upgrade",
+        "description": "This operation disables the Migration",


This should be named as "migrationConfigurations_DisableMigration" or something more self explanatory. No need to move it to a separate operation group on the client SDKs;.

changed it to 'migrationConfigurations_CompleteMigration' and changed the description. as this operation will complete migration process

hovsepm · 2018-04-13T20:17:18Z

@ravbhatnagar Could you check if it was approved by review board?

hovsepm · 2018-04-13T20:18:38Z

please add x-ms-examples to all the new operations.

hovsepm · 2018-04-13T20:20:07Z

...ification/servicebus/resource-manager/Microsoft.ServiceBus/stable/2017-04-01/servicebus.json

+          "migrationConfigurations"
+        ],
+        "operationId": "migrationConfigurations_Delete",
+        "description": "Deletes an MigrationConfig",


an [](start = 32, length = 2)

should be "Deletes a MigrationConfig"

hovsepm · 2018-04-13T20:28:41Z

...ification/servicebus/resource-manager/Microsoft.ServiceBus/stable/2017-04-01/servicebus.json

+          "properties": {
+            "provisioningState": {
+              "readOnly": true,
+              "type": "string",


provisioningState is a readonly field returned from server and may have additional values in the future. Modeling it as enums will cause client side runtime exceptions if there is a different value received.

made it read only string

anuchandy · 2018-04-17T01:06:11Z

...ification/servicebus/resource-manager/Microsoft.ServiceBus/stable/2017-04-01/servicebus.json

+      "in": "path",
+      "description": "The configuration name. Should always be \"$default\".",
+      "required": true,
+      "type": "string",


Please add "x-ms-parameter-location": "method" annotation

ravbhatnagar · 2018-04-19T06:03:10Z

Looks good!

…iptions

hovsepm · 2018-04-19T23:23:46Z

...ification/servicebus/resource-manager/Microsoft.ServiceBus/stable/2017-04-01/servicebus.json

+      }
+    },
+    "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.ServiceBus/namespaces/{namespaceName}/migrationConfigurations/{configName}": {
+      "put": {


Is this operation a LRO? IF so you need to mark it as "x-ms-long-running-operation": true. Otherwise the description is kind of confusing. If this operation initiates migration how does developer check the status of Migration? If it creates the configuration then method/operation Id is confusing.

yes its LRO, marked its as "x-ms-long-running-operation": true

hovsepm · 2018-04-19T23:26:15Z

...ification/servicebus/resource-manager/Microsoft.ServiceBus/stable/2017-04-01/servicebus.json

+      }
+    },
+    "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.ServiceBus/namespaces/{namespaceName}/migrationConfigurations/{configName}/upgrade": {
+      "post": {


are Migration upgrade and/or revert operations LRO?

no, those are not LRO

hovsepm · 2018-04-19T23:28:32Z

...ification/servicebus/resource-manager/Microsoft.ServiceBus/stable/2017-04-01/servicebus.json

+        "tags": [
+          "migrationConfigurations"
+        ],
+        "operationId": "migrationConfigurations_List",


migrationConfigurations [](start = 24, length = 23)

please think about consistency in the operation naming. Here you have migrationConfigurations while the other already existing operation group is named DisasterRecoveryConfigs

could you please help me on this?
for DisasterRecoveryConfigs operations its 'DisasterRecoveryConfigs' for Namespaces operations its 'Namespaces'.

the thing is that you have already shipped DisasterRecoveryConfigs (I mean shortened form of Configuration) that is why I advice keeping the consistency in names to shorten migrationConfigurations to MigrationConfigs

@hovsepm changed shortened to 'MigrationConfigs'. Also in URI, DR is 'disasterrecoveryconfigs' so have changed for migration to 'migrationconfigs'. please let me know the URI change is fine or need to revert it.

hovsepm

Please address review comments.

azuresdkciprbot · 2018-04-20T00:23:55Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

💡 Please review potentially introduced Error(s)/Warning(s): Analysis Report 💡

File: specification/servicebus/resource-manager/readme.md
Before the PR: Warning(s): 36 Error(s): 0
After the PR: Warning(s): 40 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

azuresdkciprbot · 2018-04-20T19:28:06Z

Hi There,

I am the AutoRest Linter Azure bot. I am here to help. My task is to analyze the situation from the AutoRest linter perspective. Please review the below analysis result:

💡 Please review potentially introduced Error(s)/Warning(s): Analysis Report 💡

File: specification/servicebus/resource-manager/readme.md
Before the PR: Warning(s): 36 Error(s): 0
After the PR: Warning(s): 40 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

hovsepm · 2018-04-20T21:44:02Z

...ification/servicebus/resource-manager/Microsoft.ServiceBus/stable/2017-04-01/servicebus.json

+          "MigrationConfigurationsStartMigration": { "$ref": "./examples/Migrationconfigurations/SBMigrationconfigurationCreateAndStartMigration.json" }
+        },
+        "description": "Creates Migration configuration and starts migration of enties from Standard to Premium namespace",
+        "parameters": [


Looks like a typo "enties"

hovsepm · 2018-04-20T21:44:56Z

...ification/servicebus/resource-manager/Microsoft.ServiceBus/stable/2017-04-01/servicebus.json

+        ],
+        "responses": {
+          "200": {
+            "description": "Delete Migration Config request accepted"


I think you should also handle 204: not found the same way as 200

Otherwise callers will get Exception in C# and Java which is undesired.

Ajit Maruti Navasare (MINDTREE LIMITED) added 2 commits April 3, 2018 09:59

SB Migration

3695b7c

review comments

1aca350

azuresdkci assigned hovsepm Apr 11, 2018

azuresdkci requested a review from hovsepm April 11, 2018 23:10

AutorestCI mentioned this pull request Apr 11, 2018

[AutoPR servicebus/resource-manager] Service Bus: added Standard to Premium namespace migration API [DO NOT MERGE] Azure/azure-sdk-for-python#2382

Closed

AutorestCI mentioned this pull request Apr 11, 2018

[AutoPR servicebus/resource-manager] Service Bus: added Standard to Premium namespace migration API [DO NOT MERGE] Azure/azure-sdk-for-node#2719

Merged

AutorestCI mentioned this pull request Apr 11, 2018

[AutoPR servicebus/resource-manager] Service Bus: added Standard to Premium namespace migration API [DO NOT MERGE] Azure/azure-sdk-for-go#1562

Closed

anuchandy reviewed Apr 12, 2018

View reviewed changes

hovsepm reviewed Apr 13, 2018

View reviewed changes

hovsepm added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Apr 13, 2018

hovsepm reviewed Apr 13, 2018

View reviewed changes

Review Comments

bde5ffc

AutorestCI mentioned this pull request Apr 16, 2018

[AutoPR servicebus/resource-manager] Service Bus: added Standard to Premium namespace migration API [DO NOT MERGE] Azure/azure-sdk-for-go#1615

Closed

anuchandy reviewed Apr 17, 2018

View reviewed changes

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 19, 2018

updated with 'x-ms-examples' and few changes to operationId and descr…

c41e136

…iptions

AutorestCI mentioned this pull request Apr 19, 2018

[AutoPR servicebus/resource-manager] Service Bus: added Standard to Premium namespace migration API [DO NOT MERGE] Azure/azure-sdk-for-go#1658

Merged

v-Ajnava changed the title ~~Service Bus: added Standard to Premium namespace migration API [DO NOT MERGE]~~ Service Bus: added Standard to Premium namespace migration API Apr 19, 2018

hovsepm reviewed Apr 19, 2018

View reviewed changes

hovsepm suggested changes Apr 19, 2018

View reviewed changes

review comments

fe023df

Review comments - MigrationConfigs

fbdcf5b

Review comments - CreateAndStartMigration and decription

0228c28

v-Ajnava force-pushed the masterSBMigrate branch from 5a154dd to 0228c28 Compare April 20, 2018 20:42

hovsepm reviewed Apr 20, 2018

View reviewed changes

typo corrected and added 204 to delete

fe062c9

v-Ajnava force-pushed the masterSBMigrate branch from e0db156 to fe062c9 Compare April 20, 2018 22:11

hovsepm approved these changes Apr 20, 2018

View reviewed changes

hovsepm merged commit d03281b into Azure:master Apr 20, 2018

AutorestCI mentioned this pull request Apr 20, 2018

[AutoPR servicebus/resource-manager] Service Bus: added Standard to Premium namespace migration API Azure/azure-sdk-for-go#1671

Merged

v-Ajnava mentioned this pull request May 5, 2018

ServiceBus: Standard to Premium Namespace Migration API and $skip and $top parameters for list calls Azure/azure-sdk-for-net#4288

Merged

9 tasks

Comments

Conversation

v-Ajnava commented Apr 11, 2018 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR information

Quality of Swagger

Uh oh!

AutorestCI commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-python

Uh oh!

AutorestCI commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-node

Uh oh!

AutorestCI commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-libraries-for-java

Uh oh!

AutorestCI commented Apr 11, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Automation for azure-sdk-for-go

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hovsepm commented Apr 13, 2018

Uh oh!

hovsepm commented Apr 13, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravbhatnagar commented Apr 19, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

v-Ajnava Apr 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hovsepm left a comment

Choose a reason for hiding this comment

Uh oh!

azuresdkciprbot commented Apr 20, 2018

Uh oh!

azuresdkciprbot commented Apr 20, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

v-Ajnava commented Apr 11, 2018 •

edited by azuresdkci

Loading

AutorestCI commented Apr 11, 2018 •

edited

Loading

AutorestCI commented Apr 11, 2018 •

edited

Loading

AutorestCI commented Apr 11, 2018 •

edited

Loading

AutorestCI commented Apr 11, 2018 •

edited

Loading

v-Ajnava Apr 20, 2018 •

edited

Loading