Update botservice swagger to perform crud operations on connection settings #3071

swagatmishra2007 · 2018-05-14T23:21:17Z

This changelist updates the swagger to be able to add/remote/update conneciton settings in botservice.
connection settings (called "connections" in the swagger) are a new feature in botservice related to authentication. Using this, a bot developer can select a service provider (like pinterest, aad, github etc) and then add a connection setting pertaining to this service. This lets his bot authenticate a user to a service provider and fetch a token on his behalf. Eg: you can add a github connection as a bot developer.Now a user of your bot can authenticate into github using your bot and access his git repositories.

azuresdkciprbot · 2018-05-14T23:25:34Z

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/botservice/resource-manager/readme.md

⚠️

0 new Warnings.(0 total)

❌0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

azuresdkciprbot · 2018-05-14T23:25:34Z

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/botservice/resource-manager/readme.md

⚠️

0 new Warnings.(0 total)

❌0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

Jeffders · 2018-05-15T03:47:32Z

...esource-manager/Microsoft.BotService/preview/2017-12-01/examples/ListTokensByBotService.json

@@ -0,0 +1,36 @@
+{
+    "parameters": {


The file name says ListTokensByBotService...shouldn't it be ListConnectionsByBotService?

ravbhatnagar

Please take a look at the comments.

ravbhatnagar · 2018-05-15T07:26:45Z

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

+          "type": "string",
+          "description": "Scopes associated with the Connection Setting"
+        },
+        "id": {


top level properties should not be repeated inside the properties envelope. Id is a top level property which implies the resource id for the connectionSetting resource. If this is the same id, you can remove it from here.

ravbhatnagar · 2018-05-15T07:26:57Z

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

+          "type": "string",
+          "description": "Client Secret associated with the Connection Setting"
+        },
+        "scopes": {


what can the scopes be?

scopes are serviceprovider dependent and are comma-separated or whitespace separated.
for example, an example scope string for aad is "email.read email.write profile.read" the specific service provider logic knows how to handle this.

ravbhatnagar · 2018-05-15T07:27:19Z

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

+          "type": "string",
+          "description": "Client Id associated with the Connection Setting."
+        },
+        "clientSecret": {


should not returned in the response of GET, PUT, PATCH. just confirming.

Yes. Also, i added a new listwithsecrets path, similar to how we have channel/listwithkeys method. On doing a post to this url, the caller can get a particular connectionname with the secrets present.
Otherwise, the clientsecret will not be returned in a normal get,put, patch

ravbhatnagar · 2018-05-15T07:27:31Z

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

+    },
+    "ConnectionSettingProperties": {
+      "properties": {
+        "clientId": {


how do customers get this?

this is service provider specific and will be covered in the documentation.

ravbhatnagar · 2018-05-15T07:28:04Z

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

+          "type": "string",
+          "description": "Id associated with the Connection Setting"
+        },
+        "name": {


this should be a top level property, which comes when you do a allOf on a "resource"

ravbhatnagar · 2018-05-15T07:29:59Z

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

+          "type": "string",
+          "description": "Service Provider Id associated with the Connection Setting"
+        },
+        "serviceProviderDisplayName": {


Why do you need both serviceProviderId and serviceProviderDisplayname to be provided? serviceProviderId should be sufficient.

The serviceproviderdisplayname is needed by the botservice azure portal blade to display the service providers available as a dropdown, when they add a connection. it makes better ui experience to have the display name than to have just the id.

ravbhatnagar · 2018-05-15T07:31:13Z

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

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


Why is this an action on the botService resource instead of an action on the provider itself? Are providers specific to an instance of the botService resource, likely not?

made this an action on the botservice provider itself, since it's not specific to an instance of the resource

marstr · 2018-05-15T23:34:06Z

Has this service gone through the API Review board? Given @ravbhatnagar's comments, it seems like it has not.

AutorestCI · 2018-05-16T20:07:38Z

Automation for azure-libraries-for-java

Nothing to generate for azure-libraries-for-java

AutorestCI · 2018-05-16T20:07:38Z

Automation for azure-sdk-for-node

Nothing to generate for azure-sdk-for-node

AutorestCI · 2018-05-16T20:08:15Z

Automation for azure-sdk-for-python

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

AutorestCI · 2018-05-16T20:09:12Z

Automation for azure-sdk-for-go

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

ravbhatnagar · 2018-05-18T07:24:12Z

Looks good.

swagatmishra2007 · 2018-05-18T18:11:24Z

@marstr we have the arm review signoff now. Can you please take a look and merge if it looks good?

marstr · 2018-05-22T17:36:06Z

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

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


With the defintions like resourceName, resourceGroupName, and connectionName which seem to be repeated, I'd move them to the "definitions"/"parameters" section in order to make this Swagger more maintainable moving forward.

added these to be parameter definitions which are now referenced through out the swagger

marstr · 2018-05-22T17:42:27Z

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

+        "operationId": "BotConnection_ListWithSecrets",
+        "parameters": [
+          {
+            "name": "resourceGroupName",


An empty resourceGroupName being sent over the wire can result in really obfuscated errors being returned to the client. Please add some validation to the definition of resourceGroupName to prevent this.

added validation similar to resource name

marstr · 2018-05-22T17:44:14Z

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

+          "ListServiceProviders"
+        ],
+        "description": "Lists the available Service Providers for creating Connection Settings",
+        "operationId": "BotConnections_ListServiceProviders",


It looks like there are two Operation Group names present here: "BotConnections" and "BotConnection" this will result in two separate client types being generated. Is this the intended behavior?

I found another one "BotToken", should that be dangled off of a separate client?

removed all "connections" and token references and updated them to be connection to keep things consistent

marstr · 2018-05-22T17:45:04Z

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

+          "Connections"
+        ],
+        "description": "Returns all the Connection Settings registered to a particular BotService resource",
+        "operationId": "Connections_ListByBotService",


Is this supposed to be `BotConnections_ListByBotService"?

yes. changed it to botconnections

changed it to botconnection_listbybotservice to keep it consistent.

AutorestCI · 2018-05-23T20:04:57Z

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

marstr

Looking a lot better!

marstr · 2018-05-23T20:27:37Z

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

    }
  },
  "parameters": {
+    "resourceGroupNameParameter": {


resourceGroupNameParameter and resourceNameParameter are missing the x-ms-parameter-location property, which can have some sorta gnarly code-gen repercussions.

I'd add "x-ms-parameter-location":"method", to both parameter definitions.

…finitions

marstr · 2018-05-23T22:44:38Z

Ready for me to merge, @swagatmishra2007?

swagatmishra2007 · 2018-05-23T23:16:27Z

@marstr thanks for the review! all done from my end. Please merge :)

Swagat Mishra added 5 commits May 14, 2018 16:13

intial commit for token api to botservice swagger

ae31104

add descriptions and examples

1c899ef

add examples

816db7e

final changes

2960a28

fix examples

2021f8a

azuresdkci assigned marstr May 14, 2018

azuresdkci requested a review from marstr May 14, 2018 23:21

Jeffders reviewed May 15, 2018

View reviewed changes

ravbhatnagar reviewed May 15, 2018

View reviewed changes

ravbhatnagar added the WaitForARMFeedback <valid label in PR review process> add this label when ARM review is required label May 15, 2018

addressed comments

19bc3ac

AutorestCI mentioned this pull request May 16, 2018

[AutoPR botservice/resource-manager] Update botservice swagger to perform crud operations on connection settings Azure/azure-sdk-for-python#2573

Merged

AutorestCI mentioned this pull request May 16, 2018

[AutoPR botservice/resource-manager] Update botservice swagger to perform crud operations on connection settings Azure/azure-sdk-for-go#1847

Merged

fix duplicate operationid

82c95c7

ravbhatnagar approved these changes May 18, 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 May 18, 2018

marstr requested changes May 22, 2018

View reviewed changes

fix comments

ee9ccc1

fixed validation errors

74ff143

marstr requested changes May 23, 2018

View reviewed changes

add ms parameter location to resource group and resource parameter de…

baa318a

…finitions

marstr approved these changes May 23, 2018

View reviewed changes

marstr merged commit daa6778 into Azure:master May 23, 2018

Update botservice swagger to perform crud operations on connection settings #3071

Update botservice swagger to perform crud operations on connection settings #3071

Uh oh!

Conversation

swagatmishra2007 commented May 14, 2018 • edited by azuresdkci Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

azuresdkciprbot commented May 14, 2018

AutoRest linter results for ARM Related Validation Errors/Warnings

Uh oh!

azuresdkciprbot commented May 14, 2018

AutoRest linter results for SDK Related Validation Errors/Warnings

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ravbhatnagar 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!

Choose a reason for hiding this comment

Uh oh!

marstr commented May 15, 2018

Uh oh!

AutorestCI commented May 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 May 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!

AutorestCI commented May 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 May 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!

ravbhatnagar commented May 18, 2018

Uh oh!

swagatmishra2007 commented May 18, 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!

swagatmishra2007 commented May 14, 2018 •

edited by azuresdkci

Loading

AutorestCI commented May 16, 2018 •

edited

Loading

AutorestCI commented May 16, 2018 •

edited

Loading

AutorestCI commented May 16, 2018 •

edited

Loading

AutorestCI commented May 16, 2018 •

edited

Loading

AutorestCI commented May 23, 2018 •

edited

Loading