Skip to content

[Hub Generated] Review request for Microsoft.StorageSync to add version 2019-02-01#5210

Merged
veronicagg merged 8 commits intomasterfrom
dev-storagesync-Microsoft.StorageSync-2019-02-01
Mar 27, 2019
Merged

[Hub Generated] Review request for Microsoft.StorageSync to add version 2019-02-01#5210
veronicagg merged 8 commits intomasterfrom
dev-storagesync-Microsoft.StorageSync-2019-02-01

Conversation

@anpint
Copy link
Member

@anpint anpint commented Feb 15, 2019

If you are a MSFT employee you can view your work branch via this link.

Contribution checklist:

@AutorestCI
Copy link

AutorestCI commented Feb 15, 2019

Automation for azure-sdk-for-ruby

Nothing to generate for azure-sdk-for-ruby

@AutorestCI
Copy link

AutorestCI commented Feb 15, 2019

Automation for azure-sdk-for-js

A PR has been created for you:
Azure/azure-sdk-for-js#1813

@AutorestCI
Copy link

AutorestCI commented Feb 15, 2019

Automation for azure-sdk-for-python

A PR has been created for you:
Azure/azure-sdk-for-python#4371

@AutorestCI
Copy link

AutorestCI commented Feb 15, 2019

Automation for azure-sdk-for-java

A PR has been created for you:
Azure/azure-sdk-for-java#3232

@AutorestCI
Copy link

AutorestCI commented Feb 15, 2019

Automation for azure-sdk-for-node

A PR has been created for you:
Azure/azure-sdk-for-node#4691

@anpint anpint requested a review from afedyashov February 15, 2019 19:28
@AutorestCI
Copy link

AutorestCI commented Feb 15, 2019

Automation for azure-sdk-for-go

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

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@veronicagg
Copy link
Contributor

@anpint thanks for using OpenAPI Hub to create this PR, are you expecting to make more changes on the specs? or is it an api-version update with the same content?

Copy link
Contributor

@veronicagg veronicagg left a comment

Choose a reason for hiding this comment

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

see question above.

@anpint
Copy link
Member Author

anpint commented Feb 15, 2019

@anpint thanks for using OpenAPI Hub to create this PR, are you expecting to make more changes on the specs? or is it an api-version update with the same content?

We will be making additional changes to the specs. I was advised by my teammate to first create an API version update with the same content, and follow-up with another PR containing changes to the protocol.

@veronicagg
Copy link
Contributor

@anpint in this case, would you want to work against a branch for your team in the repo? We can create one for you and we could review PRs against it, if you'd like. You can put all your work there, until you're ready to send it to master.
This PR is currently against master branch and we should not merge partial work to master.
If you're going to be the only person working on the changes, you can also keep adding commits to this PR and add [Do not merge] or [Work in Progress] to the title of the PR.
If you have more questions, feel free to let me know.
Thanks!

@anpint
Copy link
Member Author

anpint commented Feb 19, 2019

@anpint in this case, would you want to work against a branch for your team in the repo? We can create one for you and we could review PRs against it, if you'd like. You can put all your work there, until you're ready to send it to master.

OpenApi Hub created this branch for us: dev-storagesync-Microsoft.StorageSync-2019-02-01. Are you suggesting that we could create PRs against that branch instead of master for incremental work?

This PR is currently against master branch and we should not merge partial work to master.
If you're going to be the only person working on the changes, you can also keep adding commits to this PR and add [Do not merge] or [Work in Progress] to the title of the PR.

This is my first time going through this process, and my team member who did this before (ankushbindlish2) suggested that we follow this procedure:

  1. Create a PR with the new API version as a copy of the previous API version. (This is the current PR)
  2. Create a second PR with the actual protocol changes.

The intention of this approach is to make it easier to review the actual protocol changes, because in this current PR, all files for the 2019-02-01 API version are being treated as new files, even though it's just a copy of the previous API version, so it will be harder to see the incremental change between the new API version, and the previous API version.

@anpint anpint changed the title [Hub Generated] Review request for Microsoft.StorageSync to add version 2019-02-01 [Hub Generated][Work in Progress] Review request for Microsoft.StorageSync to add version 2019-02-01 Feb 19, 2019
@veronicagg
Copy link
Contributor

@anpint I believe we clarified this offline. If you create a branch with the updates to the api-version, that's great, then send PRs to that branch for review, so once your work is completed you can have 1 PR against master approved and merged. If you have more questions, feel free to let me know.

…pport for parallel upload and download sync status tracking (#5233)

* Modify sync status properties to support parallel upload and download activity

* Fix a typo in storagesync.json

* Fix a linter issue with ErrorCode property in FilesNotSyncingError

* Fix a validation error with SyncActivityState enum usage

* Fix "integer" types in ServerEndpointSyncStatus to specify format

* rename cloud endpoint property storageAccountShareName to azureFileShareName
@veronicagg
Copy link
Contributor

@anpint When you're done with changes in this PR, just let me know, thanks!

@anpint anpint changed the title [Hub Generated][Work in Progress] Review request for Microsoft.StorageSync to add version 2019-02-01 [Hub Generated] Review request for Microsoft.StorageSync to add version 2019-02-01 Mar 4, 2019
@anpint
Copy link
Member Author

anpint commented Mar 4, 2019

@veronicagg The changes for this protocol have been finalized.

@veronicagg
Copy link
Contributor

@anpint thanks, could you please review the CI failures for linter? Thanks!

@anpint
Copy link
Member Author

anpint commented Mar 5, 2019

@anpint thanks, could you please review the CI failures for linter? Thanks!

Sorry for not noticing the failures before. I wanted to define some properties in the protocol as both readonly and required, and it looks like that's not supported. I kept them as readonly.

@veronicagg
Copy link
Contributor

Change looks good, this PR is composed of the new api-version with an already reviewed commit plus a few changes to the properties.
Do you still need to wait on afedyashov for review?
Are the APIs deployed? If so, I can approve and merge this PR.
Thanks!

@anpint
Copy link
Member Author

anpint commented Mar 6, 2019

Change looks good, this PR is composed of the new api-version with an already reviewed commit plus a few changes to the properties.

Yes, that's correct.

Do you still need to wait on afedyashov for review?

No, we don't need to wait. Initially he was going to add some changes to the protocol, but we dicided not to make further changes.

Are the APIs deployed? If so, I can approve and merge this PR.

Can you clarify this? Do you require having the 2019-02-01 API version already supported by our RP in production? This API version is currently making its way through our release pipeline, so it will be a few weeks before it hits production. As a general process, I would expect that swagger should be the first step in creating a new API version (so that the changes go through proper approvals before we implement them in our service). Is that not the case?

@veronicagg
Copy link
Contributor

@anpint It's correct that the swagger is the first step, but we should have the APIs deployed in production before merging the PR. The reason is that once a swagger is merged in master, an SDK can get generated right away, and if the APIs are not available the SDKs won't work for customers. Hope that makes sense.
Let me know when the APIs are deployed and I can merge this PR. Thanks!

@anpint
Copy link
Member Author

anpint commented Mar 8, 2019

@anpint It's correct that the swagger is the first step, but we should have the APIs deployed in production before merging the PR. The reason is that once a swagger is merged in master, an SDK can get generated right away, and if the APIs are not available the SDKs won't work for customers. Hope that makes sense.
Let me know when the APIs are deployed and I can merge this PR. Thanks!

Ok, I understand now. I'll tag this PR as "Awaiting Deployment" and update it again once we have the new API version in production.

@anpint anpint changed the title [Hub Generated] Review request for Microsoft.StorageSync to add version 2019-02-01 [Awaiting Deployment][Hub Generated] Review request for Microsoft.StorageSync to add version 2019-02-01 Mar 8, 2019
adxsdknet added a commit to adxsdknet/azure-sdk-for-net that referenced this pull request Mar 8, 2019
REST Spec PR 'Azure/azure-rest-api-specs#5210'
REST Spec PR Author 'anpint'
REST Spec PR Last commit
adxsdknet added a commit to adxsdknet/azure-sdk-for-net that referenced this pull request Mar 26, 2019
REST Spec PR 'Azure/azure-rest-api-specs#5210'
REST Spec PR Author 'anpint'
REST Spec PR Last commit
@adxsdknet
Copy link

Automation for azure-sdk-for-net

A PR has been created for you:
Azure/azure-sdk-for-net#5596
.NET SDK Commits:
adxsdknet/azure-sdk-for-net@007d151

@anpint anpint changed the title [Awaiting Deployment][Hub Generated] Review request for Microsoft.StorageSync to add version 2019-02-01 [Hub Generated] Review request for Microsoft.StorageSync to add version 2019-02-01 Mar 26, 2019
@anpint anpint requested a review from veronicagg March 26, 2019 22:58
@anpint
Copy link
Member Author

anpint commented Mar 26, 2019

The service code that supports this new API version is now in our Canary regions, so we would like to go ahead and complete the PR. We will be rolling out the service to all production regions in the next few weeks.

@veronicagg
Copy link
Contributor

@anpint ok, great. Due to my explanation above, we should not merge and release SDKs until the APIs are in production, please let me know when that's the case, for PR merge.

@ankushbindlish2
Copy link
Member

@veronicagg We are already LIVE in FLIGHT regions. Please merge this change. All SDKS would be publish only after ALL regions are LIVE.

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.

6 participants