Container instances/groups management client#3580
Container instances/groups management client#3580ScottHolden wants to merge 4 commits intoAzure:psSdkJson6from ScottHolden:containerInstances
Conversation
|
Can one of the admins verify this patch? |
1 similar comment
|
Can one of the admins verify this patch? |
|
@ScottHolden, |
|
@sauryadas @rgardler Can one of you review this PR for Container instance. |
|
@ScottHolden can you point me to the REST API spec that you had the PR opened? |
|
Hi @shahabhijeet , The .NET SDK did not have this client, thus it was created from scratch. Let me know if you need any more info :) |
|
@sauryadas can you approve this PR? |
| { | ||
| public const string DefaultTestPrefix = "defaulttestprefix-"; | ||
| public const string DefaultResourceGroupPrefix = "testresourcegroup-"; | ||
| public const string DefaultLocationId = "japaneast"; |
There was a problem hiding this comment.
public const string DefaultLocationId = "japaneast"; [](start = 2, length = 52)
@scott@nullops.io No need. Just use location.
| public const string DefaultTestPrefix = "defaulttestprefix-"; | ||
| public const string DefaultResourceGroupPrefix = "testresourcegroup-"; | ||
| public const string DefaultLocationId = "japaneast"; | ||
| public const string DefaultLocation = "Japan East"; |
There was a problem hiding this comment.
"Japan East"; [](start = 40, length = 13)
Is it possible to use "westus" instead?
| { | ||
| throw new KeyNotFoundException(string.Format("Generated name not found for calling method: {0}", methodName), e); | ||
| } | ||
| } |
There was a problem hiding this comment.
Can you just use TestUtilities.GenerateName
| ResourceGroup resourceGroup = context.CreateResourceGroup("containergroupcrd-", containerInstanceLocation); | ||
| ContainerInstanceManagementClient containerClient = context.GetClient<ContainerInstanceManagementClient>(); | ||
|
|
||
| string containerGroupName = context.GenerateName("containergroup"); |
There was a problem hiding this comment.
context.GenerateName [](start = 32, length = 20)
Just use TestUtilities.GenerateName
| string containerGroupName = context.GenerateName("containergroup"); | ||
| string containerOsType = "Linux"; | ||
|
|
||
| string containerName = "test1"; |
There was a problem hiding this comment.
"test1" [](start = 27, length = 7)
Can you also use TestUtilities.GenerateName to generate a random name?
| ContainerInstanceManagementClient containerClient = context.GetClient<ContainerInstanceManagementClient>(); | ||
|
|
||
| string containerGroupName = context.GenerateName("containergroup"); | ||
| string containerOsType = "Linux"; |
There was a problem hiding this comment.
"Linux" [](start = 29, length = 7)
Use OperatingSystemTypes.Linux
| Assert.NotNull(createdContainerGroup); | ||
| Assert.Equal(containerGroupName, createdContainerGroup.Name); | ||
| Assert.Equal(containerOsType, createdContainerGroup.OsType); | ||
| Assert.Equal(1, createdContainerGroup.Containers.Count); |
There was a problem hiding this comment.
Also verify the there is public IP address assigned.
| Assert.Equal(1, getContainer.Ports.Count); | ||
| Assert.Equal(containerPort, getContainer.Ports.First().Port); | ||
| Assert.Equal(containerCpu, getContainer.Resources.Requests.Cpu); | ||
| Assert.Equal(containerMemoryInGB, getContainer.Resources.Requests.MemoryInGB); |
There was a problem hiding this comment.
Wrap all the assertion into a helper method.
| IPage<ContainerGroup> listSubContainerGroup = containerClient.ContainerGroups.List(); | ||
| Assert.NotNull(listRgContainerGroup); | ||
| Assert.True(listRgContainerGroup.Count() >= 1); | ||
| Assert.True(listRgContainerGroup.Any(x => containerGroupID.Equals(x?.Id))); |
There was a problem hiding this comment.
Create a test case in which you create 2 container group. When listing all, you should get both. When listing by resource group, you should only get one.
| Assert.Equal(containerMemoryInGB, createdContainer.Resources.Requests.MemoryInGB); | ||
|
|
||
| // Wait till the container group is ready before proceeding | ||
| ContainerGroupUtilities.WaitTillProvisioningStateSucceeded(containerClient, resourceGroup.Name, containerGroupName); |
There was a problem hiding this comment.
Not really necessary. You can call get or list before reach succeeded provisioning state.
|
|
||
| namespace ContainerInstance.Tests.Utilities | ||
| { | ||
| class ContainerInstanceTestUtilities |
There was a problem hiding this comment.
ContainerInstanceTestUtilities [](start = 10, length = 30)
Seems not used anywhere? Remove this class?
| <PackageReference Include="Microsoft.Extensions.Configuration.FileExtensions" Version="1.1.2" /> | ||
| <PackageReference Include="Microsoft.Extensions.Configuration.Json" Version="1.1.2" /> | ||
| <PackageReference Include="WindowsAzure.Storage" Version="8.1.4" /> | ||
| </ItemGroup> |
|
|
||
| string containerGroupName = context.GenerateName("containergroup"); | ||
| string containerOsType = "Linux"; | ||
| string containerName = "test1"; |
There was a problem hiding this comment.
Similar to the other scenario tests. Use built in function to generate name.
yolocs
left a comment
yolocs
left a comment
There was a problem hiding this comment.
Hi Scott, thanks for contributing. Left several comments.
shahabhijeet
left a comment
shahabhijeet
left a comment
There was a problem hiding this comment.
@ScottHolden thank you for your valuable contribution. This is helpful.
Please make changes suggested in the PR
| <TargetFrameworks>netcoreapp1.1</TargetFrameworks> | ||
| </PropertyGroup> | ||
| <ItemGroup> | ||
| <Compile Remove="Utilities\ContainerInstanceTestUtilities.cs" /> |
There was a problem hiding this comment.
@ScottHolden this is not needed. I know VS might have added it, can you remove it. VS will include code files by default in all sub directories.
| </ItemGroup> | ||
|
|
||
| <ItemGroup> | ||
| <Folder Include="SessionRecords\" /> |
There was a problem hiding this comment.
@ScottHolden can you clean up these itemgroup, you do not need a separate directory itemgroup for SessionRecords
|
@ScottHolden this PR has not addressed the feedback and the below PR is ready to be merged |
5 participants
Generated the ContinerInstances client along with a base set of scenario tests.
This rest api spec was already merged, so generated it straight off there :)
This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines
Testing Guidelines
SDK Generation Guidelines
*.csprojandAssemblyInfo.csfiles have been updated with the new version of the SDK.