Skip to content

[Billing RP] Add Enrollment Accounts APIs + tests.#4154

Merged
dsgouda merged 7 commits intoAzure:psSdkJson6from
wilcobmsft:wilcob/enrollmentAccounts
Mar 22, 2018
Merged

[Billing RP] Add Enrollment Accounts APIs + tests.#4154
dsgouda merged 7 commits intoAzure:psSdkJson6from
wilcobmsft:wilcob/enrollmentAccounts

Conversation

@wilcobmsft
Copy link
Member

Description

Generating a new Billing SDK based on Azure/azure-rest-api-specs#2626, to add support for enrollment accounts.

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.

2. Update existing session records to reflect updated api-version
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 create a generate.ps1 files similar to this and generate the code again using it


[assembly: AssemblyVersion("2.0.0.0")]
[assembly: AssemblyFileVersion("2.0.0.0")]
[assembly: AssemblyVersion("2.1.0.0")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not update the AssemblyVersion

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted


[assembly: AssemblyVersion("2.0.0.0")]
[assembly: AssemblyFileVersion("2.0.0.0")]
[assembly: AssemblyVersion("2.1.0.0")]
Copy link
Contributor

Choose a reason for hiding this comment

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

Please do not update the AssemblyVersion

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted

<PropertyGroup>
<Description>Microsoft Azure Billing Management Library</Description>
<Version>2.0.0-preview</Version>
<Version>2.1.0-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.

We highly recommend adding PackageReleaseNotes

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

<Authors>Microsoft Corporation</Authors>
<AssemblyName>Billing.Tests</AssemblyName>
<VersionPrefix>2.0.0-preview</VersionPrefix>
<VersionPrefix>2.1.0-preview</VersionPrefix>
Copy link
Contributor

Choose a reason for hiding this comment

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

You don't need to update the package version for test project since they are never published. Also consider simply setting it to 2.0.0 instead

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@dsgouda dsgouda self-assigned this Mar 21, 2018
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.

Still missing the generate.ps1 file

{
}

#if LEGACY
Copy link
Contributor

Choose a reason for hiding this comment

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

Was this handwritten code by any chance? Please check if this change was intended

Copy link
Member Author

Choose a reason for hiding this comment

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

@dsgouda Not handwritten - I ran generated.cmd.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see that the newer code is generated by generate.cmd. I am concerned about the code that already exists and is being overwritten here.

Copy link
Member Author

Choose a reason for hiding this comment

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

@dsgouda What options do I have?

Copy link
Member Author

Choose a reason for hiding this comment

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

@dsgouda This is simply a matter of using a newer version of AutoRest. The current generated code appears to match that of other SDKs.

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

@dsgouda
Copy link
Contributor

dsgouda commented Mar 22, 2018

@wilcobmsft Please generate the code using generate.ps1 and commit the 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

@dsgouda dsgouda merged commit 8043406 into Azure:psSdkJson6 Mar 22, 2018
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.

2 participants