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

Generate SDK.Net from swagger 'SQL database, elastic pool, and capabilities 2017-10-01-preview', and re-recording all Sql tests #4171

Merged
merged 12 commits into from
Apr 9, 2018

Conversation

payiAzure
Copy link
Contributor

@payiAzure payiAzure commented Mar 28, 2018

Description

This PR is to generate SDK.Net for SQL from new swagger 'SQL database, elastic pool, and capabilities 2017-10-01-preview', and re-recording all SQL tests.

The merged Swagger PR link: Azure/azure-rest-api-specs#2734

This checklist is used to make sure that common guidelines for a pull request are followed.

General Guidelines

  • Title of the pull request 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 more information on cleaning up the commits in your PR, see this page.

Testing Guidelines

  • Pull request includes test coverage for the included changes.

SDK Generation Guidelines

  • If an SDK is being regenerated based on a new swagger spec, a link to the pull request containing these swagger spec changes has been included above.
  • The generate.cmd file for the SDK has been updated with the version of AutoRest, as well as the commitid of your swagger spec or link to the swagger spec, used to generate the code.
  • The *.csproj and AssemblyInfo.cs files have been updated with the new version of the SDK.

@payiAzure
Copy link
Contributor Author

@azuresdkci Can anyone from SDK-Net team review this PR? This PR is basically re-recording all sql tests to make sure the rest-api changes work.

I'm not sure why the build of this PR failed. I build the following projects locally, they all work.

  1. /azure-sdk-for-net/build.proj
  2. /azure-sdk-for-net/azure-sdk-for-net/src/SDKs/SqlManagement/Management.Sql/Microsoft.Azure.Management.Sql.csproj
  3. /azure-sdk-for-net/azure-sdk-for-net/src/SDKs/SqlManagement/SQL.Tests/SQL.Tests.csproj

@jaredmoo
Copy link
Contributor

jaredmoo commented Apr 3, 2018

https://travis-ci.org/Azure/azure-sdk-for-net/builds/360473280#L6170

DatabaseRestoreScenarioTests.cs(360,25): error CS0200: Property or indexer 'Database.Edition' cannot be assigned to -- it is read only [/home/travis/build/Azure/azure-sdk-for-net/src/SDKs/SqlManagement/Sql.Tests/Sql.Tests.csproj]
DatabaseRestoreScenarioTests.cs(361,25): error CS0200: Property or indexer 'Database.RequestedServiceObjectiveName' cannot be assigned to -- it is read only [/home/travis/build/Azure/azure-sdk-for-net/src/SDKs/SqlManagement/Sql.Tests/Sql.Tests.csproj]

I think this is due to recent checkin of #4168. Please merge from psSdkJson6 and then it should repro on your machine.

@payiAzure
Copy link
Contributor Author

@jaredmoo Thanks Jared. After merge with psSdkJson6 I could repro the build error. Fix the test and redo recording on this test. Will update the PR later.

@jaredmoo
Copy link
Contributor

jaredmoo commented Apr 4, 2018

For the record: there are some customizations that I added, the idea here is to add getters for properties that existed in the old API. Setters are generally not implemented because there are some differences in PUT behavior, so I would prefer for users to get compile errors (no setter for property) rather than weird runtime behaviour.

@jaredmoo
Copy link
Contributor

jaredmoo commented Apr 4, 2018

I think AssemblyInfo.cs and Management.Sql.csproj need version bump (compared to latest version at https://www.nuget.org/packages/Microsoft.Azure.Management.Sql). And release notes:

 - Database and ElasticPool now use Sku property for scale and tier-related properties. We have made this change in order to allow future support of autoscale, and to allow for new vCore-based editions.
    * Database.Sku has replaced Database.RequestedServiceObjectiveName and Database.Edition. Database scale can be set by setting Sku.Name to the requested service objective name (e.g. S0, P1, or GP_Gen4_1), or by setting Sku.Name to the sku name (e.g. Standard, Premium, or GP_Gen1) and set Sku.Capacity to the scale measured in DTU or vCores.
    * Database.CurrentSku has replaced Database.ServiceLevelObjetive.
    * Database.CurrentServiceObjectiveId and Database.RequestedServiceObjectiveId have been removed.
    * ElasticPool.Sku has replaced ElasticPool.Dtu. Elastic pool scale can be set by setting Sku.Name to the requested sku name (e.g. StandardPool, PremiumPool) and setting Sku.Capacity to the scale measured in DTU or vCores.
    * ElasticPool.PerDatabaseSettings has replaced ElasticPool.DatabaseDtuMin and ElasticPool.DatabaseDtuMax.
 - Database.MaxSizeBytes is now a long instead of string.
 - LocationCapabilities tree has been changed in order to support capabilities of new vCore-based database and elastic pool editions.

Copy link
Contributor

@jaredmoo jaredmoo left a comment

Choose a reason for hiding this comment

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

Needs version bump & release notes. Otherwise approved :)

@payiAzure
Copy link
Contributor Author

Hi reviewers, please review this PR. Thanks

@payiAzure
Copy link
Contributor Author

@dsgouda , @azuresdkci Can any reviewers from azuresdk team take a look at this PR? Thanks!

Copy link
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

Please pull down the latest upstream changes and run the generate.ps1 command again
Looks good other than that

@payiAzure
Copy link
Contributor Author

@dsgouda Thanks Deepak! I have pull down the latest upstream and regenerated the sdk in the latest commit. waiting for the build/CI check pass

Copy link
Contributor

@dsgouda dsgouda left a comment

Choose a reason for hiding this comment

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

LGTM subject to builds passing

@payiAzure
Copy link
Contributor Author

@dsgouda Thanks Deepak. It seems the default build always has errors not related with the changes. Does it matter? If yes, can you point me how to fix it?

@dsgouda
Copy link
Contributor

dsgouda commented Apr 6, 2018

@payiAzure I am taking a look at the errors, kindly wait for it to be resolved.

@payiAzure
Copy link
Contributor Author

@dsgouda It seems the re-tried build still had the same error. What does this error mean?

c:\workspace\NetCore-SdkCI\tools\SdkBuildTools\targets\common.targets(192,5): error MSB3073: The command "dotnet test -f netcoreapp1.1 c:\workspace\NetCore-SdkCI\src\AzureStack\FabricAdmin\Fabric.Admin.Tests\Fabric.Tests.csproj -l trx;LogFileName=c:\workspace\NetCore-SdkCI\TestResults\netCore11\Fabric.Tests.trx " exited with code -1073741502. [c:\workspace\NetCore-SdkCI\build.proj]

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.

3 participants