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.

Sign up for GitHub

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

Jump to bottom

Updated Management.ServiceFabric for API version 2018-02-01 #4293

Merged
merged 1 commit into from
May 18, 2018
Merged

Updated Management.ServiceFabric for API version 2018-02-01 #4293

merged 1 commit into from
May 18, 2018

Conversation

juhacket
Copy link
Contributor

@juhacket juhacket commented May 7, 2018 •
edited
Loading

Description

Azure/azure-rest-api-specs#2844

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.

dsgouda
dsgouda suggested changes May 8, 2018
View reviewed changes
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 fix the test failures reported in CI

src/SDKs/ServiceFabric/ServiceFabric.Test/ServiceFabric.Test.csproj Outdated
@@ -4,7 +4,7 @@
<PackageId>ServiceFabric.Test</PackageId>
<Description>ServiceFabric Tests Class Library</Description>
<AssemblyName>ServiceFabric.Test</AssemblyName>
<VersionPrefix>1.0.0-preview</VersionPrefix>
<VersionPrefix>1.0.2</VersionPrefix>
Copy link
Contributor

Choose a reason for hiding this comment

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

This is never published, please set this to 1.0.0

src/SDKs/ServiceFabric/Management.ServiceFabric/Microsoft.Azure.Management.ServiceFabric.csproj
@@ -7,7 +7,7 @@
<PackageId>Microsoft.Azure.Management.ServiceFabric</PackageId>
<Description>Microsoft Azure Management ServiceFabric Library</Description>
<AssemblyName>Microsoft.Azure.Management.ServiceFabric</AssemblyName>
<Version>1.0.1-preview</Version>
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 breaking changes are being introduced here, please set version to 1.1.0
Also, if this is a stable release, please update the PackageReleaseNotes

Copy link
Contributor Author

Choose a reason for hiding this comment

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

There should be no breaking changes.

src/SDKs/_metadata/servicefabric_resource-manager.txt
@@ -0,0 +1,11 @@
2018-05-07 20:57:23 UTC

Copy link
Contributor

Choose a reason for hiding this comment

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

Please create a generate.ps1 file similar to this and regenerate the code

@dsgouda
Copy link
Contributor

dsgouda commented May 15, 2018

@juhacket Please address the test failures

dsgouda
dsgouda reviewed May 15, 2018
View reviewed changes
src/SDKs/ServiceFabric/Management.ServiceFabric/Generated/ClustersOperations.cs
/// <return>
/// A response object containing the response body and response headers.
/// </return>
public async Task<AzureOperationResponse<IPage<Cluster>>> ListNextWithHttpMessagesAsync(string nextPageLink, Dictionary<string, List<string>> customHeaders = null, CancellationToken cancellationToken = default(CancellationToken))
Copy link
Contributor

Choose a reason for hiding this comment

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

@juhacket Removing these methods means a breaking change to the end users using the current package version.
@shahabhijeet can you confirm

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 Pushed too soon, missed a file.

dsgouda
dsgouda reviewed May 15, 2018
View reviewed changes
src/SDKs/ServiceFabric/Management.ServiceFabric/Generated/ClusterVersionsOperations.cs
@@ -78,7 +289,7 @@ internal ClusterVersionsOperations(ServiceFabricManagementClient client)
/// <return>
/// A response object containing the response body and response headers.
/// </return>
public async Task<AzureOperationResponse<IPage<ClusterCodeVersionsResult>>> ListWithHttpMessagesAsync(string location, string environment, Dictionary<string, List<string>> customHeaders = null, CancellationToken cancellationToken = default(CancellationToken))
public async Task<AzureOperationResponse<ClusterCodeVersionsListResult>> GetByEnvironmentWithHttpMessagesAsync(string location, string environment, string clusterVersion, Dictionary<string, List<string>> customHeaders = null, CancellationToken cancellationToken = default(CancellationToken))
Copy link
Contributor

Choose a reason for hiding this comment

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

@juhacket Still looks like renaming a method, looks like this can affect current users

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, we changed the model for our paged objects. I have updated the version to reflect the intent to make this breaking change.

@juhacket juhacket force-pushed the psSdkJson6 branch 2 times, most recently from 3da3f12 to 2d1629b Compare May 15, 2018 23:56
dsgouda
dsgouda suggested changes May 16, 2018
View reviewed changes
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 apart from a minor comment

src/SDKs/ServiceFabric/Management.ServiceFabric/Microsoft.Azure.Management.ServiceFabric.csproj Outdated
@@ -7,7 +7,7 @@
<PackageId>Microsoft.Azure.Management.ServiceFabric</PackageId>
<Description>Microsoft Azure Management ServiceFabric Library</Description>
<AssemblyName>Microsoft.Azure.Management.ServiceFabric</AssemblyName>
<Version>1.0.1-preview</Version>
<Version>1.1.0</Version>
<PackageTags>Microsoft Azure Management Library;ServiceFabric;ServiceFabric management;</PackageTags>
<PackageReleaseNotes/>
Copy link
Contributor

Choose a reason for hiding this comment

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

We highly recommend adding PackageReleaseNotes since it helps developers using nuget
You can find an example here

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 Added PackageReleaseNotes section.

dsgouda
dsgouda approved these changes May 16, 2018
View reviewed changes
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 CIs passing
Thanks @juhacket

@juhacket
Copy link
Contributor Author

@dsgouda Can you remove the Changes Requested tag so this will get merged?

@dsgouda dsgouda merged commit 260fab8 into Azure:psSdkJson6 May 18, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dsgouda dsgouda dsgouda approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@juhacket @dsgouda