Skip to content

Conversation

@haoyingli
Copy link
Contributor

@haoyingli haoyingli commented Jul 19, 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 19, 2018

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

@haoyingli haoyingli changed the title revert tumbling window dependsOn property under preview, and apply sa… [Do not merge] [Data Factory] add tumbling window dependsOn property under stable Jul 19, 2018
@azuresdkci azuresdkci requested a review from annatisch July 19, 2018 02:25
@AutorestCI
Copy link

AutorestCI commented Jul 19, 2018

Automation for azure-sdk-for-python

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

@AutorestCI
Copy link

AutorestCI commented Jul 19, 2018

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

@AutorestCI
Copy link

AutorestCI commented Jul 19, 2018

Automation for azure-sdk-for-java

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

@AutorestCI
Copy link

AutorestCI commented Jul 19, 2018

Automation for azure-sdk-for-go

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

@annatisch
Copy link
Member

@haoyingli - do you have a timeline in mind for when this can be merged?

@haoyingli
Copy link
Contributor Author

We're waiting for our backend code to be deployed in production first. The next scheduled deployment we get from ADF team is end of the week

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@annatisch
Copy link
Member

Thanks @haoyingli - do you know if you are anticipating any further changes to this spec before the production roll-out finishes? Or is this spec already finalized?

@jhendrixMSFT
Copy link
Member

@AutorestCI regenerate azure-sdk-for-go

@annatisch
Copy link
Member

@haoyingli - any updates on the status of this PR?

@haoyingli
Copy link
Contributor Author

We were waiting for backend code to be deployed in production first, and finally it is deployed.
I will merge with latest code tomorrow, and once it's approved, we would be good to merge to master

@haoyingli
Copy link
Contributor Author

Hi, we are ready to merge the swagger. Could you please review it and let us know if it's OK to be merged to master?

],
"properties": {
"offset": {
"description": "Timespan applied to the start time of a tumbling window when evaluating dependency, .Net timespan format.",
Copy link
Member

Choose a reason for hiding this comment

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

Terms like ".Net timespan" will be very confusing to uses of clients generated in other languages. Please use official formatting terminology (e.g. ISO 8601).
If your API does not follow an official standard (which it should) then please supply an example of the formatting that needs to be supplied.

Is there a reason you're not using the swagger supported duration type rather than string?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This property "offset" is input through UI. Users have option for offset in hours, and enter the number of hour users like. Internally we use .Net for this project, and parse the offset from UI into .net timespan.

Copy link
Member

Choose a reason for hiding this comment

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

Thanks @haoyingli - but the swagger must be agnostic of any particular language - please remove references to .Net

"maxLength": 15
},
"size": {
"description": "The size of the window when evaluating the dependency. If undefined the frequency of the tumbling window will be used, .Net timespan format.",
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above.

],
"properties": {
"offset": {
"description": "Timespan applied to the start time of a tumbling window when evaluating dependency, .Net timespan format.",
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above.

"maxLength": 15
},
"size": {
"description": "The size of the window when evaluating the dependency. If undefined the frequency of the tumbling window will be used, .Net timespan format.",
Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above.

@haoyingli
Copy link
Contributor Author

I've updated the description. The format of timespan is internal only, should not expose to outside.

Copy link
Member

@annatisch annatisch left a comment

Choose a reason for hiding this comment

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

LGTM - thanks!

@annatisch annatisch merged commit 219ba00 into Azure:master Aug 29, 2018
@haoyingli haoyingli changed the title [Do not merge] [Data Factory] add tumbling window dependsOn property under stable [Data Factory] add tumbling window dependsOn property under stable Aug 29, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants