Skip to content

Conversation

@jaczhan
Copy link
Contributor

@jaczhan jaczhan commented May 10, 2017

This is adding test for new Swagger AzureRmSqlServerActiveDirectoryAdministrator API for ActiveDirectoryOperations include:

  1. Create/Update ActiveDirectory Admin
  2. Get ActiveDirectory Admin
  3. Delete ActiveDirectory Admin
  4. Get list of ActiveDirectory Admin

Swagger PR at: Azure/azure-rest-api-specs#1203

@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

1 similar comment
@azuresdkci
Copy link
Contributor

Can one of the admins verify this patch?

@msftclas
Copy link

@jaczhan,
Thanks for your contribution as a Microsoft full-time employee or intern. You do not need to sign a CLA.
Thanks,
Microsoft Pull Request Bot

@jaczhan jaczhan requested a review from nathannfan May 10, 2017 18:48
Assert.Equal(aadAdmin, getResult.Login);
Assert.Equal(new Guid("5e90ef3b-9b42-4777-819b-25c36961ea4d"), getResult.Sid);
Assert.Equal(new Guid("72f988bf-86f1-41af-91ab-2d7cd011db47"), getResult.TenantId);

Copy link
Contributor

Choose a reason for hiding this comment

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

Add a check for list here too?

Copy link
Contributor

@nathannfan nathannfan left a comment

Choose a reason for hiding this comment

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

Looks good to me if it passes validation.

<Service Include="{82a7f48d-3b50-4b1e-b82e-3ada8210c358}" />
</ItemGroup>
<ItemGroup>
<PackageReference Update="Microsoft.Rest.ClientRuntime.Azure" Version="3.3.5" />
Copy link
Contributor

Choose a reason for hiding this comment

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

@jaczhan you don't need to add these references, they get implicitly added.

{
public class AzureRmSqlServerActiveDirectoryAdministratorTest
{
[Fact]
Copy link
Contributor

Choose a reason for hiding this comment

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

@jaczhan please record test using latest nugets (e.g. 1.5.0 version of Resource manager) Currently you are using 1.1.0-preview version of Resource Manager.
All new tests should be using latest version of Resource Manager.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

how do I switch to the latest nugets?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

just updated

<TargetFrameworks>net452;netstandard1.4</TargetFrameworks>
</PropertyGroup>
<ItemGroup>
<PackageReference Include="Microsoft.Azure.Management.ResourceManager" Version="1.6.0-preview" />
Copy link
Contributor

Choose a reason for hiding this comment

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

@jaczhan any reason you need this nuget in your SQL package?
The requirement was only for recording new test, so your other change is all you need.
You don't want additional dependency on your SQL SDK package.
Please revert "this" particular change.
Please keep your other change that you made in the test project (that is what I had asked for)

</ItemGroup>
</Project> No newline at end of file
<ItemGroup>
<PackageReference Update="Microsoft.Rest.ClientRuntime.Azure.TestFramework" Version="1.6.0" />
Copy link
Contributor

@shahabhijeet shahabhijeet May 11, 2017

Choose a reason for hiding this comment

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

@jaczhan you do not need this, this is being added to every project.
Any reason you had to add reference to TestFramework?
You can remove this package reference.

Copy link
Contributor Author

@jaczhan jaczhan left a comment

Choose a reason for hiding this comment

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

remove Resource Manager 1.6

@shahabhijeet
Copy link
Contributor

@jaczhan add to whitelist

@jaczhan
Copy link
Contributor Author

jaczhan commented May 11, 2017

Adding test for Swagger AzureRmSqlServerActiveDirectoryAdministrator

Azure/azure-rest-api-specs#1203

@jaczhan jaczhan changed the title Add test for AzureRmSqlServerActiveDirectoryAdministratorTest Add test for AzureRmSqlServerActiveDirectoryAdministrator API. https://github.com/Azure/azure-rest-api-specs/pull/1203 May 11, 2017
@jaczhan jaczhan changed the title Add test for AzureRmSqlServerActiveDirectoryAdministrator API. https://github.com/Azure/azure-rest-api-specs/pull/1203 Add test for AzureRmSqlServerActiveDirectoryAdministrator API May 11, 2017
Copy link
Contributor Author

@jaczhan jaczhan left a comment

Choose a reason for hiding this comment

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

changes made to fix 1.6 tag

@alvadb alvadb removed the Fluent label May 18, 2017
@shahabhijeet shahabhijeet changed the base branch from vs17Dev to psSdkJson6 May 22, 2017 20:12
@jaredmoo
Copy link
Contributor

We will rerecord the tests separately - #3267

@shahabhijeet
Copy link
Contributor

@jaczhan looks good, as soon as CI passes this can be merged.

@shahabhijeet shahabhijeet merged commit b59cca5 into Azure:psSdkJson6 May 23, 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.

7 participants