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

Regenearte Cognitive Services Mgmt. SDK with latest Spec and AutoRest (targeting release as 2.1) #3909

Merged
merged 26 commits into from
Dec 19, 2017

Conversation

felixwa
Copy link
Contributor

@felixwa felixwa commented Dec 1, 2017

Description


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.

@felixwa felixwa changed the title Regenearte sdk 2.1 Regenearte Cognitive Services sdk with latest Spec and AutoRest (targeting release as 2.1) Dec 1, 2017
@felixwa felixwa changed the title Regenearte Cognitive Services sdk with latest Spec and AutoRest (targeting release as 2.1) Regenearte Cognitive Services Mgmt. SDK with latest Spec and AutoRest (targeting release as 2.1) Dec 1, 2017
Copy link
Member

@shahabhijeet shahabhijeet left a comment

Choose a reason for hiding this comment

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

Please link the spec PR.
Also use generate.cmd to generated sdk, this will create a meta data file (.txt)

</PropertyGroup>
<PropertyGroup>
<TargetFrameworks>net452;netstandard1.4</TargetFrameworks>
</PropertyGroup>
<ItemGroup>
<Compile Remove="Properties\AssemblyInfo.cs" />
Copy link
Member

Choose a reason for hiding this comment

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

@felixwa please remove from line 17 to 19
also remove line 24 to 27

Copy link
Contributor Author

@felixwa felixwa Dec 4, 2017

Choose a reason for hiding this comment

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

@shahabhijeet , I'm seeing CS0579 if AssemblyInfo.cs is included.

obj\Debug\netstandard1.4\Microsoft.Azure.Management.CognitiveServices.AssemblyInfo.cs(14,12,14,54): error CS0579: Duplicate 'System.Reflection.AssemblyCompanyAttribute' attribute

Can you please tell me how to mitigate this error? Should I add GenerateAssemblyInfo = false?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I talked to @DavidLiCIG and he said we can remove this AssemblyInfo.cs file so I removed it.

@@ -10,6 +10,9 @@
<PropertyGroup>
<TargetFrameworks>netcoreapp1.1</TargetFrameworks>
</PropertyGroup>
<ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

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

@felixwa please remove line 13 to 15 and line 35 to 38

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Remove and also removed AssemblyInfo.cs

@felixwa
Copy link
Contributor Author

felixwa commented Dec 4, 2017

Spec PR: Azure/azure-rest-api-specs#1981

@felixwa
Copy link
Contributor Author

felixwa commented Dec 4, 2017

The changes were generated generate.cmd. I also fixed a folder structure issue in generate.cmd in this PR.
A remaining issue with generate.cmd is that now it generates the folder under SDKs\CognitiveServices folder but it should be SDKs\CognitiveServices\management folder. @shahabhijeet if you know how to fix this I can also do that in this PR.
I know how to fix it now. Azure/azure-rest-api-specs#2103

Remove AssemblyInfo.cs file
Remove Properties folder in test proj
Copy link
Member

@shahabhijeet shahabhijeet left a comment

Choose a reason for hiding this comment

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

Missing meta data file when generated using generate.cmd.
Any reason AssemblyInfo.cs was deleted.

<AssemblyName>Microsoft.Azure.Management.CognitiveServices</AssemblyName>
<PackageTags>Microsoft Azure Cognitive Services management;Cognitive Services;Cognitive Services management</PackageTags>
<PackageReleaseNotes/>
<PackageReleaseNotes />
Copy link
Member

Choose a reason for hiding this comment

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

Can you add package release notes to specify what is new in this release.

</PropertyGroup>
<PropertyGroup>
<TargetFrameworks>net452;netstandard1.4</TargetFrameworks>
</PropertyGroup>
<ItemGroup>
<Content Include="notice.txt" />
</ItemGroup>

<ItemGroup>
Copy link
Member

Choose a reason for hiding this comment

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

@felixwa please remove this item group as it's not needed to specify explicitly about directory structure.
Makes the project xml unnecessarily large.

1. add metradata file.
2. regenerate SDK by fixing folder structure PR.
2017-12-07 03:31:20 UTC

1) azure-rest-api-specs repository information
GitHub user: felixwa
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shahabhijeet , I know this is generated by my own fork. Please take a look at this PR:Azure/azure-rest-api-specs#2103. After that PR is approved and merged I will regenerate.

@felixwa
Copy link
Contributor Author

felixwa commented Dec 7, 2017

@shahabhijeet , the reason I removed AssemblyInfo.cs is because it generates compiler error CS0579

obj\Debug\netstandard1.4\Microsoft.Azure.Management.CognitiveServices.AssemblyInfo.cs(14,12,14,54): error CS0579: Duplicate 'System.Reflection.AssemblyCompanyAttribute' attribute

Add PackageReleaseNotes
@shahabhijeet
Copy link
Member

shahabhijeet commented Dec 8, 2017

@felixwa if you simply do
msbuild build.proj it will update your build tools and then if you try to rebuild it should not give you that error.
if it still does, let me know and I would like to see it and will fix it if required. But we cannot ship sdk without AssemblyInfo
Plus resolve the below conflicts

@felixwa
Copy link
Contributor Author

felixwa commented Dec 11, 2017

@shahabhijeet , now AssemblyInfo.cs is back and conflict is resolved. I also changed AssemblyVersion and AssemblyFileVersion to 2.1. Please continue your review.

Change Assembly version to 2.1.0.0
Make ReleaseNotes one line.
Remove AzSdk.RP.props file as seems like it should be not commit
Copy link
Member

@shahabhijeet shahabhijeet left a comment

Choose a reason for hiding this comment

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

The Spec PR is merged. Please do the following:

  1. Pull latest from upstream
  2. Regenerate your SDK
  3. Msbuild build.proj to get the latest build tools
  4. Build you sdk from command line or IDE
  5. Create/update this PR.

@@ -7,8 +7,8 @@
[assembly: AssemblyTitle("Microsoft Azure Cognitive Services Management Library")]
[assembly: AssemblyDescription("Provides Microsoft Azure Cognitive Services management functions for managing Microsoft Azure Cognitive Services accounts.")]

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

Choose a reason for hiding this comment

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

@felixwa please do not update AssemblyVersion when you change the minor version of AssemblyFileVersion.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@shahabhijeet , please note there is a breaking change here (provisionState is changed to string from enum), why do not increase AssemblyVersion in this case?

felixwa and others added 5 commits December 13, 2017 11:47
Regenerated against latest spec.
Regenerate SDK again as shahabhijeet requested
Bump version to 3.0.0.0 as there is a breaking change
Add AzSdk.RP.props
@shahabhijeet shahabhijeet merged commit fd9782c into Azure:psSdkJson6 Dec 19, 2017
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