BotService: Support for channel management in bots #2887

carlosscastro · 2018-04-16T21:04:23Z

The goal of this pull request is mainly to introduce Channels, which are different means to expose a bot. Given that a bot resource has multiple channels, we have modeled the channels as a nested (and proxy only) resource of the bot.

We have multiple channels where bots can be exposed, and each has a different payload, but there is common information and machinery across channels as well.

This is why we modeled channels the following way

/botservice/{botid}/channel/{channelid}

Where channel id specifies the type of the channel, such as Facebook, Email or Skype. In the swagger , given that each channel has a specific payload, we create a base class channel and then inherited classes, making use of the "discriminator" feature of swagger.

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-16T21:04:55Z

Automation for azure-libraries-for-java

Nothing to generate for azure-libraries-for-java

AutorestCI · 2018-04-16T21:05:00Z

Automation for azure-sdk-for-node

Nothing to generate for azure-sdk-for-node

carlosscastro · 2018-04-16T21:05:04Z

@olydis I'm re-starting this PR FYI. I got delayed discussing the approach to secrets with Gaurav from ARM team.

AutorestCI · 2018-04-16T21:05:28Z

Automation for azure-sdk-for-python

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

AutorestCI · 2018-04-16T21:06:02Z

Automation for azure-sdk-for-go

Nothing to generate for azure-sdk-for-go

carlosscastro · 2018-04-16T21:18:05Z

Hello @marstr! This RP was under pull request but we were collecting feedback from Gaurav so we deactivated it while we worked on it. Here is the previous pull request: #2793. The status was WaitForARMFeedback, hope we can pick up from there.

I'm happy to answer any questions or have a quick call if you want to chat about the service :)

marstr · 2018-04-16T23:40:46Z

Thanks for the context, @carlosscastro! I'll begin reviewing this PR tomorrow morning.

ravbhatnagar · 2018-04-17T07:31:21Z

...ification/botservice/resource-manager/Microsoft.BotService/stable/2017-12-01/botservice.json

+        }
+      }
+    },
+    "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.BotService/botServices/{resourceName}/channels/{channelName}/list": {


@carlosscastro - can we call this action listSomething? listResgistrationWithKeys or listRegistrationWithSecrets?

Sure! listChannelWithKeys then.

ravbhatnagar · 2018-04-17T07:34:12Z

Confirmed with @carlosscastro that secrets wont be returned through GET/PUT response. A separate POST API has been added to list the resource with secrets. As discussed in the email thread, there was one issue with this but we deemed it would be ok and the RP is fine with this modeling. Signing off from ARM side. Just one note regarding the name of the action.

…r pull request, and also adding support for multiple pages in facebook channel

marstr

Some comments to get started with.

marstr · 2018-04-17T17:25:03Z

...ification/botservice/resource-manager/Microsoft.BotService/stable/2017-12-01/botservice.json

        ],
        "description": "Creates a Bot Service. Bot Service is a resource group wide resource type.",
-        "operationId": "BotServices_Create",
+        "operationId": "Bots_Create",


Changing the Operation Group that is associated with an Operation is a breaking change from the code gen perspective. For instance with the Go generator, each operation group has a client, and operations are dangled off of it.

Because this spec is in the stable folder, we don't accept breaking changes inside of an API Version without escalation.

@marstr This is a new API, so no breaking changes since it has never been published. @olydis merged it the state from azure-rest-api-specs-pr last week as a starting point and he created the "stable" folder.

Ah got it, let's move everything to a preview folder then. I'll notify all the SDK owners to make sure they aren't caught off-guard.

Got it, moving to a preview folder in the next batch of changes.

marstr · 2018-04-17T17:29:04Z

...ification/botservice/resource-manager/Microsoft.BotService/stable/2017-12-01/botservice.json

+            "description": "The name of the Bot resource."
+          },
+          {
+            "name": "channelName",


A couple more fields are required to make this conform to our x-ms-enum extension. You can read more about it here:
https://github.com/Azure/autorest/blob/master/docs/extensions/readme.md#x-ms-enum

marstr · 2018-04-17T17:30:51Z

...ification/botservice/resource-manager/Microsoft.BotService/stable/2017-12-01/botservice.json

+            "minLength": 2,
+            "pattern": "^[a-zA-Z0-9][a-zA-Z0-9_.-]*$",
+            "description": "The name of the Bot resource."
+          },


The above are usually defined as a global parameters with the "x-ms-parameter-location":"method" property set.

This isn't strictly necessary, but it should lead to easier maintenance in the future for you. :)

marstr · 2018-04-17T17:32:08Z

...ification/botservice/resource-manager/Microsoft.BotService/stable/2017-12-01/botservice.json

+            "description": "The name of the Bot resource."
+          },
+          {
+            "name": "channelName",


Given that this is used multiple times, I'd again recommend refactoring it into a global parameter that's referenced in each operation for consistency's sake.

Definitely, this will be great.

marstr · 2018-04-17T17:34:40Z

...ification/botservice/resource-manager/Microsoft.BotService/stable/2017-12-01/botservice.json

+            "description": "The name of the Bot resource."
+          },
+          {
+            "name": "channelName",


It is unclear to me if this is the same as the other "channelName" parameters. Is this supposed to be the same enum as is declared a few times above?

I'm centralizing this into a common parameter.

marstr · 2018-04-17T17:36:10Z

...ification/botservice/resource-manager/Microsoft.BotService/stable/2017-12-01/botservice.json

+            "description": "The name of the Bot resource."
+          },
+          {
+            "name": "channelName",


marstr · 2018-04-17T17:37:25Z

...ification/botservice/resource-manager/Microsoft.BotService/stable/2017-12-01/botservice.json

+      "description": "Indicates the name of a supported channel",
+      "enum": [
+        "FacebookChannel",
+        "EmailChannel"


Ah ah, so it is actually defined! Please extend this and reference it in all of the "channelName" declarations above.

marstr · 2018-04-17T17:43:38Z

...ification/botservice/resource-manager/Microsoft.BotService/stable/2017-12-01/botservice.json

+        "modelAsString": true
+      }
+    },
+    "ChannelName": {


[nit] This seems like a pretty confusing name to me. It may be that you can't change it for whatever reason, but to me it seems like this should be named "ChannelType". Shouldn't the "ChannelName" be the name of the specific channel? Like, the type of the channel may be "TelegramChannel" and the name could be something like "My Super Awesome Friend Group".

edit: grammar

marstr · 2018-04-17T17:45:43Z

Also @carlosscastro, could you fix the new errors reported here: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/367735550#L799 ? Some of them were introduced in this PR.

edit: grammar

marstr · 2018-04-17T17:53:02Z

Another thing @carlosscastro, as you mention:

Where channel id specifies the type of the channel, such as Facebook, Email or Skype. In the swagger , given that each channel has a specific payload, we create a base class channel and then inherited classes, making use of the "discriminator" feature of swagger.

As discriminator is defined in Swagger, it isn't quite sufficient for our code-gen. Take a look at our extension to see what we need to support discriminated types:
https://github.com/Azure/autorest/blob/master/docs/extensions/readme.md#x-ms-discriminator-value

edit: grammar

carlosscastro · 2018-04-17T18:02:44Z

@marstr Thanks for the thorough review, it is greatly appreciated! Will have an update incorporating your feedback within the next few hours!

…and updating readme.md to reflect the preview directory

azuresdkciprbot · 2018-04-17T20:05:46Z

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/botservice/resource-manager/readme.md
Before the PR: Warning(s): 3 Error(s): 0
After the PR: Warning(s): 45 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

carlosscastro · 2018-04-17T20:14:25Z

@marstr did a good pass on your suggestions and fixed a few more things. I did not change ChannelNAme to ChannelType, as in our system channel type has a different meaning (first party, 3rd party, extension) so it would be confusing for users.

marstr · 2018-04-18T15:42:40Z

I did not change ChannelNAme to ChannelType, as in our system channel type has a different meaning

No worries, I figured as much. Just figured I'd drop my two cents.

marstr · 2018-04-18T15:56:47Z

Quick question, I see this has ARM sign-off, but did it ever go through the API Review board?

marstr · 2018-04-18T16:14:26Z

Reading @azuresdkciprbot's comment, it looks like a bunch of RPC Violations were introduced in the most recent commit. (Or more likely, we fixed something that unblocked its ability to see the violations.) Almost all of them have to do with using booleans instead of something more descriptive.

Usually that's something to bothers the ARM team, but I see that you have @ravbhatnagar's approval. From an SDK point of view, it's not really a problem. If this has already been approved by the API Review Board, as I mention above, then I'm okay with it. If these definitions are not going to change, we should add suppressions to prevent noise in future reviews.

carlosscastro · 2018-04-18T16:16:24Z

Hi @marstr! We designed this change starting with a meeting with the OpenApi council, including ARM members and @fearthecowboy. They did recommend me this approach to channels and did a first review some time back.

I'd definitely open for them to take a last peek if that is the process :)

Regarding the booleans, yes, we discussed this some time back and we prefer to let them through. If in the future there might be more potential states, we'd do enums (we did many enums in this change where we deemed it necessary). But for all the booleans that we have, we know that they will stay binary.

Edit: answer question on booleans

marstr · 2018-04-18T16:20:42Z

...fication/botservice/resource-manager/Microsoft.BotService/preview/2017-12-01/botservice.json

+          "Channel"
+        ],
+        "description": "Lists a Channel registration for a Bot Service including secrets",
+        "operationId": "Channel_List",


This should be updated to "operationId": "Channels_ListWithKeys".

Updating "Channel" to "Channels" is to be consistent with the operations above. Without the change, our code generators would generate both a ChannelClient and a ChannelsClient.

Our RPC bot identified the omitted "WithKeys". The logic being, if it was important enough to convey the "WithKeys" in the path, it must be important enough to convey in the generated SDKs.

There's another RPC Violation noted by the bot:

"The child tracked resource, 'channels' with immediate parent 'Bot', must have a list by immediate parent operation."

Truthfully, I'm not familiar with this error. I believe it's complaining that there isn't an operation registered under the path "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.BotService/botServices/{resourceName}/channels/{channelName}/list"

That could reverse the conclusion of the comment above, and instead of changing the operationId to "Channels_ListWithKeys" you may update it to just "Channels_List", but change the path its registered.

Another option is that your service may needs both a "List" and a "ListWithKeys".

If none of those options make sense, we may be able to just add a suppression. Just let me know.

I will rename to Channels_ListWithKeys for consistency, good catch!

Regarding the violation, that operation does not make sense in our context which is why we chose not to expose it. I think a suppression would be fine here.

I will put the change for the operationId in 2 minutes :)

Great, I'll add the suppression also.

marstr · 2018-04-18T16:39:17Z

They did recommend me this approach to channels and did a first review some time back. ... Regarding the booleans, yes, we discussed this some time back and we prefer to let them through.

Sweet, this is is no problem then. We should add suppresions to the README file for all of the boolean usage warnings. :)

azuresdkciprbot · 2018-04-18T18:07:05Z

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:

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

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

carlosscastro · 2018-04-18T19:52:19Z

@marstr the latest changes are out. We are a bit in a time constrain while we get closer to //BUILD conference. Do we need more folks to review? If yes, can we include them today so we get this moving? Thanks for all your help :)

marstr · 2018-04-19T16:48:51Z

No other reviewers are necessary. :)

Are you ready for me to merge this?

carlosscastro · 2018-04-19T17:38:56Z

Awesome, thanks for your review @marstr! Actually - lets not merge for 2 hours, so I consult with my team members whether they have any changes being prepared (I think there are not, but want to confirm to simplify you work and avoid a new PR)

How is the process to move this to stable? Let it sit for 1 month or so?

carlosscastro · 2018-04-19T18:13:28Z

@marstr Confirmed - we are good to merge :) Thanks again for all your help!

marstr · 2018-04-19T19:41:53Z

How is the process to move this to stable? Let it sit for 1 month or so?

We can move it to stable whenever your team feels that's appropriate. :) However, breaking changes aren't allowed in the stable folder without it getting reported all the way up to scottgu. Given that this is a relatively new service, and that you were just making a few changes to the Swagger that would break clients, I wanted to have your back.

When you folks are comfortable saying no more breaking changes are coming inside of a given API Version, we can submit a new PR that just moves which folder its in. When that happens, a few languages that our teams own should recognize that it's now safe to start publishing without any "-beta" or similar caveats.

carlosscastro · 2018-04-20T18:36:57Z

@marstr seems like the right thing to do, let's keep it in preview until it feels stable, probably 1 month after //BUILD.

carlosscastro and others added 4 commits April 1, 2018 20:00

swagger update

90d96a5

update wrong tag

3deb7a0

Merge branch 'master' into master

7383f8e

adding secret post operation

299e1ed

azuresdkci assigned marstr Apr 16, 2018

azuresdkci requested a review from marstr April 16, 2018 21:04

AutorestCI mentioned this pull request Apr 16, 2018

[AutoPR botservice/resource-manager] BotService: Support for channel management in bots Azure/azure-sdk-for-python#2407

Merged

AutorestCI mentioned this pull request Apr 16, 2018

[AutoPR botservice/resource-manager] BotService: Support for channel management in bots Azure/azure-sdk-for-go#1610

Closed

ravbhatnagar reviewed Apr 17, 2018

View reviewed changes

ravbhatnagar added the ARMSignedOff <valid label in PR review process>add this label when ARM approve updates after review label Apr 17, 2018

Rename channel list operation with secrets as suggested in the Swagge…

8a4ec52

…r pull request, and also adding support for multiple pages in facebook channel

carlosscastro mentioned this pull request Apr 17, 2018

New SDK: Azure Bot Service Azure/azure-sdk-for-net#4184

Merged

9 tasks

marstr requested changes Apr 17, 2018

View reviewed changes

carlosscastro added 4 commits April 17, 2018 12:20

Move specification to preview folder

2eaeccc

Fixing oav validations for examples

c94942d

Incorporating feedback from Swagger PR

0d002f6

BotService: Adding x-ms-parameter-location to channel name parameter …

1108095

…and updating readme.md to reflect the preview directory

marstr requested changes Apr 18, 2018

View reviewed changes

suppress warnings and fix operation id for list channels with secrets

cd7f0d6

marstr approved these changes Apr 19, 2018

View reviewed changes

marstr merged commit 3b753f8 into Azure:master Apr 19, 2018

BotService: Support for channel management in bots #2887

BotService: Support for channel management in bots #2887

Uh oh!

Conversation

carlosscastro commented Apr 16, 2018 • edited 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 16, 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 16, 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!

carlosscastro commented Apr 16, 2018

Uh oh!

AutorestCI commented Apr 16, 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 16, 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!

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

Uh oh!

Uh oh!

marstr commented Apr 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravbhatnagar commented Apr 17, 2018

Uh oh!

marstr left a comment

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!

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!

marstr Apr 17, 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!

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

carlosscastro commented Apr 17, 2018

Uh oh!

azuresdkciprbot commented Apr 17, 2018

Uh oh!

carlosscastro commented Apr 17, 2018

carlosscastro commented Apr 16, 2018 •

edited

Loading

AutorestCI commented Apr 16, 2018 •

edited

Loading

AutorestCI commented Apr 16, 2018 •

edited

Loading

AutorestCI commented Apr 16, 2018 •

edited

Loading

AutorestCI commented Apr 16, 2018 •

edited

Loading

carlosscastro commented Apr 16, 2018 •

edited

Loading

marstr Apr 17, 2018 •

edited

Loading

marstr commented Apr 17, 2018 •

edited

Loading

marstr commented Apr 17, 2018 •

edited

Loading

carlosscastro commented Apr 18, 2018 •

edited

Loading

carlosscastro Apr 18, 2018 •

edited

Loading

carlosscastro commented Apr 19, 2018 •

edited

Loading