[Azure Customer Insight] Updated the 2017-01-01 version with the latest interaction API. Added 2017-04-26 version #1343

oucfei · 2017-06-23T17:59:05Z

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.

msftclas · 2017-06-23T17:59:09Z

@oucfei,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

veronicagg

Please review model validation errors at https://travis-ci.org/Azure/azure-rest-api-specs/jobs/246332416#L1358
Operations API is missing from customer-insights spec: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/246332414#L1364
Should 'location' be removed from parameters of PATCH operation https://travis-ci.org/Azure/azure-rest-api-specs/jobs/246332414#L1416 ? Please note that this may cause a breaking change in the SDKs, and therefore a change may not want to be done in the existing api-version but in the new one.
Please review same errors in 2017-04-26 of the spec: https://travis-ci.org/Azure/azure-rest-api-specs/jobs/246332414#L1812
Please review warnings from linter validation as well https://travis-ci.org/Azure/azure-rest-api-specs/jobs/246332414#L1428 for both specs.
For the new api-version, it'd be great if you could have an initial commit of the old version and a newer commit with the changes (see https://github.com/Azure/azure-rest-api-specs/blob/master/.github/CONTRIBUTING.md#filenames-and-folder-structure)

veronicagg · 2017-06-23T20:44:17Z

arm-customer-insights/2017-01-01/swagger/customer-insights.json

          "type": "string",
          "description": "The primary participant property name for an interaction ,This is used to logically represent the agent of the interaction, Specify the participant name here from ParticipantName."
        },
-        "dataSources": {


removing/renaming a property is a breaking change (has the service made a breaking change?) and may result in SDK breaking changes, is this intentional for this api-version?
similar comment applies to other changes made to this api-version, please provide additional context on the changes.

Hi, this is intentional and it's OK because the 2017-01-01 version never get released before. Last time when we update this swagger, this part was a work-in-progress. Now that it's done and it should look like this (when you call this API with 2017-01-01 version, this is what the returned schema looks like)

If the current spec does not match the wire, meaning that it was initially incorrect, then it may be ok corrected (though for anyone who shipped it avoiding a breaking change is best). Regarding your statement about "201-01-01 version never get released before", what do you mean by this?
Having the swagger spec in master of this repo, means anyone could generate an SDK, I see there's an SDK for Go at https://github.com/Azure/azure-sdk-for-go/tree/master/arm/customer-insights.

hi sorry looks like it's released before. However this part was incomplete so I'm sure no one was using it. Anyway the version 2017-01-01 should look like this swagger now

veronicagg · 2017-06-23T21:09:20Z

arm-customer-insights/2017-04-26/swagger/customer-insights.json

+          {
+            "name": "kpiName",
+            "in": "path",
+            "requir


is "modelAsString: false" intentional? This will make the type an enum, see https://github.com/Azure/autorest/blob/master/docs/extensions/readme.md#x-ms-enum

hi, sorry it's unclear which line you're referring to. the "modelAsString: false" appear many times in the swagger. But regardless, those are copied from the old api-version, and no changes in the new api version. So it's already there before this PR. Same for the other places.

I'm referring to targetEntityType, sourceEntityType which are new properties in this spec.

This is intentional. They're indeed enum type

veronicagg · 2017-06-23T21:10:33Z

arm-customer-insights/2017-04-26/swagger/customer-insights.json

+          {
+            "name": "kpiName",
+            "in": "path",
+            "requir


similar question as previous is "modelAsString: false" intentional?

same as above

veronicagg · 2017-06-23T21:11:39Z

arm-customer-insights/2017-04-26/swagger/customer-insights.json

+          {
+            "name": "kpiName",
+            "in": "path",
+            "requir


should this description be different from the one below and more descriptive?

sorry which line is this? It doesn't show correctly above

descriptions of sourceEntityType and targetEntityType are the same.

veronicagg · 2017-06-23T21:14:27Z

@ravbhatnagar there's a new api-version being added with some small changes, will you be looking into this review from ARM side?

ravbhatnagar · 2017-06-24T00:48:18Z

@veronicagg - yes I would like to. Since its a new file it has to be reviewed using notepad++ or something similar. Any recommendations? Can a PR be sent which compares it against the older api version so that only what changed can be reviewed.

oucfei · 2017-06-24T18:50:33Z

@veronicagg thanks for review the PR.

The model validation error has been fixed.
Operations API is not implemented yet in customer insight. We have a backlog item for that.
'Location' can not be removed from the PATCH operation because in the code we're explicitly checking it and throws FORBIDDEN if it changed.
I understand it would be easier to review if I had an initial commit for the new api version. Sorry about that. But I can't modify the commits now, right? Maybe you should use some code compare tool to see what changed.

Question:

Do we have to fix all the warnings from linter validation? Those warnings are already there before this PR.
Why is the Travis CI build failed? I don't see any error message related with the swagger
Thanks again!

veronicagg · 2017-06-26T21:14:00Z

@ravbhatnagar I've been using VS Code, and comparing with the old version, see http://dailydotnettips.com/2015/06/04/how-to-compare-files-in-visual-studio-code/
We do want the old version of the spec in the first commit and then commits on top of that to make it easier for review, and I asked the author about it, see above. In the meantime, you can use VS Code.

veronicagg · 2017-06-26T21:36:10Z

@oucfei I've replied to your questions in the inline comments, please review.
Regarding your points above:

Thanks, please make sure the examples are matching the wire format
Ok, be aware that until fixed, it'll get reported as a violation. @ravbhatnagar - FYI this is an RPC violation.
'Location' is required in the body parameter of the PATCH operation at the moment, so are you requiring users of this API to send the property with the same value as it was originally set? @ravbhatnagar RPC violdation https://travis-ci.org/Azure/azure-rest-api-specs/jobs/246332414#L1416
We prefer it this way yes. You could change your previous commits, yes, though if there are review comments they will get messed up, so you can do it after the current comments, but I wouldn't do it after @ravbhatnagar reviews the spec, or they'll get kind of lost.

RE: Questions:

Linter validation: Since you're introducing a new api-version, it's a good time to review all violations. Note that "errors" will be reported for the spec, "warnings" are recommendations, you should evaluate the ones that make sense to you.
Travis CI - there's an issue with one of the tools, I've reported it and the owner is working on it, it shouldn't be due to this PR, we expect it'll be resolved soon.

oucfei · 2017-06-26T22:55:50Z

@veronicagg thanks for your reply

I have ran "oav validation-example" and no error found.
OK.
I looked at the code again, turned out the location is not required. I have removed from the required properties in the swagger.
OK I will not modify my previous commits for now.

I also looked at the warnings from linter validation and they don't make sense to me, so I will not fix those.

veronicagg · 2017-06-26T23:32:35Z

@oucfei thanks. Regarding 1. ok, the tool will compare the examples with the spec, so if the wire format matches the example, then the spec should more accurately represent the wire format.
3. Ok, I'll let @ravbhatnagar comment on it, since a "Resource" becomes a "Proxy resource" if location is not required, and I'd like to make sure from ARM perspective that's appropriate for your spec.

@ravbhatnagar please review and provide your comments/approval, on behalf of ARM team.

ravbhatnagar

Please take a look at the comments I have added. Majority of those are around existing APIs.
I see in a lot of resource types you have "name" top level property and also a name property inside the properties bag. It is redundant information.

ravbhatnagar · 2017-06-28T19:57:07Z

arm-customer-insights/2017-04-26/swagger/customer-insights.json

+              "$ref": "#/definitions/ProfileResourceFormat"
+            }
+          },
+          "202": {


Any specific reason why you use the 202 + location header async pattern here but for PUT /hubs you use the 201 + provisioningState. 201 pattern is recommended by ARM for PUT/PATCH as it enables clients provide richer experience when tracking long running operation and provide a reliable way to retrieve and show error messages when it fails.

hi, I don't know any specific reason except that this is how the API was designed and implemented. At this point we're not planning to make change to this API.

ravbhatnagar · 2017-06-28T20:06:15Z

arm-customer-insights/2017-04-26/swagger/customer-insights.json

+              "$ref": "#/definitions/InteractionResourceFormat"
+            }
+          },
+          "202": {


Same comment about long running operation as above for LRO (201?)

same as above.

ravbhatnagar · 2017-06-28T20:12:16Z

arm-customer-insights/2017-04-26/swagger/customer-insights.json

+          {
+            "name": "kpiName",
+            "in": "path",
+            "requir


Existing API - but add the allowed values for this as you have done for other resources.

which line is this?

ravbhatnagar · 2017-06-28T20:19:29Z

arm-customer-insights/2017-04-26/swagger/customer-insights.json

+          {
+            "name": "kpiName",
+            "in": "path",
+            "requir


Just to make sure - secrets like keys should not be returned in the response of PUT or GET. It could be confusing to users of SDK if they see/expect these in response.

which line is this?

ravbhatnagar · 2017-06-28T21:19:03Z

arm-customer-insights/2017-04-26/swagger/customer-insights.json

+        }
+      }
+    },
+    "/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.CustomerInsights/hubs/{hubName}/authorizationPolicies/{authorizationPolicyName}/regeneratePrimaryKey": {


Existing API - but do you really need two separate APIs for regenerating primary and secondary keys? I think one API would have sufficed with the request body indicating which key to regenerate.

I agree it should be sufficed with just one API, but again this is how the API was designed and implemented, and I don't know why.

Ok. consider changing this in the next API-version revision to make the API cleaner.

ravbhatnagar · 2017-06-28T21:24:20Z

arm-customer-insights/2017-04-26/swagger/customer-insights.json

+          {
+            "name": "kpiName",
+            "in": "path",
+            "requir


Seems like you could have leveraged the "provisioningState" property for this. Is it because of the "expiring" state that you added the state property instead of using the provisioningState. All other states seems to be related to provisioning

which line is this?

ravbhatnagar · 2017-06-28T21:25:23Z

arm-customer-insights/2017-04-26/swagger/customer-insights.json

+          {
+            "name": "kpiName",
+            "in": "path",
+            "requir


Existing API - but why do you need tenantId? Can the hub be in a different tenant?

in our API the "tenantId" is actually the "hubId"... we have a bad name and it is confusing...

ravbhatnagar · 2017-06-28T22:19:08Z

arm-customer-insights/2017-04-26/swagger/customer-insights.json

+          {
+            "name": "kpiName",
+            "in": "path",
+            "requir


Existing API but still wanted to note that resource types should be pluralized - kpi

not sure which line you're referring to

Not sure why it doesnt show up there. But likely you wont be able to change it now. I am referring to the resource type "kpi"

ravbhatnagar · 2017-06-28T22:21:15Z

arm-customer-insights/2017-04-26/swagger/customer-insights.json

+          {
+            "name": "kpiName",
+            "in": "path",
+            "requir


This resource type name conflicts with the Microsoft.Authorization/roleAssignments which is a commonly known RBAC model. This should not have been named this since we dont want to confuse users with the Azure wide concepts like RBAC role assignments.

which line is this? But anyway I don't think we can change this now.

Not sure why the line numbers got messed up for some comments. But likely you wont be able to change it now. I am referring to the resource type "roleAssignments"

ravbhatnagar · 2017-06-28T22:25:42Z

arm-customer-insights/2017-04-26/swagger/customer-insights.json

+          {
+            "name": "kpiName",
+            "in": "path",
+            "requir


Segments not correct for this API. It would be working for you since ARM may be routing based on the parent. The correct segments should be /images/{imageName}/getEntityTypeImageUploadUrl. If imageName is not applicable then do you really need the "images" type? Can you invoke this action on a hubs/{hubName}

I don't know why it doesn't have {imageName}, but it is what our API looks like and it seems working fine

What does this API do? Is the URL same for all images in a hubs/{hubName}. I am wondering if its working as you expect then why do you even need a /images segment?

Hi, this api is to get the images' upload URL. As you can see in the swagger it depends on the entityType, entityTypeName, relativePath, so the URL is not the same for all images in a hub, but it has nothing to do with the imageName. This API is in the ImagesController, I think that's why the person who developed this add a /images segment

ravbhatnagar · 2017-06-28T23:58:01Z

For #3 - Location should be required for Tracked resources. All this means is that your create parameters look different from your update parameters. You just have to add another model which becomes the request body of your PATCH. That way customers know what is required and what is not and also what can or cannot be updated.

veronicagg · 2017-06-29T18:35:36Z

@oucfei Please review @ravbhatnagar comments, and if you confirm the resources mentioned in "3" are tracked resources, please make the corresponding updates to the spec. Thanks!

oucfei · 2017-06-29T19:18:46Z

@ravbhatnagar thanks for your review.

Most of the comment you left are on existing APIs, and I don't know how to resolve those except to tell you that this is how the API was designed and implemented. We're not going to make change to the API, nor the swagger at this point of time. Anyway this swagger was reviewed and approved before, so I think those issues should not block the PR, right?
Is it just me or the same for others, that I can't see which lines of code you're referring to in some of your comments? Again this has already been approved before so I don't think there're any real issues, but if you don't agree, please leave the exact line numbers you're referring to, and I'll take a look again.
I'm not 100% sure what a Tracked resources means, but the create parameters and the update parameters for this API are the same. There's no additional model for PATCH. So I guess it's not tracked resource?

ravbhatnagar · 2017-07-05T07:20:15Z

Yes, you are right. Feel free to ignore the feedback around existing APIs. We will follow up with you separately on that. Tracked resource means that the "routingType" is set to "default" in the ARM manifest. Hubs is a tracked resource. Anyway location of a resource cant be changed. And so if someone tries to PATCH (change the location) which they can since the same model is being used, the operation will/should fail.

oucfei · 2017-07-05T17:51:50Z

@veronicagg @ravbhatnagar I've resolved all the comments, and we've agreed not to fix the issues around existing API at this time. So could you please sign off this PR? Please let me know if you still have questions.

ravbhatnagar · 2017-07-05T18:01:31Z

@oucfei - Only one open question - Tracked resource means that the "routingType" is set to "default" in the ARM manifest. Hubs is a tracked resource. Anyway location of a resource cant be changed. And so if someone tries to PATCH (change the location) which they can since the same model is being used, the operation will/should fail. @veronicagg - This probably is not ideal SDK experience. Please confirm.

oucfei · 2017-07-05T18:32:34Z

@ravbhatnagar got it. Made "location" required in the PATCH operation.

azuresdkci · 2017-07-05T18:39:45Z

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: arm-customer-insights/2017-01-01/swagger/customer-insights.json
Before the PR: Warning(s): 36 Error(s): 2
After the PR: Warning(s): 37 Error(s): 2

File: arm-customer-insights/2017-04-26/swagger/customer-insights.json
Before the PR: Warning(s): 0 Error(s): 0
After the PR: Warning(s): 36 Error(s): 2

Know more about AutoRest Linter Guidelines.

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

ravbhatnagar · 2017-07-05T18:43:45Z

@oucfei - Location of a resource cannot be updated after the resource has been created. So it cant be made required in the PATCH. Ideally, it should not be present in the request body of the PATCH.

This reverts commit bf37472.

oucfei · 2017-07-05T18:52:13Z

@ravbhatnagar OK... sorry for my misunderstanding. But currently in the API, location is part of the patch request body, and we're explicitly checking if it's changed. So I'm not going to make change to the swagger at this time...

azuresdkci · 2017-07-05T18:57:04Z

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: arm-customer-insights/2017-01-01/swagger/customer-insights.json
Before the PR: Warning(s): 36 Error(s): 2
After the PR: Warning(s): 37 Error(s): 1

File: arm-customer-insights/2017-04-26/swagger/customer-insights.json
Before the PR: Warning(s): 0 Error(s): 0
After the PR: Warning(s): 36 Error(s): 1

Know more about AutoRest Linter Guidelines.

Send feedback and make AutoRest Linter Azure Bot smarter day by day!

Thanks for your co-operation.

oucfei · 2017-07-05T21:30:04Z

@ravbhatnagar do you agree with my above comment? can you sign off now?

ravbhatnagar · 2017-07-05T21:36:18Z

Ok, but please consider updating this at some point since it will improve the client experience.
@veronicagg - ARM signs off.

veronicagg · 2017-07-05T22:22:01Z

ok, thanks @ravbhatnagar .
@oucfei just so you know, from the SDK perspective, having location not required and as shared model for put and patch will result in an SDK API that won't "require" location for PUT body parameter (https://github.com/oucfei/azure-rest-api-specs/blob/9a36dc27b6b6a4470fad4c39d55f18161178f017/arm-customer-insights/2017-01-01/swagger/customer-insights.json#L75) so the consumer of the API may not send location and get a failed response from the service. For next updates to the API, you'd want to consider creating separate models for PUT or PATCH so you can more accurately represent the service.

oucfei · 2017-07-05T22:29:46Z

@veronicagg thanks! I will create items for the issues mentioned above.

AutorestCI · 2017-07-05T22:32:01Z

No modification for AutorestCI/azure-sdk-for-ruby

AutorestCI · 2017-07-05T22:32:06Z

No modification for AutorestCI/azure-sdk-for-python

AutorestCI · 2017-07-05T22:32:08Z

No modification for AutorestCI/azure-sdk-for-node

xiaofma added 3 commits June 23, 2017 10:30

updated to latest interaction api

57dd62a

updating examples for the latest interaction api

f948214

new api version (2017-04-26) swagger and examples

f32c863

msftclas added the cla-not-required label Jun 23, 2017

azuresdkci assigned veronicagg Jun 23, 2017

oucfei changed the title ~~Updated the 2017-01-01 version with the latest interaction API. Added 2017-04-26 version~~ [Azure Customer Insight] Updated the 2017-01-01 version with the latest interaction API. Added 2017-04-26 version Jun 23, 2017

fixing some json validation issue

4726d7f

veronicagg suggested changes Jun 23, 2017

View reviewed changes

fixing some model validation error

2223295

minor fixes

ccaf5fb

fixed the description for source/target entity type

d0a23bc

veronicagg added the potential-sdk-breaking-change label Jun 26, 2017

removing "location" from required property

85fe7eb

ravbhatnagar suggested changes Jun 28, 2017

View reviewed changes

location should be required for hub

bf37472

Revert "location should be required for hub"

9a36dc2

This reverts commit bf37472.

veronicagg approved these changes Jul 5, 2017

View reviewed changes

veronicagg merged commit 5bb3527 into Azure:master Jul 5, 2017

[Azure Customer Insight] Updated the 2017-01-01 version with the latest interaction API. Added 2017-04-26 version #1343

[Azure Customer Insight] Updated the 2017-01-01 version with the latest interaction API. Added 2017-04-26 version #1343

Uh oh!

Conversation

oucfei commented Jun 23, 2017 • 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!

msftclas commented Jun 23, 2017

Uh oh!

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

Choose a reason for hiding this comment

Uh oh!

veronicagg commented Jun 23, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ravbhatnagar commented Jun 24, 2017

Uh oh!

oucfei commented Jun 24, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

veronicagg commented Jun 26, 2017

Uh oh!

veronicagg commented Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

oucfei commented Jun 26, 2017

Uh oh!

veronicagg commented Jun 26, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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

oucfei commented Jun 23, 2017 •

edited by azuresdkci

Loading

veronicagg commented Jun 23, 2017 •

edited

Loading

oucfei commented Jun 24, 2017 •

edited

Loading

veronicagg commented Jun 26, 2017 •

edited

Loading

veronicagg commented Jun 26, 2017 •

edited

Loading