[Do not merge][Datafactory] Add Tumbling Window dependsOn property to Trigger.json #2902

alexsaff · 2018-04-18T21:57:55Z

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-18T21:58:06Z

All CLA requirements met.

AutorestCI · 2018-04-18T22:01:42Z

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-18T22:02:58Z

Automation for azure-sdk-for-node

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-node#2770

AutorestCI · 2018-04-18T22:04:24Z

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

AutorestCI · 2018-04-18T22:22:56Z

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

hvermis · 2018-04-19T00:01:28Z

...y/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/entityTypes/Trigger.json

+      "type": "object",
+      "properties": {
+        "type": {
+          "description": "Reference type \"TriggerReference\" for Tumbling Window.",


Suggested description: "Tumbling window trigger reference type." This is consistent with our other descriptions of similar kind.

hvermis · 2018-04-19T00:27:20Z

...y/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/entityTypes/Trigger.json

@@ -365,6 +372,24 @@
          "maximum": 86400
        }
      }
+    },
+    "TumblingWindowDependency": {


Are we planning on having conditional dependency for triggers? If not then it is better to name this as "TumblingWindowTriggerReference" to be consistent with other naming conventions.

hvermis · 2018-04-19T00:27:32Z

...y/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/entityTypes/Trigger.json

@@ -365,6 +372,24 @@
          "maximum": 86400
        }
      }
+    },
+    "TumblingWindowDependency": {
+      "description": "Tumbling Window dependency information.",


Suggested description: "Tumbling window trigger reference type."

hvermis · 2018-04-19T00:32:29Z

...y/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/entityTypes/Trigger.json

+      "properties": {
+        "type": {
+          "description": "Reference type \"TriggerReference\" for Tumbling Window.",
+          "type": "string"


I think ARM folks will comment on this too, you should add an enum here:

"enum": [
"TumblingWindowTriggerReference"
]

hvermis · 2018-04-19T00:33:21Z

...y/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/entityTypes/Trigger.json

+          "type": "string"
+        },
+        "referenceName": {
+          "description": "Trigger reference name.",


"Reference TumblingWindowTrigger name."

hvermis · 2018-04-19T00:36:02Z

...y/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/entityTypes/Trigger.json

+            },
+            "dependsOn": {
+              "type": "array",
+              "description": "Tumbling Window depends on condition.",


In your case there is no condition, so suggested description: "Tumbling window triggers that this trigger depends on."

azuresdkciprbot · 2018-04-19T15:52:14Z

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): 0 Error(s): 0

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

hvermis · 2018-04-19T16:06:01Z

...y/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/entityTypes/Trigger.json

+              "type": "array",
+              "description": "Tumbling window triggers that this trigger depends on.",
+              "items": {
+                "$ref": "#/definitions/TumblingWindowDependency"


You need to change this too to "TumblingWindowTriggerReference"

Already resolved

azuresdkciprbot · 2018-04-19T16:13: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:

💡 Please review potentially introduced Error(s)/Warning(s): Analysis Report 💡

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

AutoRest Linter Guidelines | AutoRest Linter Issues | Send feedback

Thanks for your co-operation.

mcardosos · 2018-04-19T17:11:02Z

...y/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/entityTypes/Trigger.json

+        "type": {
+          "description": "Tumbling window trigger reference type.",
+          "type": "string",
+          "enum": [


enum [](start = 11, length = 4)

Also use x-ms-enum

Using x-ms-enum is blocked by hvermis

@mcardosos We don't want enum types generated for this, it is more of a hint to customer that this is the value. Here's another example where our swagger has the same structure: https://github.com/Azure/azure-rest-api-specs/blob/master/specification/datafactory/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/datafactory.json#L3024

mcardosos · 2018-04-19T17:11:32Z

...y/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/entityTypes/Trigger.json

+      "description": "Tumbling window trigger reference type.",
+      "type": "object",
+      "properties": {
+        "type": {


type [](start = 9, length = 4)

Is this type a discriminator?

Here is a JSON example:
"dependsOn": [
{
"type": "TriggerReference",
"referenceName": "mytumblingwindowtrigger1"
},
{
"type": "TriggerReference",
"referenceName": "mytumblingwindowtrigger2"
}
]

What I mean is, could the type have another value?

No, the type is not discriminator.

At this point we are not planning "type" to have another value, may be in the future

hvermis · 2018-04-19T19:47:35Z

@mcardosos Please do not merge this yet as the backend is not ready yet.

mcardosos · 2018-04-20T17:57:10Z

@hvermis When will this be available?

mcardosos

For awareness, this swagger has linter errors. I know they were not introduced in this PR

salameer · 2018-05-02T02:45:53Z

@hvermis please ping this thread when your ready to merge this swagger

hvermis · 2018-06-05T21:47:49Z

...y/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/entityTypes/Trigger.json

+          ]
+        },
+        "referenceName": {
+          "description": "Reference TumblingWindowTrigger name.",


Update the description too

hvermis · 2018-06-05T21:48:28Z

...y/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/entityTypes/Trigger.json

@@ -365,6 +372,27 @@
          "maximum": 86400
        }
      }
+    },
+    "TumblingWindowTriggerReference": {


Change the type name here too

hvermis · 2018-06-05T21:48:35Z

...y/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/entityTypes/Trigger.json

+              "type": "array",
+              "description": "Tumbling window triggers that this trigger depends on.",
+              "items": {
+                "$ref": "#/definitions/TumblingWindowTriggerReference"


Change it here too

hvermis · 2018-06-05T21:49:06Z

...y/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/entityTypes/Trigger.json

+            },
+            "dependsOn": {
+              "type": "array",
+              "description": "Tumbling window triggers that this trigger depends on.",


Leave the description here the same to specifically indicate that it is a tumbling window trigger

You can also add - "Only tumbling window triggers are supported." If we ever add other trigger types here we will update the description.

…erence

hvermis · 2018-06-05T22:29:51Z

...y/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/entityTypes/Trigger.json

@@ -365,6 +372,27 @@
          "maximum": 86400
        }
      }
+    },
+    "TriggerReference": {
+      "description": "Tumbling window trigger reference type.",


Remove "Tumbling window" from description.

hvermis · 2018-06-05T22:30:02Z

...y/resource-manager/Microsoft.DataFactory/preview/2017-09-01-preview/entityTypes/Trigger.json

+      "type": "object",
+      "properties": {
+        "type": {
+          "description": "Tumbling window trigger reference type.",


Remove "Tumbling window" from description.

AutorestCI · 2018-06-06T03:17:13Z

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

lmazuel · 2018-06-30T00:04:08Z

@AutorestCI rebuild azure-sdk-for-python

hvermis · 2018-06-30T00:41:04Z

@alexsaff I beleive this PR should be closed and new one opened by Nalini?

alexsaff · 2018-07-05T17:00:15Z

This PR is abandoned and will be replaced by a new one with a different data model

AutorestCI · 2018-07-05T18:26:30Z

Automation for azure-sdk-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:
Azure/azure-sdk-for-java#2136

Add Tumbling Window dependsOn property

d3332a5

azuresdkci assigned mcardosos Apr 18, 2018

azuresdkci requested a review from mcardosos April 18, 2018 21:59

alexsaff changed the title ~~Master~~ Add Tumbling Window dependsOn property to Trigger.json Apr 18, 2018

This was referenced Apr 18, 2018

[AutoPR datafactory/resource-manager] Master AutorestCI/azure-libraries-for-java#92

Open

[AutoPR datafactory/resource-manager] Master Azure/azure-sdk-for-node#2760

Closed

AutorestCI mentioned this pull request Apr 18, 2018

[AutoPR datafactory/resource-manager] Add Tumbling Window dependsOn property to Trigger.json Azure/azure-sdk-for-python#2428

Closed

AutorestCI mentioned this pull request Apr 18, 2018

[AutoPR datafactory/resource-manager] Add Tumbling Window dependsOn property to Trigger.json Azure/azure-sdk-for-go#1649

Closed

hvermis requested changes Apr 19, 2018

View reviewed changes

Updates on hvermis notes

b67ca87

Fix build error due to #ref change

9001692

hvermis reviewed Apr 19, 2018

View reviewed changes

mcardosos reviewed Apr 19, 2018

View reviewed changes

alexsaff changed the title ~~Add Tumbling Window dependsOn property to Trigger.json~~ [Datafactory] Add Tumbling Window dependsOn property to Trigger.json Apr 19, 2018

hvermis changed the title ~~[Datafactory] Add Tumbling Window dependsOn property to Trigger.json~~ [Do not merge][Datafactory] Add Tumbling Window dependsOn property to Trigger.json Apr 19, 2018

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

mcardosos approved these changes Apr 20, 2018

View reviewed changes

salameer unassigned mcardosos May 2, 2018

salameer added the Reassign label May 2, 2018

azuresdkci removed the Reassign label May 2, 2018

azuresdkci assigned hovsepm May 2, 2018

azuresdkci requested a review from hovsepm May 2, 2018 03:03

Update enum for TumblingWindowTriggerReference to TriggerReference

c304005

hvermis reviewed Jun 5, 2018

View reviewed changes

Update to use TriggerReference instead of Tumbling window trigger ref…

d5f7d2b

…erence

hvermis reviewed Jun 5, 2018

View reviewed changes

AutorestCI mentioned this pull request Jun 6, 2018

[AutoPR datafactory/resource-manager] [Do not merge][Datafactory] Add Tumbling Window dependsOn property to Trigger.json Azure/azure-sdk-for-go#1972

Closed

hovsepm added the Reassign label Jun 8, 2018

alexsaff closed this Jul 5, 2018

AutorestCI mentioned this pull request Jul 5, 2018

[AutoPR datafactory/resource-manager] [Do not merge][Datafactory] Add Tumbling Window dependsOn property to Trigger.json Azure/azure-sdk-for-java#2179

Closed

[Do not merge][Datafactory] Add Tumbling Window dependsOn property to Trigger.json #2902

[Do not merge][Datafactory] Add Tumbling Window dependsOn property to Trigger.json #2902

Conversation

alexsaff commented Apr 18, 2018 • edited by azuresdkci Loading

PR information

Quality of Swagger

msftclas commented Apr 18, 2018 • edited Loading

AutorestCI commented Apr 18, 2018 • edited Loading

Automation for azure-libraries-for-java

AutorestCI commented Apr 18, 2018 • edited Loading

Automation for azure-sdk-for-node

AutorestCI commented Apr 18, 2018 • edited Loading

Automation for azure-sdk-for-python

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

azuresdkciprbot commented Apr 19, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

azuresdkciprbot commented Apr 19, 2018

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

alexsaff Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alexsaff Apr 19, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hvermis commented Apr 19, 2018

mcardosos commented Apr 20, 2018

mcardosos left a comment

Choose a reason for hiding this comment

salameer commented May 2, 2018

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

AutorestCI commented Jun 6, 2018 • edited Loading

Automation for azure-sdk-for-ruby

lmazuel commented Jun 30, 2018

hvermis commented Jun 30, 2018

alexsaff commented Jul 5, 2018

AutorestCI commented Jul 5, 2018 • edited Loading

Automation for azure-sdk-for-java

alexsaff commented Apr 18, 2018 •

edited by azuresdkci

Loading

msftclas commented Apr 18, 2018 •

edited

Loading

AutorestCI commented Apr 18, 2018 •

edited

Loading

AutorestCI commented Apr 18, 2018 •

edited

Loading

AutorestCI commented Apr 18, 2018 •

edited

Loading

AutorestCI commented Apr 18, 2018 •

edited

Loading

alexsaff Apr 19, 2018 •

edited

Loading

alexsaff Apr 19, 2018 •

edited

Loading

AutorestCI commented Jun 6, 2018 •

edited

Loading

AutorestCI commented Jul 5, 2018 •

edited

Loading