[Datafactory] Add configure repo api swagger #2866

frankycrm14 · 2018-04-12T21:50:04Z

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 · 2018-04-12T21:50:15Z

All CLA requirements met.

AutorestCI · 2018-04-12T21:53:47Z

Automation for azure-libraries-for-java

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:
AutorestCI/azure-libraries-for-java#18

AutorestCI · 2018-04-12T21:57:58Z

Automation for azure-sdk-for-python

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

AutorestCI · 2018-04-12T22:15:57Z

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

AutorestCI · 2018-04-12T22:26:09Z

Automation for azure-sdk-for-node

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

hvermis · 2018-04-12T22:09:05Z

...tafactory/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/datafactory.json

@@ -105,6 +105,54 @@
        }
      }
    },
+    "/subscriptions/{subscriptionId}/providers/Microsoft.DataFactory/locations/{locationId}/configureFactoryRepo": {


This path doesn't look valid.
should be like this:
/subscriptions/{subscriptionId}/resourceGroups/{resourceGroupName}/providers/Microsoft.DataFactory/factories/{factoryName}/configureFactoryRepo. And if you need locationId passed from client, pass it as a parameter in query string or in body.

As we talked offline about this, I'll explore other options, and ask feedback on this from the ARM tema.

I doubt I need to tell you, @ravbhatnagar, but make sure to take a look at this comment thread. :)

Just replied on email, IMO this should be modeled as an action on /factories/{factoryName} as mentioned by @hvermis

Also, this returns a factory resource, Is it fine where someone who just has permission to this POST action can read the resource even though he may not have /read or /write permission. I am sure this API is not creating a factory resource. Is it changing the shape of the factory resource?

this changes some properties of the factory, to call this api you will need to have permissions for this post call and also write permissions to the factory

marstr

I've verified that non of the CI failures regarding the files in this PR are introduced by these changes. Other than the already noted path concerns, this PR looks good from a code-generation perspective.

marstr · 2018-04-13T16:26:45Z

...tafactory/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/datafactory.json

@@ -105,6 +105,54 @@
        }
      }
    },
+    "/subscriptions/{subscriptionId}/providers/Microsoft.DataFactory/locations/{locationId}/configureFactoryRepo": {


I doubt I need to tell you, @ravbhatnagar, but make sure to take a look at this comment thread. :)

hvermis · 2018-04-18T01:14:34Z

...tafactory/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/datafactory.json

        ],
-        "operationId": "Factories_ListByResourceGroup",
+        "operationId": "ConfigureRepo_Update",


This should be Factories_ConfigureRepo

hvermis · 2018-04-18T01:16:31Z

...tafactory/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/datafactory.json

        "x-ms-examples": {
-          "Factories_ListByResourceGroup": {
-            "$ref": "./examples/Factories_ListByResourceGroup.json"
+          "Factories_CreateOrUpdate": {


This also should be Factories_ConfigureRepo

hvermis · 2018-04-18T01:19:10Z

...tafactory/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/datafactory.json

-          "Factories_ListByResourceGroup": {
-            "$ref": "./examples/Factories_ListByResourceGroup.json"
+          "Factories_CreateOrUpdate": {
+            "$ref": "./examples/Factories_UpdateRepo.json"


The name of the example file usually matches the operation ID, should be Factories_ConfigureRepo.json

hvermis

Make sure to address the comments on your last commit too.

hvermis · 2018-04-18T01:23:05Z

...tafactory/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/datafactory.json

+        "description": "Factory's VSTS repo information.",
+        "properties": {
+          "factoryId": {
+            "description": "The factory id.",


This is the full ARM resource ID for the factory right? Since your API is under factory resource now, you shouldn't need this, since all information will be in your path.

I need this property for the linkedActionCheck

You can construct it since you have the factory name, rg name, sub id in your headers when the API gets called. We shouldn't require the client to send this extra field if we already have all the information.

azuresdkciprbot · 2018-04-18T17:14: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:

File: specification/datafactory/resource-manager/readme.md
Before the PR: Warning(s): 14 Error(s): 1
After the PR: Warning(s): 14 Error(s): 1

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

hvermis · 2018-04-18T17:45:34Z

...tafactory/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/datafactory.json

+        "description": "Factory's VSTS repo information.",
+        "properties": {
+          "factoryId": {
+            "description": "The factory id.",


You can construct it since you have the factory name, rg name, sub id in your headers when the API gets called. We shouldn't require the client to send this extra field if we already have all the information.

hvermis · 2018-04-18T17:47:11Z

...nager/Microsoft.DataFactory/preview/2017-09-01-preview/examples/Factories_ConfigureRepo.json

+            "tenantId": "12f988bf-86d1-41af-91ab-2d7cd011db49"
+          }
+        },
+        "id": "/subscriptions/12345678-1234-1234-12345678abc/resourceGroups/exampleresourcegroup/providers/Microsoft.DataFactory/factories/examplefactoryname",


See, this Id gets returned. So in the code you might even have this as one of the parameters passed to your API.

Talked offline, the resource ID is for the cases when the call goes through the desired action "Microsoft.DataFactory/factories/configureRepo/action" and ARM needs it to validate the permissions. Recalling my comment.

azuresdkciprbot · 2018-04-18T18:48:16Z

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/datafactory/resource-manager/readme.md
Before the PR: Warning(s): 14 Error(s): 1
After the PR: Warning(s): 14 Error(s): 1

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

ravbhatnagar · 2018-04-19T05:55:11Z

Signing off as the feedback given over email has been addressed.

marstr · 2018-04-19T16:53:31Z

Are these changes deployed to the public? Are you ready for me to merge?

frankycrm14 · 2018-04-19T20:22:24Z

Not yet Martin, just doing validation, I'll let you know once this is ready.
thanks!

marstr · 2018-04-19T21:24:14Z

Got it, @frankycrm14. What's the timeline? Should we be closing this PR for a while until you're ready?

frankycrm14 · 2018-04-19T23:56:51Z

I'm testing the api to verify that the approach we took actually works.

This reverts commit fa5757e.

This reverts commit 2c233a5.

This reverts commit 27d71a4.

azuresdkciprbot · 2018-04-26T22:20:32Z

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

⚠️

0 new Warnings.(5 total)

❌0 new Errors.(0 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

azuresdkciprbot · 2018-04-26T22:20:32Z

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

⚠️

0 new Warnings.(9 total)

❌0 new Errors.(1 total)

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

marstr · 2018-05-07T23:08:53Z

Still working on this PR, @frankycrm14? What's its status?

frankycrm14 · 2018-05-08T00:08:33Z

It’s done, but I’m waiting for deployment on the backend, I was told that’s the way to go, not to change swagger until changes are deployed in backend’s prod. It’s been taking some time to deploy on the backend side for some issues that were found, but hopefully it will be done soon. From: Martin Strobel [mailto:notifications@github.com] Sent: Monday, May 7, 2018 4:09 PM To: Azure/azure-rest-api-specs <azure-rest-api-specs@noreply.github.com> Cc: Franky Gutierrez Gutierrez <fgutierr@microsoft.com>; Mention <mention@noreply.github.com> Subject: Re: [Azure/azure-rest-api-specs] [Do not merge][Datafactory] Add configure repo api swagger (#2866) Still working on this PR, @frankycrm14<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Ffrankycrm14&data=02%7C01%7Cfgutierr%40microsoft.com%7C56077c32127a490c6b3908d5b46f8724%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636613313460308450&sdata=%2F0a9PbWkd5KFaCjK7RMy%2BQKDQBYcFin%2Fp1Iuc6DsiuI%3D&reserved=0>? What's its status? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2FAzure%2Fazure-rest-api-specs%2Fpull%2F2866%23issuecomment-387233858&data=02%7C01%7Cfgutierr%40microsoft.com%7C56077c32127a490c6b3908d5b46f8724%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636613313460318458&sdata=S%2B0leo3EmahgHxsxQjxWHsINzB3Er80MJYsL0Dr2iTI%3D&reserved=0>, or mute the thread<https://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAkhz1HngeVdMJUgMkBiE0hsLfR3pCF5Vks5twNQQgaJpZM4TSjBM&data=02%7C01%7Cfgutierr%40microsoft.com%7C56077c32127a490c6b3908d5b46f8724%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636613313460328462&sdata=vKItU5BDZ7y5vImhCVETolWsiCBZohMa%2F30uExMVOb0%3D&reserved=0>.

marstr · 2018-05-22T16:37:28Z

...tafactory/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/datafactory.json

+            "description": "The factory resource id.",
+            "type": "string"
+          },
+          "resourceGroupName": {


I'd ref this definition instead, in order to get client-side validation:
https://github.com/Azure/azure-rest-api-specs/pull/2866/files#diff-bf6d272fbfeeed92d1fc24a2bb8706a8R3781

Chatted offline with @frankycrm14 about this, it looks like the defintion I mentioned is in the parameters section. For that reason, the real fix would be to refactor most of this into the definitions section, and reference that from the parameter and anywhere else it's used. However, that would only buy us a little client-side validation, and wouldn't be a breaking change to fix in the future. For those reasons, I'm not going to block this PR on this kinda comment.

All that remains before I merge is ensuring that none of the linter errors being reported were introduced by this PR.

marstr

To the best of my knowledge, I believe the CI errors here predate this PR.

add configure repo api swagger

d7f5567

azuresdkci assigned marstr Apr 12, 2018

azuresdkci requested a review from marstr April 12, 2018 21:50

AutorestCI mentioned this pull request Apr 12, 2018

[AutoPR datafactory/resource-manager] add configure repo api swagger AutorestCI/azure-libraries-for-java#75

Open

AutorestCI mentioned this pull request Apr 12, 2018

[AutoPR datafactory/resource-manager] add configure repo api swagger Azure/azure-sdk-for-python#2392

Merged

AutorestCI mentioned this pull request Apr 12, 2018

[AutoPR datafactory/resource-manager] add configure repo api swagger Azure/azure-sdk-for-go#1579

Closed

AutorestCI mentioned this pull request Apr 12, 2018

[AutoPR datafactory/resource-manager] add configure repo api swagger Azure/azure-sdk-for-node#2731

Merged

frankycrm14 added 2 commits April 12, 2018 16:09

fixing autorest issue

42b127d

add missing parameter

c6c57b0

hvermis requested changes Apr 13, 2018

View reviewed changes

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

marstr reviewed Apr 13, 2018

View reviewed changes

change route

27d71a4

hvermis requested changes Apr 18, 2018

View reviewed changes

AutorestCI mentioned this pull request Apr 18, 2018

[AutoPR datafactory/resource-manager] add configure repo api swagger Azure/azure-sdk-for-go#1634

Closed

hvermis requested changes Apr 18, 2018

View reviewed changes

fix per comments

2c233a5

hvermis requested changes Apr 18, 2018

View reviewed changes

change factoryId for factoryResourceId

fa5757e

hvermis approved these changes Apr 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 Apr 19, 2018

hvermis changed the title ~~add configure repo api swagger~~ [Do not merge][Datafactory] Add configure repo api swagger Apr 19, 2018

marstr added the DoNotMerge <valid label in PR review process> use to hold merge after approval label Apr 23, 2018

frankycrm14 added 4 commits April 23, 2018 13:49

Revert "change factoryId for factoryResourceId"

d9d3d1e

This reverts commit fa5757e.

Revert "fix per comments"

0628a88

This reverts commit 2c233a5.

Revert "change route"

7fd89d2

This reverts commit 27d71a4.

rename example

47d1905

marstr requested changes May 22, 2018

View reviewed changes

marstr approved these changes May 22, 2018

View reviewed changes

marstr removed the DoNotMerge <valid label in PR review process> use to hold merge after approval label May 22, 2018

marstr changed the title ~~[Do not merge][Datafactory] Add configure repo api swagger~~ [Datafactory] Add configure repo api swagger May 22, 2018

marstr merged commit 7a6afb0 into Azure:master May 22, 2018

AutorestCI mentioned this pull request May 22, 2018

[AutoPR datafactory/resource-manager] [Datafactory] Add configure repo api swagger Azure/azure-sdk-for-go#1884

Merged

frankycrm14 mentioned this pull request May 23, 2018

[Data Factory] Adding configure factory repo operation Azure/azure-sdk-for-net#4360

Merged

8 tasks

[Datafactory] Add configure repo api swagger #2866

[Datafactory] Add configure repo api swagger #2866

Conversation

frankycrm14 commented Apr 12, 2018 • edited Loading

PR information

Quality of Swagger

msftclas commented Apr 12, 2018 • edited Loading

AutorestCI commented Apr 12, 2018 • edited Loading

Automation for azure-libraries-for-java

AutorestCI commented Apr 12, 2018 • edited Loading

Automation for azure-sdk-for-python

AutorestCI commented Apr 12, 2018 • edited Loading

Automation for azure-sdk-for-go

AutorestCI commented Apr 12, 2018 • edited Loading

Automation for azure-sdk-for-node

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

hvermis 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

azuresdkciprbot commented Apr 18, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azuresdkciprbot commented Apr 18, 2018

ravbhatnagar commented Apr 19, 2018

marstr commented Apr 19, 2018

frankycrm14 commented Apr 19, 2018

marstr commented Apr 19, 2018

frankycrm14 commented Apr 19, 2018

azuresdkciprbot commented Apr 26, 2018

AutoRest linter results for SDK Related Validation Errors/Warnings

azuresdkciprbot commented Apr 26, 2018

AutoRest linter results for ARM Related Validation Errors/Warnings

marstr commented May 7, 2018

frankycrm14 commented May 8, 2018 via email

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marstr left a comment

Choose a reason for hiding this comment

frankycrm14 commented Apr 12, 2018 •

edited

Loading

msftclas commented Apr 12, 2018 •

edited

Loading

AutorestCI commented Apr 12, 2018 •

edited

Loading

AutorestCI commented Apr 12, 2018 •

edited

Loading

AutorestCI commented Apr 12, 2018 •

edited

Loading

AutorestCI commented Apr 12, 2018 •

edited

Loading