Skip to content

Conversation

@panchagnula
Copy link
Contributor

@panchagnula panchagnula commented Aug 22, 2018

Description

New .NET SDK generated for Websites extension.
Swagger PR Azure/azure-rest-api-specs#3692


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.

NuGet.Config Outdated
Copy link
Contributor

Choose a reason for hiding this comment

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

Please undo these changes

Copy link
Contributor

Choose a reason for hiding this comment

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

Please pull latest changes from remote and run msbuild build.proj and regenerate the code

Copy link
Contributor

Choose a reason for hiding this comment

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

Looks like the API version is being bumped. Please bump the major version here
Also update the PackageReleaseNotes and AssemblyInfo.cs accordingly

@dsgouda
Copy link
Contributor

dsgouda commented Aug 22, 2018

@panchagnula also fix the CI build failures

@panchagnula
Copy link
Contributor Author

@dsgouda - feedback addressed

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.

It looks like the REST API version is stable, is there a reason you wish to publish preview Nuget packages.
We generally recommend publishing stable Nuget packages for stable REST API versions

@@ -19,5 +19,8 @@

<!-- Please do not move/edit code below this line -->
<Import Condition=" Exists('$([MSBuild]::GetPathOfFileAbove(AzSdk.RP.props))') " Project="$([MSBuild]::GetPathOfFileAbove('AzSdk.RP.props'))" />
<ItemGroup>
<PackageReference Update="Microsoft.Rest.ClientRuntime.Azure" Version="3.3.15" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Please remove this

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@dsgouda had to add this due to a bug in .NET SDK with provisioning state polling code . see the email below for details
@shahabhijeet as FYI as well.

Can you update to the latest version of Microsoft.Rest.ClientRuntime.Azure and that should do the fix for that particular error.

Abhijeet

From: Nicholas King
Sent: Wednesday, August 22, 2018 3:43 PM
To: Abhijeet Shah
Hi Abhijeet,

Sisira and I are working on updating the Web Apps SDK right now, and we’ve gotten stuck recording some of our tests. I dug through the code and noticed you’ve been working on the provisioning state polling code. Hopefully you’ll be able to help us out.

We have a test that creates an AppServicePlan (a.k.a. ServerFarm internally), then updates the AppServicePlan. Both creating and updating are done with PUT requests to the same URL. When creating the AppServicePlan the response contains provisioningState: Succeeded, but when updating it, the response contains provisioningState: null. The internal SDK Client is throwing an exception “Provisioning state is missing from long running operation.”

I attached a recording of the failing test. My main question for now is, is there an issue with how our API currently works, or is the problem fixable inside the .NET SDK?

Thanks
Nick

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for confirming. We need to take a look at this and decide if we can allow a one off here or if we are ready to move our ClientRuntime dependency to the latest version for all sdks.

Copy link
Contributor

Choose a reason for hiding this comment

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

@panchagnula Please revert this change, we are planning to bump the version dependency for all SDKs across the repo, will merge this PR once that is done.

@@ -9,7 +9,7 @@
[assembly: AssemblyDescription("Provides management functionality for Microsoft Azure Web Sites.")]

[assembly: AssemblyVersion("1.0.0")]
[assembly: AssemblyFileVersion("1.8.0")]
[assembly: AssemblyFileVersion("1.9.0")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Since you are bumping the major version, both AssemblyFileVersion and AssemblyVersion must be bumped to 2.0.0.0

Copy link
Contributor Author

Choose a reason for hiding this comment

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

will do.

@panchagnula
Copy link
Contributor Author

@dsgouda regarding "is there a reason you wish to publish preview Nuget packages.
We generally recommend publishing stable Nuget packages for stable REST API versions" - I am not sure about the history for this SDK being in preview - I just updated the version, following up some folks on my team who might have context on our SDK version being in preview. CC:// @naveedaz

@dsgouda
Copy link
Contributor

dsgouda commented Aug 23, 2018

Will wait from an approval from @naveedaz regarding SDK Nuget versioning

@naveedaz
Copy link
Contributor

Updating to non preview version should be fine.

@panchagnula
Copy link
Contributor Author

@dsgouda , the related rest-api-specs change got merged, please let me know if this is ready to be published as well or if you need anything else from our end. Thank you.

@dsgouda
Copy link
Contributor

dsgouda commented Aug 28, 2018

@panchagnula Will address/merge this today

@dsgouda
Copy link
Contributor

dsgouda commented Aug 28, 2018

Waiting on clientruntime dependencies to bumped across the repo

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants