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

[Cogs Face] Update Face SDK with latest auto-rest Swagger and update unit tests #3977

Merged
merged 1 commit into from
Jan 25, 2018
Merged

[Cogs Face] Update Face SDK with latest auto-rest Swagger and update unit tests #3977

merged 1 commit into from
Jan 25, 2018

Conversation

lebronJ
Copy link
Contributor

@lebronJ lebronJ commented Jan 4, 2018 •
edited
Loading

This has been internally reviewed (lebronJ#1) inside Face SDK team.

shahabhijeet
shahabhijeet suggested changes Jan 4, 2018
View reviewed changes
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 sync with David li in case you need more information about recording tests. If not let me know if you have any questions. I am tagging David on this PR

...Ks/CognitiveServices/dataPlane/Vision/Vision/Microsoft.Azure.CognitiveServices.Vision.csproj Outdated
@@ -3,7 +3,7 @@
<PropertyGroup>
<PackageId>Microsoft.Azure.CognitiveServices.Vision</PackageId>
<Description>This client library provides access to the Microsoft Cognitive Services Vision APIs.</Description>
<VersionPrefix>1.0.1-preview</VersionPrefix>
<VersionPrefix>1.0.2-preview</VersionPrefix>
Copy link
Member

Choose a reason for hiding this comment

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

@lebronJ please model your csproj according to the below project.
https://github.com/Azure/azure-sdk-for-net/blob/psSdkJson6/src/SDKs/CognitiveServices/dataPlane/Search/BingCustomSearch/BingCustomSearch/Microsoft.Azure.CognitiveServices.Search.CustomSearch.csproj
you are missing imports at the bottom of the project file and also you don't need few project properties that you have added.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Refined this csproj following the styles of above BingCustomSearch

src/SDKs/CognitiveServices/dataPlane/Vision/Vision.Tests/Face/BaseTests.cs Outdated
{
// Retrieve the configuration information.
FaceSubscriptionKey = "";
Environment.SetEnvironmentVariable("AZURE_TEST_MODE", "Playback");
Copy link
Member

Choose a reason for hiding this comment

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

@lebronJ you do not set this in test case, rather you set this in your connection string.
you use test framework to record tests
https://github.com/Azure/azure-powershell/blob/preview/documentation/Using-Azure-TestFramework.md#manually-set-environment-variables

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Removed this unnecessary setting
Environment.SetEnvironmentVariable("AZURE_TEST_MODE", "Playback");

@shahabhijeet
Copy link
Member

@DavidLiCIG can you review the test part of this PR

@lebronJ
Copy link
Contributor Author

lebronJ commented Jan 7, 2018

@shahabhijeet Updated following your comments.
If this is fine to merge, could you please help share how to release this updated Nuget package? Thanks.

@lebronJ lebronJ closed this Jan 7, 2018
@lebronJ lebronJ reopened this Jan 7, 2018
@DavidLiCIG
Copy link
Contributor

This package should be released as an update to Microsoft.Azure.CognitiveServices.Vision package which is already on version 1.0.1.preview.

shahabhijeet
shahabhijeet suggested changes Jan 9, 2018
View reviewed changes
...Ks/CognitiveServices/dataPlane/Vision/Vision/Microsoft.Azure.CognitiveServices.Vision.csproj Outdated
<Folder Include="Generated\Face\Models\" />
<Folder Include="Properties\" />
</ItemGroup>
<Import Condition=" Exists('$([MSBuild]::GetPathOfFileAbove(AzSdk.RP.props))') " Project="$([MSBuild]::GetPathOfFileAbove('AzSdk.RP.props'))" />
Copy link
Member

@shahabhijeet shahabhijeet Jan 9, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

@lebronJ add the following comment above/below line 2 and line 16
https://github.com/Azure/azure-sdk-for-net/blob/psSdkJson6/src/SDKs/AnalysisServices/Management.Analysis/Microsoft.Azure.Management.Analysis.csproj#L2

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done

@lebronJ
Copy link
Contributor Author

lebronJ commented Jan 11, 2018

@shahabhijeet Updated following your comments. Any updates? Thanks.

@shahabhijeet
Copy link
Member

Have you used generate.cmd to generate your SDK? Using generate.cmd is essential to create the meta data file (it's a .txt file that has information about from what this SDK was generated)
I see your missing that file.

@lebronJ
Copy link
Contributor Author

lebronJ commented Jan 12, 2018

@shahabhijeet Updated by regenerating SDK with generate.cmd. Thanks for your comments and sorry for not noticing this before.

shahabhijeet
shahabhijeet suggested changes Jan 16, 2018
View reviewed changes
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 add your vision sdk under this directory
SDKs\CognitiveServices\dataPlane\Vision\VisionSdk\VisionSDKs\CognitiveServices\dataPlane\Vision\VisionSdk\Vision.Tests

This helps in identifying tests for your sdk.
This aligns with all the other SDKs that are checked in, e.g. ContentModerator

@lebronJ
Copy link
Contributor Author

lebronJ commented Jan 22, 2018

@shahabhijeet I have updated this PR by separating Computer Vision / Face SDKs and aligning them with ContentModerator and other Cogs SDKs using the same folder structure.
SDKs are regenerated by generate.cmd after adjusting the readme.md in Azure/azure-rest-api-specs#2305.

shahabhijeet
shahabhijeet suggested changes Jan 23, 2018
View reviewed changes
Copy link
Member

@shahabhijeet shahabhijeet left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Please remove CognitiveServices/dataPlane/Vision/Vision/Generated/Face
I believe those files were left when you moved the files.

At this point you are better off create a new PR, this PR is already riddled with lot of commits.
It's better you create a new PR with all these changes with minimum commits

@lebronJ
Regenerate Face/ComputerVision SDK with latest spec and update the un…
37d38ac 
…it tests, separate Face/CV aligning with other Vision APIs.
@lebronJ
Copy link
Contributor Author

lebronJ commented Jan 23, 2018 •
edited
Loading

Thanks @shahabhijeet , I removed the unused Vision SDK folder as you commented.
Considering creating a new PR with minimal commits, I think it's not straightforward to separate this PR, because this PR only does two interdependent things:

  1. Regenerate Face & CV SDK with latest spec in [Cogs Face] Adjust SDK generated target folder for Face API and Computer Vision API under Vision folder azure-rest-api-specs#2305, and update unit tests based on the SDK changes.
  2. Separate Face & CV SDKs aligning with other Vision SDKs.

So I rebased this PR to latest commit and force pushed it as one single commit.

@shahabhijeet shahabhijeet merged commit c95d9ea into Azure:psSdkJson6 Jan 25, 2018
@lebronJ
Copy link
Contributor Author

lebronJ commented Jan 25, 2018

Hi @shahabhijeet , could you please help share how to release this updated Nuget package? Thanks.
(Previously https://www.nuget.org/packages/Microsoft.Azure.CognitiveServices.Vision/)

One thing to note:
As this PR totally separated Computer Vision and Face into two independent SDKs, I think we should release TWO Nuget packages Microsoft.Azure.CognitiveServices.Vision.ComputerVision and Microsoft.Azure.CognitiveServices.Vision.Face.

@DavidLiCIG
Copy link
Contributor

Hi @lebronJ you can use this job to publish the new Nuget packages.

https://azuresdkci.westus2.cloudapp.azure.com/view/NetCore-SDK/job/NetCore-SDK-Publish/

@lebronJ
Copy link
Contributor Author

lebronJ commented Jan 25, 2018

Thanks @DavidLiCIG , published.

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

@shahabhijeet shahabhijeet shahabhijeet approved these changes

@DavidLiCIG DavidLiCIG DavidLiCIG 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.
3 participants
@lebronJ @shahabhijeet @DavidLiCIG