CSharp SDK - June 2018#4488
Conversation
|
@frustoo2004 Please fix the failing tests. You may have to re-record the tests |
|
@dsgouda , Please find the REST spec PR - Azure/azure-rest-api-specs#3273 |
|
@frustoo2004 Please pull from upstream and replace the |
dsgouda
left a comment
dsgouda
left a comment
There was a problem hiding this comment.
Since the api version is being bumped, please bump the major version in csproj
Also please run
msbuild build.proj /t:build /p:Scope=SDKs\Management.RecoveryServices.Backup
and
msbuild build.proj /t:build /p:Scope=SDKs\Management.RecoveryServices
and commit all the artifacts generated
|
@dsgouda , There has been no change in API version, '2017-07-01' is still the latest. Can you let me know why you think API version is being bumped up? |
There was a problem hiding this comment.
@frustoo2004
Looks like an SDK for the 2016-12-01 version was never published. Which implies the next SDK to be published will be a new version of the REST API, hence major version bump
There was a problem hiding this comment.
@dsgouda , We have alerady published SDK with the following versions- "2016-12-01" and "2017-07-01". If I remember correctly, the last SDK publish date was August 2017. Let me know if you need any more information
There was a problem hiding this comment.
Adding @DheerendraRathor , who published the last SDK.
There was a problem hiding this comment.
This version corresponds to the REST API version, not the SDK version.,
There was a problem hiding this comment.
@dsgouda I don't get it. SDK was published with version 2016-12-01 in https://www.nuget.org/packages/Microsoft.Azure.Management.RecoveryServices.Backup/2.1.0-preview
Also why major version bump is required when only API version is upgraded and no contract change has been made given that api versions are not even transparent to SDK user?
There was a problem hiding this comment.
@frustoo2004 Could you point us to the nuget package that was published with 2017-07-01 API version?
@DheerendraRathor the concern is whenever we bump the API version (2016-12-01 to 2017-07-01 in this case) the major version number must be bumped for the package.
Hope this makes sense.
There was a problem hiding this comment.
@dsgouda This doesn't make sense to me. Why does API version change require major version bumping when everything is backward compatible? And in case of C# SDK, user can't even change api-version so SDK is pretty much a black box here.
Last time major version bumping (from 1.3 to 2.0) was done due to real breaking changes!
There was a problem hiding this comment.
@shahabhijeet , Can you please review this?. @dsgouda , Can you please expedite the process?
There was a problem hiding this comment.
@frustoo2004 and @DheerendraRathor please understand that there are versioning requirements that .NET SDK follows.
You have couple of issues in this PR
- Why are you still in preview when your REST spec isn't
- Why have you bumped up your API version to begin with?
- If you have not broken any contact, why the same changes cannot be applied to 2016-12-02
-
SDK is just a reflection of your REST spec.
-
Ideally you should be making additive change to your existing API version and not upgrade from 2016-12-01 to 2017-07-01
Since you last released, new feature has been introduced that the user will see the underlying API version on the nuget.org site (here is one such example)
For full desktop .NET framework scenarios, slso consider how can a user who wants to use functionality introduced in 2016 API version and 2017 SxS (side by side)
Please bump up major version if not, please clarify why have you changed the underlying API version if the changes are additive and does not break contracts or is it that this new functionality will not be applied in all regions?
There was a problem hiding this comment.
@frustoo2004 @DheerendraRathor please address @shahabhijeet 's comments here. Is there a reason why the REST spec is in stable version but the Nuget package is preview?
|
@dsgouda , @shahabhijeet I have upgraded the major version. Please check. |
|
@dsgouda , @shahabhijeet , Can you please review the PR? |
|
@frustoo2004 Could you please merge the commits into a single commit and update the PR. Will merge right away |
|
I have squashed the commits to a single one. Please merge this PR. |
|
@dsgouda Please merge the PR. |
4 participants
Description
This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines
Testing Guidelines
SDK Generation Guidelines
*.csprojandAssemblyInfo.csfiles have been updated with the new version of the SDK.