Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PowerShell .NET SDK Upgrade With V2020-02-02 ShortTermRetention (STR) of SQL DB. #13782

Conversation

lululilliancoding
Copy link
Contributor

@lululilliancoding lululilliancoding commented Jul 28, 2020

Generate new version of .NET SDK for new API V2020-02-02-Preview ShortTermRetentionPolicy.

  1. Related Unit tests don't passed since backend development not fully finished and API not deployed to live stage.
  2. This PR read STR API swagger from feature branch rather than master.

TODO:

  1. After SQL DB API specification (Add V2020-02-02-Preview ShortTermRetention API Specification of SQL DB azure-rest-api-specs#9407) merged into master, re-generate clients. (../src/Generated...) DONE

  2. After API deployed to Stage, re-run unit tests and re-record its test sessions. (../tests/SessionRecords/ShortTermRetentionTests/..)

  3. Sync with latest master and fix a new sdk version. (.../Microsoft.Azure.Management.Sql.csproj, .../AssemblyInfo.cs)

We want to start code review early before above TODO items completed.

So please ignore TODO items for now and leave comments on this PR. You don't have to approve at this moment.

1) Unit testst not passed (Backend MS changes not finished yet). TODO:Re-run tests and re-copy test session.
2) STR API swagger in feature branch rather than master. TODO: Re-generate clients.
@lululilliancoding
Copy link
Contributor Author

lululilliancoding commented Aug 6, 2020

@isra-fel The auto generate cmd can generate more unrelated changes. Some files I can undo them so they will not included in PR but some files have both mine and other unrelated changes so I can't undo them. And the auto generated file looks like can't be modified manually, then what should I do?

//
// Code generated by Microsoft (R) AutoRest Code Generator.
// Changes may cause incorrect behavior and will be lost if the code is
// regenerated.
//

Last time since I know the client generated from feature branch rather than master swagger branch, so I manually edit some files. But now, since I generated clients via swagger master branch, so I suppose I can't manually fix files right?

For example in SdkInfo_SqlManagementClient.cs, if both of them were modified, the Capabilities is not related my changes, but the BackupSTRPolicies related my changes. What should I do?

            new Tuple<string, string, string>("Sql", "BackupShortTermRetentionPolicies", "2020-02-02-preview"),
            new Tuple<string, string, string>("Sql", "Capabilities", "2018-06-01-preview"),

I temp keep previous changes and not include any unrelated changes. Please let me know if this is not correct way.

@isra-fel
Copy link
Member

isra-fel commented Aug 7, 2020

@lululilliancoding never edit generated code.
Basically, if you wish to control what are generated and what are not, edit the tag defined in readme.md.

I suggest you talk to who contributed to last release, and define what exactly you want to put in the SDK.
(Removing an operation is a breaking change, and we want to avoid breaking change if possible, even for a preview package).

@lululilliancoding
Copy link
Contributor Author

lululilliancoding commented Aug 7, 2020

@lululilliancoding never edit generated code.
Basically, if you wish to control what are generated and what are not, edit the tag defined in readme.md.

I suggest you talk to who contributed to last release, and define what exactly you want to put in the SDK.
(Removing an operation is a breaking change, and we want to avoid breaking change if possible, even for a preview package).

I edit my change to readme.md, see: Tag: package-composite-v3 - Microsoft.Sql/preview/2020-02-02-preview/shortTermRetentionPolicies.json, I generate client files via master branch of azure-rest-api-specs so does there are many un-related clients generated. How can I avoid them without manually fix auto generated files?

@lululilliancoding
Copy link
Contributor Author

lululilliancoding commented Aug 10, 2020

Just verified with Alicia (released PS .net sdk last year), as she said it is ok to leave un-related changes but don't manually fix auto generated files. I'll update in next push.

-- I correct this, the dev manually fixed.

@isra-fel isra-fel self-assigned this Aug 14, 2020
@lululilliancoding
Copy link
Contributor Author

lululilliancoding commented Sep 2, 2020

Why this PR1 and PR2 can only include their related changes on file SqlManagementClient.cs?
When I run cmd to auto generate, there are too many changes not related with the new API I added. I don't think it make sense to include other swaggers changes into my PR. I synced with these authors and they either manually edit sdk of these auto generated files. Manually fix might be a risky way for fixing this problem but SQL is a big organization with many teams launching features frequent. It is not efficient if one feature waiting for other group of features to generate one sdk. I'll raise up this conversation with SQL team.

I mentioned the PR1 updated the version from 1.44.0.0 to 1.44.1.0
Should I update version to 1.45 or 1.44.2.0? What's difference?

@lululilliancoding
Copy link
Contributor Author

lululilliancoding commented Sep 3, 2020

This PR has expected build error "Sql.Tests.ShortTermRetentionTests failed" since no live API deployed to Stage yet.
I'll come back to continue.

@isra-fel
Copy link
Member

isra-fel commented Sep 4, 2020

We asked our partners not to modify generated code, but to be honest, even if they did, we won't know. That's why sometimes modified source code got checked in.

I don't think it make sense to include other swaggers changes into my PR.

I agree with you. Modifying source code seems to be the best choice for now. I do hope some day SQL team could work out a new version that is completely consistant with swagger.

Should I update version to 1.45 or 1.44.2.0? What's difference?

Technically both are OK. 1.45.0 is more preferable.
.NET SDK versions follow semantic versioning, which isn't really strict on preview versions. I think what's important is to keep it consistent in SQL SDK itself. Old releases all updated the minor version, so I'd prefer 1.45.0. cc @strehan1993

BTW since SQL is a big team, I suggest you have someone in charge of .net SDK release. It could help with more consistent versioning, stable release cadence, and reduce communication cost, resulting in better SDK quality.

@strehan1993
Copy link
Contributor

We asked our partners not to modify generated code, but to be honest, even if they did, we won't know. That's why sometimes modified source code got checked in.

I don't think it make sense to include other swaggers changes into my PR.
I agree with you. Modifying source code seems to be the best choice for now. I do hope some day SQL team could work out a new version that is completely consistant with swagger.

Should I update version to 1.45 or 1.44.2.0? What's difference?

Technically both are OK. 1.45.0 is more preferable.
.NET SDK versions follow semantic versioning, which isn't really strict on preview versions. I think what's important is to keep it consistent in SQL SDK itself. Old releases all updated the minor version, so I'd prefer 1.45.0. cc @strehan1993

BTW since SQL is a big team, I suggest you have someone in charge of .net SDK release. It could help with more consistent versioning, stable release cadence, and reduce communication cost, resulting in better SDK quality.

Totally agreed. We need a major clean up.
Also, I used 1.44.1-preview as it was suggested in the Request Changes comments.

@nisha-bhatia
Copy link
Member

@lululilliancoding Please fix merge conflicts, we can't review till this branch is up to date.

@lululilliancoding
Copy link
Contributor Author

We (server side) are waiting for the backend code deployed to cluster Stage to unblock scenario tests otherwise this PR still gonna be blocked. Will keep updated this PR once we got a live API and fix no matter conflict or new versions at that time. Thanks a lot guys.

@markcowl
Copy link
Member

@lululilliancoding I am going to close this PR. You can raise a new PR once the service-side issues are resolved.

@markcowl markcowl closed this Sep 29, 2020
@lululilliancoding
Copy link
Contributor Author

Can you reopen this PR? The backend code changes deployed to Stage already. @markcowl

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Mgmt This issue is related to a management-plane library.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants