-
Notifications
You must be signed in to change notification settings - Fork 4.9k
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
Adding LogAnalytics API. #4035
Adding LogAnalytics API. #4035
Conversation
@@ -17,6 +17,12 @@ | |||
<ProjectReference Include="..\Management.Compute\Microsoft.Azure.Management.Compute.csproj" /> | |||
</ItemGroup> | |||
|
|||
<ItemGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@hyonholee please use package reference and not reference
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason you are not using the package reference?
@@ -57,7 +57,11 @@ public void TestContainerServiceUpdateOperations() | |||
ValidateContainerService(inputContainerService, containerService); | |||
|
|||
var listResult = m_CrpClient.ContainerServices.ListByResourceGroup(rgName); | |||
#if NET46 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As we do not support .NET 46, this piece of code will never be executed.
Any reason for adding .NET 46 moniker?
@@ -33,6 +33,26 @@ private void TestVMScaleSetServiceFabricImpl(MockContext context) | |||
string vmssName = "crptesthtn39hve"; | |||
|
|||
var response = m_CrpClient.VirtualMachineScaleSets.ForceRecoveryServiceFabricPlatformUpdateDomainWalk(rgName, vmssName, 0); | |||
|
|||
#if NET46 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Your test is never built against .NET 46, so this is essentially dead code.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@shahabhijeet Yes. This is a dead code, but in our repo, we build tests using NET46 because moving to netcore is not done in our repo. Since we copy test codes from our internal repo to github, we need to change it every time. That is why I added this part.
@shahabhijeet MGMT SDKs in other languages than C# do not nee to submit their SDK code and test changes. Can you help explain why is that? Or can you also waive C# for this part. Thanks. |
@@ -17,6 +17,12 @@ | |||
<ProjectReference Include="..\Management.Compute\Microsoft.Azure.Management.Compute.csproj" /> | |||
</ItemGroup> | |||
|
|||
<ItemGroup> |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any reason you are not using the package reference?
3214853
to
769ecf4
Compare
Azure/azure-rest-api-specs#2383
Description
This checklist is used to make sure that common guidelines for a pull request are followed.
General Guidelines
Testing Guidelines
SDK Generation Guidelines
*.csproj
andAssemblyInfo.cs
files have been updated with the new version of the SDK.