Add Bot Framework Enterprise Channel resources. #3577

NickEricson · 2018-08-03T16:06:57Z

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-08-03T16:07:38Z

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#3252

AutorestCI · 2018-08-03T16:07:44Z

Automation for azure-sdk-for-node

Nothing to generate for azure-sdk-for-node

AutorestCI · 2018-08-03T16:07:44Z

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

AutorestCI · 2018-08-03T16:08:08Z

Automation for azure-sdk-for-java

Nothing to generate for azure-sdk-for-java

azuresdkci · 2018-08-03T16:10:10Z

Can one of the admins verify this patch?

AutorestCI · 2018-08-03T16:12:54Z

Automation for azure-sdk-for-go

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

carlosscastro · 2018-08-03T16:52:31Z

...source-manager/Microsoft.BotService/preview/2018-07-12/examples/CreateEnterpriseChannel.json

+    "api-version": "2018-07-12",
+    "resourceName": "contoso-dl",
+    "parameters": {
+      "location": "West US",


So, do enterprise channels have a location? Bots and channels are global. Seems accurate since there will be an app service for them, but want to confirm.

Enterprise Channels have a location - there are resources associated with just the channel itself and they are deployed to this region.

carlosscastro · 2018-08-03T16:52:52Z

...source-manager/Microsoft.BotService/preview/2018-07-12/examples/CreateEnterpriseChannel.json

+      "properties": {
+        "nodes": [
+          {
+            "id": "00000000-0000-0000-0000-000000000000",


How do users know what to put here? I'm curious.

It has to be a guid. I could allow empty on create calls and generate one for them.

After the create they will have one of course - the get will return it.

carlosscastro · 2018-08-03T16:54:01Z

...fication/botservice/resource-manager/Microsoft.BotService/preview/2018-07-12/botservice.json

@@ -1012,6 +1012,283 @@
          "nextLinkName": "nextLink"
        }
      }
+    },
+    "/providers/Microsoft.BotService/checkECNameAvailability": {


why not full name? There are people that consume the API and readability of the paths is quite valuable. I'd root for checkEnterpriseChannelNameAvailability

Sounds good, changed.

carlosscastro · 2018-08-03T16:56:07Z

...fication/botservice/resource-manager/Microsoft.BotService/preview/2018-07-12/botservice.json

+      "properties": {
+        "id": {
+          "type": "string",
+          "description": "Id of Enterprise Channel Node"


Do users set this or do we assign it? Use mutability / readonly attributes to clarify which fields we generate

Sounds good. I set the node id to be readonly since it is generated and cannot be changed by the user.

carlosscastro · 2018-08-03T16:56:30Z

...fication/botservice/resource-manager/Microsoft.BotService/preview/2018-07-12/botservice.json

+          "type": "string",
+          "description": "The name of the Enterprise Channel Node"
+        },
+        "azureSku": {


Do we need to declare an enum or a list api for these? What are the options?

I can probably add this later. We don't have the options yet.

carlosscastro · 2018-08-03T16:56:57Z

...fication/botservice/resource-manager/Microsoft.BotService/preview/2018-07-12/botservice.json

+          "type": "string",
+          "description": "The sku of the Enterprise Channel Node"
+        },
+        "azureLocation": {


aren't location and sku part of the nested resource itself? do we need to repeat them?

These are different - nodes are deployed in their own locations. So there is an Enterprise Channel deployed in a location that has associated with it a set of nodes deployed in their own locations. They are a single entity - one cannot live without the other.

marstr · 2018-08-06T21:53:09Z

...fication/botservice/resource-manager/Microsoft.BotService/preview/2018-07-12/botservice.json

+          "Enterprise Channel"
+        ],
+        "description": "Check whether an Enterprise Channel name is available.",
+        "operationId": "EnterpriseChannels_GetCheckNameAvailability",


Please update the name of this operation group to be singular instead of plural.

Actually, when I try to change the name to EnterpriseChannel_GetCheckNameAvailability I get the following warning.

WARNING (OperationIdNounConflictingModelNames/R2063/SDKViolation): OperationId has a noun that conflicts with one of the model names in definitions section. The model name will be disambiguated to 'EnterpriseChannelModel'. Consider using the plural form of 'EnterpriseChannel' to avoid this. Note: If you have already shipped an SDK on top of this spec, fixing this warning may introduce a breaking change.
- file:///C:/src/azure-rest-api-specs/specification/botservice/resource-manager/Microsoft.BotService/preview/2018-07-12/botservice.json:1022:8 ($.paths["/providers/Microsoft.BotService/checkEnterpriseChannelNameAvailability"].get.operationId)

What do you suggest?

Ah got it, sorry please ignore my earlier suggestion.

@NickEricson
Regarding this operation - are you sure it should be a GET and not a POST?
According to this PR - maybe it should be POST:
#3781

Additionally - "GetCheckNameAvailability" is a bit of a mouthful - I would change this to just "CheckNameAvailability"

yes, good catch.

marstr · 2018-08-06T21:56:34Z

...fication/botservice/resource-manager/Microsoft.BotService/preview/2018-07-12/botservice.json

+            "schema": {
+              "$ref": "#/definitions/EnterpriseChannel"
+            },
+            "description": "The parameters to provide to update the Enterprise Channel."


This will need x-ms-parameter-location.

Add it to the body parameter? Docs mention that
"This extension can only be applied on global parameters. If this is applied on any parameter in an operation then it will be ignored."

resourceGroupNameParameter and resourceNameParameter have x-ms-parameter-location as method already.

Whoops, sorry ignore this comment as well.

marstr · 2018-08-06T21:57:33Z

...fication/botservice/resource-manager/Microsoft.BotService/preview/2018-07-12/botservice.json

+            "schema": {
+              "$ref": "#/definitions/EnterpriseChannel"
+            },
+            "description": "The parameters to provide for the new Enterprise Channel."


Same as here: https://github.com/Azure/azure-rest-api-specs/pull/3577/files#diff-69a7c573ec058eabc65e37eeba8cb700R1182

marstr · 2018-08-06T21:58:47Z

...fication/botservice/resource-manager/Microsoft.BotService/preview/2018-07-12/botservice.json

+            }
+          },
+          "201": {
+            "description": "If resource is created successfully, the service should return 201 (Created). Execution to continue asynchronously.",


Does this mean that you're conforming to LRO semantics? https://github.com/Azure/autorest/tree/master/docs/extensions#x-ms-long-running-operation

We will do that. I think I need to add a route to handle the LRO responses.

I added these:

"x-ms-long-running-operation": true,
"x-ms-long-running-operation-options" : {
"final-state-via": "azure-async-operation"
},

Two questions:

We are using the Azure RP SDK (https://msazure.visualstudio.com/One/_git/AzureUX-ResourceProviderSDK) to build the SDK. Is this the correct "final-state-via" to use when we return ResourceOperationStatus.CompleteAsynchronously from the IManagedGroupWideResourceTypeHandler instance?

Do we need to define the route we are using for returning the Operation Result status when Azure is polling us for completion? We are using the DefaultOperationResultsResourceTypeHandler which seems to hit the route operationresults/{operationResultName}. Looking at some of the current swaggers I do not see that route in them (ie Disk.json).

@shahabhijeet, do you have any insight to offer to @NickEricson on Long Running Operations implementation?

@marstr @shahabhijeet Is there anything else we need to do here?

ravbhatnagar

couple of notes.

ravbhatnagar · 2018-08-15T07:20:40Z

...fication/botservice/resource-manager/Microsoft.BotService/preview/2018-07-12/botservice.json

+    },
+    "EnterpriseChannelProperties": {
+      "properties": {
+        "state": {


please make it an enum

Do enums allow null?

Looks like I can do Enum as string like ProvisioningState

ravbhatnagar · 2018-08-15T07:21:43Z

...fication/botservice/resource-manager/Microsoft.BotService/preview/2018-07-12/botservice.json

+          "type": "string",
+          "description": "The name of the Enterprise Channel Node"
+        },
+        "azureSku": {


how is this different from the ARM top level sku property?

You can add nodes to a channel which can have different performance characteristics. Customers will be charged different amount based on the channel sku + sum of the node sku.

Nodes cannot exist without a channel and channels can't exist without a node.

…specs into bfec

marstr · 2018-08-24T20:53:22Z

I'm going to be OOF all next week, so I'm marking this PR for reassignment.

ravbhatnagar · 2018-08-28T00:00:03Z

Signing off from ARm side.

annatisch

Just some minor things.

Please be consistent with your descriptions for capitalization, adding periods to the end of sentences, capitalizing abbreviations ("ID") etc - as these descriptions will be used to generate the reference docs for both the API and all the client SDKs :)

annatisch · 2018-08-28T18:07:04Z

...fication/botservice/resource-manager/Microsoft.BotService/preview/2018-07-12/botservice.json

@@ -2127,6 +2416,122 @@
          "type": "string"
        }
      }
+    },
+    "EnterpriseChannelCheckNameAvailabilityRequestBody": {


We try to avoid REST API terminology in the naming of models - as it is confusing in the client-side user experience - can you please rename this definition? Even if you just remove "Body" from the end, e.g.:
EnterpriseChannelCheckNameAvailabilityRequest

annatisch · 2018-08-28T18:07:19Z

...fication/botservice/resource-manager/Microsoft.BotService/preview/2018-07-12/botservice.json

+        }
+      }
+    },
+    "EnterpriseChannelCheckNameAvailabilityResponseBody": {


We try to avoid REST API terminology in the naming of models - as it is confusing in the client-side user experience - can you please rename this definition? Even if you just remove "Body" from the end, e.g.:
EnterpriseChannelCheckNameAvailabilityResponse

annatisch · 2018-08-28T18:08:13Z

...fication/botservice/resource-manager/Microsoft.BotService/preview/2018-07-12/botservice.json

+      "type": "object",
+      "properties": {
+        "name": {
+          "description": "the name of the Enterprise Channel for which availability needs to be checked.",


Minor grammar: capital 'T'

annatisch · 2018-08-28T18:09:15Z

...fication/botservice/resource-manager/Microsoft.BotService/preview/2018-07-12/botservice.json

@@ -2127,6 +2416,122 @@
          "type": "string"
        }
      }
+    },
+    "EnterpriseChannelCheckNameAvailabilityRequestBody": {
+      "description": "The request body for a request to Bot Service Management to check availability of an Enterprise Channel name.",


Please avoid REST API terminology in the parameter descriptions:
e.g. "A request to Bot Service Management to check availability of....."

annatisch · 2018-08-28T18:11:08Z

...fication/botservice/resource-manager/Microsoft.BotService/preview/2018-07-12/botservice.json

+      }
+    },
+    "EnterpriseChannelCheckNameAvailabilityResponseBody": {
+      "description": "The response body returned for a request to Bot Service Management to check availability of an Enterprise Channel name.",


Please avoid REST API terminology in the parameter descriptions:
e.g. "The name availability response from Bot Service Management for an Enterprise Channel....."

annatisch · 2018-08-28T18:11:59Z

...fication/botservice/resource-manager/Microsoft.BotService/preview/2018-07-12/botservice.json

+      "type": "object",
+      "properties": {
+        "valid": {
+          "description": "indicates if the Enterprise Channel name is valid.",


Minor grammar: capital 'I'

annatisch · 2018-08-28T18:12:15Z

...fication/botservice/resource-manager/Microsoft.BotService/preview/2018-07-12/botservice.json

+          "type": "boolean"
+        },
+        "message": {
+          "description": "additional message from the bot management api showing why a bot name is not available",


Minor grammar: capital 'A' and add period '.'

Also, in other places you have capitalized "Bot Service Management" - I would probably just simplify the description, e.g.:
"Additional information about why a bot name is not available."

annatisch · 2018-08-28T18:18:18Z

...fication/botservice/resource-manager/Microsoft.BotService/preview/2018-07-12/botservice.json

+          "description": "Id of Enterprise Channel Node. This is generated by the Bot Framework.",
+          "readOnly": true
+        },
+        "state": {


I see that EnterpriseChannelNode and EnterpriseChannelProperties both have a "state" parameter defined by an enum. Is the intention that these enums are the same? Currently, they both have the name "state", so when generated to a client - they are both referencing the same enum model. Does this sound correct to you? (i.e. both models will use exactly the same values for this parameter).

We do not want them to reference the same model - they are different enums. We want the properties to have the name state in both of them. Do I change the x-ms-enum to something else ie:

"state": {
"type": "string",
"description": "The current state of the Enterprise Channel Node.",
"enum": ["Creating", "CreateFailed", "Started", "Starting", "StartFailed", "Stopped", "Stopping", "StopFailed", "Deleting", "DeleteFailed"],
"x-ms-enum": {
"name": "nodeState", <-------------- change this.
"modelAsString": true
}
},

That seemed right so I did that. Let me know if I went the wrong way

Yes that's correct - changing "nodeState" to something specific should fix it :)

Changed it to EnterpriseChannelNodeState

…into bfec

annatisch · 2018-09-04T21:28:22Z

...fication/botservice/resource-manager/Microsoft.BotService/preview/2018-07-12/botservice.json

+            "name": "parameters",
+            "in": "body",
+            "required": true,
+            "x-ms-client-flatten": true,


Hi @NickEricson - last question:
Are you sure you wish to flatten out the body parameter here? It's not wrong - but does result in rather a large generated method signature:
https://github.com/Azure/azure-sdk-for-python/pull/3151/files#diff-c30e6c4ddd2d3e8f68fd313d4757fe05R327

We are using it on our other objects as well and would like to be consistent.

Anna's is sufficient!

Add Bot Framework Enterprise Channel resources.

f6a25fc

azuresdkci assigned marstr Aug 3, 2018

azuresdkci requested a review from marstr August 3, 2018 16:07

AutorestCI mentioned this pull request Aug 3, 2018

[AutoPR botservice/resource-manager] Add Bot Framework Enterprise Channel resources. Azure/azure-sdk-for-python#3075

Closed

AutorestCI mentioned this pull request Aug 3, 2018

[AutoPR botservice/resource-manager] Add Bot Framework Enterprise Channel resources. Azure/azure-sdk-for-go#2390

Merged

carlosscastro reviewed Aug 3, 2018

View reviewed changes

Use full name for check availability method.

43451db

AutorestCI mentioned this pull request Aug 3, 2018

[AutoPR botservice/resource-manager] Add Bot Framework Enterprise Channel resources. Azure/azure-sdk-for-python#3076

Closed

Change node id to be readonly.

0d331dc

lmazuel mentioned this pull request Aug 6, 2018

updated pythonsdk for new swagger api-version for botservice Azure/azure-sdk-for-python#3073

Merged

Update readme.python.md

fd2f913

marstr previously requested changes Aug 6, 2018

View reviewed changes

marstr added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label Aug 6, 2018

marstr requested a review from ravbhatnagar August 6, 2018 21:59

ravbhatnagar reviewed Aug 15, 2018

View reviewed changes

NickEricson added 2 commits August 15, 2018 08:42

Add async operation nodes.

e2fe651

Merge branch 'bfec' of https://github.com/NickEricson/azure-rest-api-…

242277f

…specs into bfec

AutorestCI mentioned this pull request Aug 15, 2018

[AutoPR botservice/resource-manager] Add Bot Framework Enterprise Channel resources. Azure/azure-sdk-for-python#3151

Merged

Make state an Enum.

e4a6e85

marstr added the Reassign label Aug 24, 2018

azuresdkci assigned annatisch and unassigned marstr Aug 24, 2018

azuresdkci requested a review from annatisch August 24, 2018 21:25

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 Aug 28, 2018

annatisch requested changes Aug 28, 2018

View reviewed changes

NickEricson added 6 commits August 31, 2018 13:17

Fix some descriptions.

825f509

Change enum on channel and node so they don't create the same enum.

4e4ecaf

Using full name for enum instead of shorter name.

4bfd870

Update to post

36ad9c0

Merge branch 'master' of https://github.com/Azure/azure-rest-api-specs …

a2771a7

…into bfec

Update py package-version to 0.3.0

7ade32d

annatisch reviewed Sep 4, 2018

View reviewed changes

annatisch approved these changes Sep 5, 2018

View reviewed changes

annatisch merged commit 33a02b6 into Azure:master Sep 5, 2018

Add Bot Framework Enterprise Channel resources. #3577

Add Bot Framework Enterprise Channel resources. #3577

Conversation

NickEricson commented Aug 3, 2018 • edited Loading

PR information

Quality of Swagger

AutorestCI commented Aug 3, 2018 • edited Loading

Automation for azure-sdk-for-python

AutorestCI commented Aug 3, 2018 • edited Loading

Automation for azure-sdk-for-node

AutorestCI commented Aug 3, 2018 • edited Loading

Automation for azure-sdk-for-ruby

AutorestCI commented Aug 3, 2018 • edited Loading

Automation for azure-sdk-for-java

azuresdkci commented Aug 3, 2018

AutorestCI commented Aug 3, 2018 • edited Loading

Automation for azure-sdk-for-go

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ravbhatnagar left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marstr commented Aug 24, 2018

ravbhatnagar commented Aug 28, 2018

annatisch left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

NickEricson commented Aug 3, 2018 •

edited

Loading

AutorestCI commented Aug 3, 2018 •

edited

Loading

AutorestCI commented Aug 3, 2018 •

edited

Loading

AutorestCI commented Aug 3, 2018 •

edited

Loading

AutorestCI commented Aug 3, 2018 •

edited

Loading

AutorestCI commented Aug 3, 2018 •

edited

Loading