Skip to content

Conversation

@alexsaff
Copy link

@alexsaff alexsaff commented Jul 5, 2018

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

@AutorestCI
Copy link

AutorestCI commented Jul 5, 2018

Automation for azure-sdk-for-python

Nothing to generate for azure-sdk-for-python

@alexsaff alexsaff changed the title Master [Do not merge][Datafactory] Add Tumbling Window dependsOn property to Trigger.json for Tumbling Window Trigger and self-dependent Tumbling Window Trigger Jul 5, 2018
@AutorestCI
Copy link

AutorestCI commented Jul 5, 2018

Automation for azure-sdk-for-node

Nothing to generate for azure-sdk-for-node

@AutorestCI
Copy link

AutorestCI commented Jul 5, 2018

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

@AutorestCI
Copy link

AutorestCI commented Jul 5, 2018

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

@hovsepm hovsepm added the DoNotMerge <valid label in PR review process> use to hold merge after approval label Jul 5, 2018
@hovsepm
Copy link
Contributor

hovsepm commented Jul 5, 2018

@alexsaff please ping me directly when this PR will be ready for review and merge.

@hovsepm hovsepm changed the title [Do not merge][Datafactory] Add Tumbling Window dependsOn property to Trigger.json for Tumbling Window Trigger and self-dependent Tumbling Window Trigger [Datafactory] Add Tumbling Window dependsOn property to Trigger.json for Tumbling Window Trigger and self-dependent Tumbling Window Trigger Jul 5, 2018
@AutorestCI
Copy link

AutorestCI commented Jul 5, 2018

Automation for azure-sdk-for-go

A PR has been created for you:
Azure/azure-sdk-for-go#2222

"description": "Referenced trigger",
"$ref": "#/definitions/TriggerReference"
},
"offset": {
Copy link
Contributor

@hvermis hvermis Jul 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

offset and size can be moved to a separate type and reused in both self dependency and tumbling widow dependency reference classes. Please refer to the mail thread and the doc Nailini created. #Resolved

Copy link
Contributor

@hvermis hvermis Jul 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also it will be better to create another class derived from DependencyReference as Nalini had in her doc and put referenceTrigger in that class, because in future if you support ScheduledTriggerReference, it will be derived from this new class along with TumblingWindowTriggerDependencyReference. Only in case of self rereference you don't need to have referenceTrigger property, in all other cases you will need it. If we don't do these changes now, later they will breaking changes. #Resolved

Copy link
Author

@alexsaff alexsaff Jul 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Offset is required for self-dependency, so it makes not much sense to create a separate type for offset and size. #Resolved

Copy link
Author

@alexsaff alexsaff Jul 6, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

TriggerDependencyReference has been added #Resolved

},
"dependsOn": {
"type": "array",
"description": "Tumbling window triggers that this trigger depends on. Only tumbling window triggers are supported.",
Copy link
Contributor

@hvermis hvermis Jul 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Tumbling window [](start = 30, length = 16)

Remove this tumbling window part from here:
Triggers that this trigger depends on. Only tumbling window triggers are supported. #Resolved

]
},
"DependencyReference": {
"description": "Referenced dependency",
Copy link
Contributor

@hvermis hvermis Jul 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need dot at the end, otherwise validation will fail. #Resolved

"discriminator": "type",
"properties": {
"type": {
"description": "The type of dependency reference",
Copy link
Contributor

@hvermis hvermis Jul 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dot #Resolved

]
},
"TriggerDependencyReference": {
"description": "Trigger referenced dependency",
Copy link
Contributor

@hvermis hvermis Jul 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dot #Resolved

],
"properties": {
"referenceTrigger": {
"description": "Referenced trigger",
Copy link
Contributor

@hvermis hvermis Jul 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dot #Resolved

]
},
"TumblingWindowTriggerDependencyReference": {
"description": "Referenced tumbling window trigger dependency",
Copy link
Contributor

@hvermis hvermis Jul 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dot #Resolved

],
"properties": {
"offset": {
"description": "Timespan applied to the start time of a tumbling window when evaluating dependency, pattern: ((\\d+)\\.)?(\\d\\d):(60|([0-5][0-9])):(60|([0-5][0-9])).",
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

((\d+)\.)?(\d\d):(60|([0-5][0-9])):(60|([0-5][0-9])) [](start = 119, length = 56)

I think you can add this as validation, then SDK will have a code for that. Check the parameters section in https://github.com/Azure/azure-rest-api-specs/blob/master/specification/datafactory/resource-manager/Microsoft.DataFactory/stable/2018-06-01/datafactory.json

}
},
"SelfDependencyTumblingWindowTriggerReference": {
"description": "Self referenced tumbling window trigger dependency",
Copy link
Contributor

@hvermis hvermis Jul 7, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Dot #Resolved

Copy link
Contributor

@hvermis hvermis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

:shipit:

@jhendrixMSFT
Copy link
Member

@AutorestCI regenerate azure-sdk-for-go

@alexsaff
Copy link
Author

Replace by #3414

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

DoNotMerge <valid label in PR review process> use to hold merge after approval

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants