Closed bergano65 closed 3 years ago
No need to comment the controller methods
Sent from my Windows Phone
From: bergano65mailto:notifications@github.com Sent: 31/01/2017 21:29 To: shklar/SubMinimizermailto:SubMinimizer@noreply.github.com Cc: Maxim Shklarmailto:maximsh@microsoft.com; Mentionmailto:mention@noreply.github.com Subject: Re: [shklar/SubMinimizer] delete is performed with intelligent api retrieval (#65)
Comment parameters of all public methods? ( controller aren’t documented also)
From: Maxim Shklar [mailto:notifications@github.com] Sent: Monday, January 30, 2017 3:14 AM To: shklar/SubMinimizer SubMinimizer@noreply.github.com Cc: Evgeny Vitenberg eviten@microsoft.com; Author author@noreply.github.com Subject: Re: [shklar/SubMinimizer] delete is performed with intelligent api retrieval (#65)
@shklar requested changes on this pull request.
In Shared/AzureResourceManagerUtil.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489486782&sdata=Cr3780Gt40RD4VTAuUNjnAxlfHo1mdrIEueNreXGF5s%3D&reserved=0:
using ResourceManagementClient = Microsoft.Azure.Management.ResourceManager.ResourceManagementClient;
using Subscription = CogsMinimizer.Shared.Subscription;
namespace CogsMinimizer.Shared
{
public static class AzureResourceManagerUtil
{
Please add comments about the purpose of this dictionary and what are the key and values
In Shared/AzureResourceManagerUtil.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489486782&sdata=Cr3780Gt40RD4VTAuUNjnAxlfHo1mdrIEueNreXGF5s%3D&reserved=0:
- if (resourceTypes.Count() > 0)
{
return resourceTypes.First();
}
else
{
return null;
}
}
catch (Exception)
{
return null;
}
}
public static IEnumerable
Please add documentation to public methods.
In Shared/AzureResourceManagerUtil.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489486782&sdata=Cr3780Gt40RD4VTAuUNjnAxlfHo1mdrIEueNreXGF5s%3D&reserved=0:
@@ -353,6 +401,51 @@ public static void RevokeAllRolesFromServicePrincipalOnSubscription(string objec
}
}
Please document the method
@@ -353,6 +401,51 @@ public static void RevokeAllRolesFromServicePrincipalOnSubscription(string objec
}
}
Why do you need the resourceManagementClient? You need one method for extracting the resource type. Another method should look up the API version for that resource type. That's what should be cached.
@@ -353,6 +401,51 @@ public static void RevokeAllRolesFromServicePrincipalOnSubscription(string objec
}
}
public static ProviderResourceType GetResourceType(ResourceManagementClient resourceManagementClient, string azureresourceId)
{
Diagnostics.EnsureArgumentNotNull(() => resourceManagementClient);
Diagnostics.EnsureStringNotNullOrWhiteSpace(() => azureresourceId);
string[] resourceIdProps = azureresourceId.Split(new char[] { '/' });
We already have code in the service that extracts the resource type per resource. No need to duplicate it. You should refactor the code in a way that would use that instead of trying to parse it out again from the azure resource id.
@@ -496,39 +589,104 @@ public static ResourceManagementClient GetAppResourceManagementClient(string sub
return authorizationManagementClient;
}
public static GenericResource GetAzureResource(ResourceManagementClient resourceClient, string azureresourceid, string apiVersion, ITracer tracer)
Please document. Why do you need to get the resource? It takes time. Instead, we can simply try to delete the resource with the various API versions until we find the right one.
}
else
{
CloudException exception = new CloudException("No API to operate discovered");
Rephrase - "Couldn't find proper API version to delete resource"
+
///
/// An enum that indicates the status of a resource in the cleanup lifecycle
///
public enum ResourceOperationStatus
{
Succeeded,
Failed
}
public enum FailureReason
{
NoError,
WrongApiVersion,
ResourceInUse,
ResourceNotFound,
What about "resource is held by another resource"
@@ -152,20 +152,20 @@ private void DeleteMarkedResources()
//Try to delete the resources that are marked for delete
foreach (var resource in resourcesMarkedForDeletion)
{
try
{
_tracer.TraceVerbose($"Trying to delete the resource {resource.Name} of Type {resource.Type}");
AzureResourceManagerUtil.DeleteAzureResource(m_resourceManagementClient, resource.AzureResourceIdentifier, _tracer);
_tracer.TraceVerbose($"Trying to delete the resource {resource.Name} of Type {resource.Type}");
// We don't catch exceptions since method doesn't throw but catches all possible exceptions, trace them and
// return succeeded when succeeded deleting resource or resource doesn't exist (we suppose it is deleted manually by somebody
// failed if deleting failed
ResourceOperationResult result = AzureResourceManagerUtil.DeleteAzureResource(m_resourceManagementClient, resource.AzureResourceIdentifier, _tracer);
At this point we have the resource type in our hand. You can pass it down to the delete method in order to spare all the parsing and look up per subscription. There should be a single mapping used by all subscriptions where the key is resource type, and the value is an API version (or a list of them if there are several options).
{
_tracer.TraceError($"Failed to delete the resource {resource.Name} of Type {resource.Type}. Exception details: {e.Message}");
_tracer.TraceError($"Failed to delete the resource {resource.Name} of Type {resource.Type}");
You should also trace the failure type and message.
In SubMinimizerTests/SubMinimizerTests.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489496796&sdata=egi9zpR2Wc8ZS9s%2FEmi1Zo9%2B5HL9FS%2BlNQZcA2aRsEU%3D&reserved=0:
+using System.Data.Entity.Migrations;
+using System.IdentityModel.Claims;
+using System.Linq;
+using System.Reflection;
+using CogsMinimizer.Shared;
+using Microsoft.Azure.Management.Authorization;
+using Microsoft.Azure.Management.Authorization.Models;
+using Microsoft.Azure.Management.ResourceManager;
+using Microsoft.Azure.Management.ResourceManager.Models;
+using ResourceManagementClient = Microsoft.Azure.Management.ResourceManager.ResourceManagementClient;
+using Subscription = CogsMinimizer.Shared.Subscription;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+namespace SubMinimizerTests
+{
These are not unit tests, but rather E2E tests that only you can run given your authorisation to access that specific subscription. Instead you should refactor the code to allow unit testing by using interfaces and mocks. However, not necessarily as part of this change.
@@ -0,0 +1,64 @@
+namespace CogsMinimizer.Shared
+{
using System;
using System.Collections.Generic;
///
/// Implementation of the
///
public class TestTracer : ITracer
Why do you need it?
- string resourceTypeName = resourceIdProps[resourceIdProps.Count() - 2];
+
try
{
IEnumerable
IEnumerable
if (resourceTypes.Count() > 0)
{
return resourceTypes.First();
}
else
{
return null;
}
}
catch (Exception)
It is not a good idea to catch a general exception. It could hide real issues which we will never find.
- else
{
return null;
}
}
catch (Exception)
{
return null;
}
}
public static IEnumerable
{
Diagnostics.EnsureArgumentNotNull(() => resourceManagementClient);
if (!providerList.ContainsKey(resourceManagementClient.SubscriptionId))
I don't understand why you need to keep a mapping of the subscription to the providers list.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489506798&sdata=4wS%2B80WKtqowV2q2tKR6XrLy7Gvt%2FENi9Nv6TmDNHeo%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FARCVLADSQkcyLCVY6vVKBOPau48zu8kXks5rXcX6gaJpZM4LvC_F&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489506798&sdata=jTU3cXGWPHYs8o89OIHIoo2vpl3TJgtY8DIabuifleU%3D&reserved=0.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23issuecomment-276465804&data=02%7C01%7Cmaximsh%40microsoft.com%7Cf28612049466448dba8108d44a0f6dbb%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636214877510044483&sdata=PQv8Jk6u4pJsVxSOatwuBljiIg7fCWJGHiC6IrUZC1s%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAPVAdosUDuQBepwqOBBAVv_7X14AhaOyks5rX4uCgaJpZM4LvC_F&data=02%7C01%7Cmaximsh%40microsoft.com%7Cf28612049466448dba8108d44a0f6dbb%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636214877510054491&sdata=JNh%2BCN5FIlJx594S7627qkbDy59wO6whpBz%2BGTKm6YI%3D&reserved=0.
ok
From: Maxim Shklar [mailto:notifications@github.com] Sent: Tuesday, January 31, 2017 1:07 PM To: shklar/SubMinimizer SubMinimizer@noreply.github.com Cc: Evgeny Vitenberg eviten@microsoft.com; Author author@noreply.github.com Subject: Re: [shklar/SubMinimizer] delete is performed with intelligent api retrieval (#65)
No need to comment the controller methods
Sent from my Windows Phone
From: bergano65mailto:notifications@github.com Sent: 31/01/2017 21:29 To: shklar/SubMinimizermailto:SubMinimizer@noreply.github.com Cc: Maxim Shklarmailto:maximsh@microsoft.com; Mentionmailto:mention@noreply.github.com Subject: Re: [shklar/SubMinimizer] delete is performed with intelligent api retrieval (#65)
Comment parameters of all public methods? ( controller aren’t documented also)
From: Maxim Shklar [mailto:notifications@github.com] Sent: Monday, January 30, 2017 3:14 AM To: shklar/SubMinimizer SubMinimizer@noreply.github.com<mailto:SubMinimizer@noreply.github.com> Cc: Evgeny Vitenberg eviten@microsoft.com<mailto:eviten@microsoft.com>; Author author@noreply.github.com<mailto:author@noreply.github.com> Subject: Re: [shklar/SubMinimizer] delete is performed with intelligent api retrieval (#65)
@shklar requested changes on this pull request.
In Shared/AzureResourceManagerUtil.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489486782&sdata=Cr3780Gt40RD4VTAuUNjnAxlfHo1mdrIEueNreXGF5s%3D&reserved=0:
using ResourceManagementClient = Microsoft.Azure.Management.ResourceManager.ResourceManagementClient;
using Subscription = CogsMinimizer.Shared.Subscription;
namespace CogsMinimizer.Shared
{
public static class AzureResourceManagerUtil
{
Please add comments about the purpose of this dictionary and what are the key and values
In Shared/AzureResourceManagerUtil.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489486782&sdata=Cr3780Gt40RD4VTAuUNjnAxlfHo1mdrIEueNreXGF5s%3D&reserved=0:
- if (resourceTypes.Count() > 0)
{
return resourceTypes.First();
}
else
{
return null;
}
}
catch (Exception)
{
return null;
}
}
public static IEnumerable
Please add documentation to public methods.
In Shared/AzureResourceManagerUtil.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489486782&sdata=Cr3780Gt40RD4VTAuUNjnAxlfHo1mdrIEueNreXGF5s%3D&reserved=0:
@@ -353,6 +401,51 @@ public static void RevokeAllRolesFromServicePrincipalOnSubscription(string objec
}
}
Please document the method
@@ -353,6 +401,51 @@ public static void RevokeAllRolesFromServicePrincipalOnSubscription(string objec
}
}
Why do you need the resourceManagementClient? You need one method for extracting the resource type. Another method should look up the API version for that resource type. That's what should be cached.
@@ -353,6 +401,51 @@ public static void RevokeAllRolesFromServicePrincipalOnSubscription(string objec
}
}
public static ProviderResourceType GetResourceType(ResourceManagementClient resourceManagementClient, string azureresourceId)
{
Diagnostics.EnsureArgumentNotNull(() => resourceManagementClient);
Diagnostics.EnsureStringNotNullOrWhiteSpace(() => azureresourceId);
string[] resourceIdProps = azureresourceId.Split(new char[] { '/' });
We already have code in the service that extracts the resource type per resource. No need to duplicate it. You should refactor the code in a way that would use that instead of trying to parse it out again from the azure resource id.
@@ -496,39 +589,104 @@ public static ResourceManagementClient GetAppResourceManagementClient(string sub
return authorizationManagementClient;
}
public static GenericResource GetAzureResource(ResourceManagementClient resourceClient, string azureresourceid, string apiVersion, ITracer tracer)
Please document. Why do you need to get the resource? It takes time. Instead, we can simply try to delete the resource with the various API versions until we find the right one.
}
else
{
CloudException exception = new CloudException("No API to operate discovered");
Rephrase - "Couldn't find proper API version to delete resource"
+
///
/// An enum that indicates the status of a resource in the cleanup lifecycle
///
public enum ResourceOperationStatus
{
Succeeded,
Failed
}
public enum FailureReason
{
NoError,
WrongApiVersion,
ResourceInUse,
ResourceNotFound,
What about "resource is held by another resource"
@@ -152,20 +152,20 @@ private void DeleteMarkedResources()
//Try to delete the resources that are marked for delete
foreach (var resource in resourcesMarkedForDeletion)
{
try
{
_tracer.TraceVerbose($"Trying to delete the resource {resource.Name} of Type {resource.Type}");
AzureResourceManagerUtil.DeleteAzureResource(m_resourceManagementClient, resource.AzureResourceIdentifier, _tracer);
_tracer.TraceVerbose($"Trying to delete the resource {resource.Name} of Type {resource.Type}");
// We don't catch exceptions since method doesn't throw but catches all possible exceptions, trace them and
// return succeeded when succeeded deleting resource or resource doesn't exist (we suppose it is deleted manually by somebody
// failed if deleting failed
ResourceOperationResult result = AzureResourceManagerUtil.DeleteAzureResource(m_resourceManagementClient, resource.AzureResourceIdentifier, _tracer);
At this point we have the resource type in our hand. You can pass it down to the delete method in order to spare all the parsing and look up per subscription. There should be a single mapping used by all subscriptions where the key is resource type, and the value is an API version (or a list of them if there are several options).
{
_tracer.TraceError($"Failed to delete the resource {resource.Name} of Type {resource.Type}. Exception details: {e.Message}");
_tracer.TraceError($"Failed to delete the resource {resource.Name} of Type {resource.Type}");
You should also trace the failure type and message.
In SubMinimizerTests/SubMinimizerTests.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489496796&sdata=egi9zpR2Wc8ZS9s%2FEmi1Zo9%2B5HL9FS%2BlNQZcA2aRsEU%3D&reserved=0:
+using System.Data.Entity.Migrations;
+using System.IdentityModel.Claims;
+using System.Linq;
+using System.Reflection;
+using CogsMinimizer.Shared;
+using Microsoft.Azure.Management.Authorization;
+using Microsoft.Azure.Management.Authorization.Models;
+using Microsoft.Azure.Management.ResourceManager;
+using Microsoft.Azure.Management.ResourceManager.Models;
+using ResourceManagementClient = Microsoft.Azure.Management.ResourceManager.ResourceManagementClient;
+using Subscription = CogsMinimizer.Shared.Subscription;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+namespace SubMinimizerTests
+{
These are not unit tests, but rather E2E tests that only you can run given your authorisation to access that specific subscription. Instead you should refactor the code to allow unit testing by using interfaces and mocks. However, not necessarily as part of this change.
@@ -0,0 +1,64 @@
+namespace CogsMinimizer.Shared
+{
using System;
using System.Collections.Generic;
///
/// Implementation of the
///
public class TestTracer : ITracer
Why do you need it?
- string resourceTypeName = resourceIdProps[resourceIdProps.Count() - 2];
+
try
{
IEnumerable
IEnumerable
if (resourceTypes.Count() > 0)
{
return resourceTypes.First();
}
else
{
return null;
}
}
catch (Exception)
It is not a good idea to catch a general exception. It could hide real issues which we will never find.
- else
{
return null;
}
}
catch (Exception)
{
return null;
}
}
public static IEnumerable
{
Diagnostics.EnsureArgumentNotNull(() => resourceManagementClient);
if (!providerList.ContainsKey(resourceManagementClient.SubscriptionId))
I don't understand why you need to keep a mapping of the subscription to the providers list.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489506798&sdata=4wS%2B80WKtqowV2q2tKR6XrLy7Gvt%2FENi9Nv6TmDNHeo%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FARCVLADSQkcyLCVY6vVKBOPau48zu8kXks5rXcX6gaJpZM4LvC_F&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489506798&sdata=jTU3cXGWPHYs8o89OIHIoo2vpl3TJgtY8DIabuifleU%3D&reserved=0.
— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23issuecomment-276465804&data=02%7C01%7Cmaximsh%40microsoft.com%7Cf28612049466448dba8108d44a0f6dbb%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636214877510044483&sdata=PQv8Jk6u4pJsVxSOatwuBljiIg7fCWJGHiC6IrUZC1s%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FAPVAdosUDuQBepwqOBBAVv_7X14AhaOyks5rX4uCgaJpZM4LvC_F&data=02%7C01%7Cmaximsh%40microsoft.com%7Cf28612049466448dba8108d44a0f6dbb%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636214877510054491&sdata=JNh%2BCN5FIlJx594S7627qkbDy59wO6whpBz%2BGTKm6YI%3D&reserved=0.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23issuecomment-276492095&data=02%7C01%7Ceviten%40microsoft.com%7Ce3888ead23574088ce0608d44a1d0a55%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636214935966429303&sdata=u3H32amk%2Btuqlqpg2uq%2FzL%2BU7YKgsvGd%2FHokVGCjQmc%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FARCVLLbApVJmUgLen_8iTFeM3GCEtYh3ks5rX6JYgaJpZM4LvC_F&data=02%7C01%7Ceviten%40microsoft.com%7Ce3888ead23574088ce0608d44a1d0a55%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636214935966429303&sdata=g%2F6KnB8hmQ%2BR6its7%2FPQV7fKORgku59PHe3GTrciaO8%3D&reserved=0.
From: Maxim Shklar [mailto:notifications@github.com] Sent: Monday, January 30, 2017 3:14 AM To: shklar/SubMinimizer SubMinimizer@noreply.github.com Cc: Evgeny Vitenberg eviten@microsoft.com; Author author@noreply.github.com Subject: Re: [shklar/SubMinimizer] delete is performed with intelligent api retrieval (#65)
@shklar requested changes on this pull request.
In Shared/AzureResourceManagerUtil.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489486782&sdata=Cr3780Gt40RD4VTAuUNjnAxlfHo1mdrIEueNreXGF5s%3D&reserved=0:
using ResourceManagementClient = Microsoft.Azure.Management.ResourceManager.ResourceManagementClient;
using Subscription = CogsMinimizer.Shared.Subscription;
namespace CogsMinimizer.Shared
{
public static class AzureResourceManagerUtil
{
Please add comments about the purpose of this dictionary and what are the key and values
In Shared/AzureResourceManagerUtil.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489486782&sdata=Cr3780Gt40RD4VTAuUNjnAxlfHo1mdrIEueNreXGF5s%3D&reserved=0:
- if (resourceTypes.Count() > 0)
{
return resourceTypes.First();
}
else
{
return null;
}
}
catch (Exception)
{
return null;
}
}
public static IEnumerable
Please add documentation to public methods.
done
In Shared/AzureResourceManagerUtil.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489486782&sdata=Cr3780Gt40RD4VTAuUNjnAxlfHo1mdrIEueNreXGF5s%3D&reserved=0:
@@ -353,6 +401,51 @@ public static void RevokeAllRolesFromServicePrincipalOnSubscription(string objec
}
}
Please document the method
done
@@ -353,6 +401,51 @@ public static void RevokeAllRolesFromServicePrincipalOnSubscription(string objec
}
}
Why do you need the resourceManagementClient? You need one method for extracting the resource type. Another method should look up the API version for that resource type. That's what should be cached.
@@ -353,6 +401,51 @@ public static void RevokeAllRolesFromServicePrincipalOnSubscription(string objec
}
}
public static ProviderResourceType GetResourceType(ResourceManagementClient resourceManagementClient, string azureresourceId)
{
Diagnostics.EnsureArgumentNotNull(() => resourceManagementClient);
Diagnostics.EnsureStringNotNullOrWhiteSpace(() => azureresourceId);
string[] resourceIdProps = azureresourceId.Split(new char[] { '/' });
We already have code in the service that extracts the resource type per resource. No need to duplicate it. You should refactor the code in a way that would use that instead of trying to parse it out again from the azure resource id.
Sorry, where is code extracting resource types that we already have?
@@ -496,39 +589,104 @@ public static ResourceManagementClient GetAppResourceManagementClient(string sub
return authorizationManagementClient;
}
public static GenericResource GetAzureResource(ResourceManagementClient resourceClient, string azureresourceid, string apiVersion, ITracer tracer)
Please document. Why do you need to get the resource? It takes time. Instead, we can simply try to delete the resource with the various API versions until we find the right one.
}
else
{
CloudException exception = new CloudException("No API to operate discovered");
Rephrase - "Couldn't find proper API version to delete resource"
+
///
/// An enum that indicates the status of a resource in the cleanup lifecycle
///
public enum ResourceOperationStatus
{
Succeeded,
Failed
}
public enum FailureReason
{
NoError,
WrongApiVersion,
ResourceInUse,
ResourceNotFound,
What about "resource is held by another resource"
@@ -152,20 +152,20 @@ private void DeleteMarkedResources()
//Try to delete the resources that are marked for delete
foreach (var resource in resourcesMarkedForDeletion)
{
try
{
_tracer.TraceVerbose($"Trying to delete the resource {resource.Name} of Type {resource.Type}");
AzureResourceManagerUtil.DeleteAzureResource(m_resourceManagementClient, resource.AzureResourceIdentifier, _tracer);
_tracer.TraceVerbose($"Trying to delete the resource {resource.Name} of Type {resource.Type}");
// We don't catch exceptions since method doesn't throw but catches all possible exceptions, trace them and
// return succeeded when succeeded deleting resource or resource doesn't exist (we suppose it is deleted manually by somebody
// failed if deleting failed
ResourceOperationResult result = AzureResourceManagerUtil.DeleteAzureResource(m_resourceManagementClient, resource.AzureResourceIdentifier, _tracer);
At this point we have the resource type in our hand. You can pass it down to the delete method in order to spare all the parsing and look up per subscription. There should be a single mapping used by all subscriptions where the key is resource type, and the value is an API version (or a list of them if there are several options).
{
_tracer.TraceError($"Failed to delete the resource {resource.Name} of Type {resource.Type}. Exception details: {e.Message}");
_tracer.TraceError($"Failed to delete the resource {resource.Name} of Type {resource.Type}");
You should also trace the failure type and message.
In SubMinimizerTests/SubMinimizerTests.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489496796&sdata=egi9zpR2Wc8ZS9s%2FEmi1Zo9%2B5HL9FS%2BlNQZcA2aRsEU%3D&reserved=0:
+using System.Data.Entity.Migrations;
+using System.IdentityModel.Claims;
+using System.Linq;
+using System.Reflection;
+using CogsMinimizer.Shared;
+using Microsoft.Azure.Management.Authorization;
+using Microsoft.Azure.Management.Authorization.Models;
+using Microsoft.Azure.Management.ResourceManager;
+using Microsoft.Azure.Management.ResourceManager.Models;
+using ResourceManagementClient = Microsoft.Azure.Management.ResourceManager.ResourceManagementClient;
+using Subscription = CogsMinimizer.Shared.Subscription;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+namespace SubMinimizerTests
+{
These are not unit tests, but rather E2E tests that only you can run given your authorisation to access that specific subscription. Instead you should refactor the code to allow unit testing by using interfaces and mocks. However, not necessarily as part of this change.
@@ -0,0 +1,64 @@
+namespace CogsMinimizer.Shared
+{
using System;
using System.Collections.Generic;
///
/// Implementation of the
///
public class TestTracer : ITracer
Why do you need it?
- string resourceTypeName = resourceIdProps[resourceIdProps.Count() - 2];
+
try
{
IEnumerable
IEnumerable
if (resourceTypes.Count() > 0)
{
return resourceTypes.First();
}
else
{5
return null;
}
}
catch (Exception)
It is not a good idea to catch a general exception. It could hide real issues which we will never find.
- else
{
return null;
}
}
catch (Exception)
{
return null;
}
}
public static IEnumerable
{
Diagnostics.EnsureArgumentNotNull(() => resourceManagementClient);
if (!providerList.ContainsKey(resourceManagementClient.SubscriptionId))
I don't understand why you need to keep a mapping of the subscription to the providers list.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489506798&sdata=4wS%2B80WKtqowV2q2tKR6XrLy7Gvt%2FENi9Nv6TmDNHeo%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FARCVLADSQkcyLCVY6vVKBOPau48zu8kXks5rXcX6gaJpZM4LvC_F&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489506798&sdata=jTU3cXGWPHYs8o89OIHIoo2vpl3TJgtY8DIabuifleU%3D&reserved=0.
Comment parameters of all public methods? ( controller aren’t documented also)
From: Maxim Shklar [mailto:notifications@github.com] Sent: Monday, January 30, 2017 3:14 AM To: shklar/SubMinimizer SubMinimizer@noreply.github.com Cc: Evgeny Vitenberg eviten@microsoft.com; Author author@noreply.github.com Subject: Re: [shklar/SubMinimizer] delete is performed with intelligent api retrieval (#65)
@shklar requested changes on this pull request.
In Shared/AzureResourceManagerUtil.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489486782&sdata=Cr3780Gt40RD4VTAuUNjnAxlfHo1mdrIEueNreXGF5s%3D&reserved=0:
using Subscription = CogsMinimizer.Shared.Subscription;
namespace CogsMinimizer.Shared
{
Please add comments about the purpose of this dictionary and what are the key and values
In Shared/AzureResourceManagerUtil.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489486782&sdata=Cr3780Gt40RD4VTAuUNjnAxlfHo1mdrIEueNreXGF5s%3D&reserved=0:
{
return resourceTypes.First();
}
else
{
return null;
}
}
catch (Exception)
{
return null;
}
}
public static IEnumerable GetResourceProviderList(ResourceManagementClient resourceManagementClient)
Please add documentation to public methods.
In Shared/AzureResourceManagerUtil.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489486782&sdata=Cr3780Gt40RD4VTAuUNjnAxlfHo1mdrIEueNreXGF5s%3D&reserved=0:
Please document the method
In Shared/AzureResourceManagerUtil.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489496796&sdata=egi9zpR2Wc8ZS9s%2FEmi1Zo9%2B5HL9FS%2BlNQZcA2aRsEU%3D&reserved=0:
Why do you need the resourceManagementClient? You need one method for extracting the resource type. Another method should look up the API version for that resource type. That's what should be cached.
In Shared/AzureResourceManagerUtil.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489496796&sdata=egi9zpR2Wc8ZS9s%2FEmi1Zo9%2B5HL9FS%2BlNQZcA2aRsEU%3D&reserved=0:
public static ProviderResourceType GetResourceType(ResourceManagementClient resourceManagementClient, string azureresourceId)
{
Diagnostics.EnsureArgumentNotNull(() => resourceManagementClient);
Diagnostics.EnsureStringNotNullOrWhiteSpace(() => azureresourceId);
string[] resourceIdProps = azureresourceId.Split(new char[] { '/' });
We already have code in the service that extracts the resource type per resource. No need to duplicate it. You should refactor the code in a way that would use that instead of trying to parse it out again from the azure resource id.
In Shared/AzureResourceManagerUtil.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489496796&sdata=egi9zpR2Wc8ZS9s%2FEmi1Zo9%2B5HL9FS%2BlNQZcA2aRsEU%3D&reserved=0:
endregion
public static GenericResource GetAzureResource(ResourceManagementClient resourceClient, string azureresourceid, string apiVersion, ITracer tracer)
Please document. Why do you need to get the resource? It takes time. Instead, we can simply try to delete the resource with the various API versions until we find the right one.
In Shared/AzureResourceManagerUtil.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489496796&sdata=egi9zpR2Wc8ZS9s%2FEmi1Zo9%2B5HL9FS%2BlNQZcA2aRsEU%3D&reserved=0:
else
{
CloudException exception = new CloudException("No API to operate discovered");
Rephrase - "Couldn't find proper API version to delete resource"
In Shared/ResourceOperationStatus.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489496796&sdata=egi9zpR2Wc8ZS9s%2FEmi1Zo9%2B5HL9FS%2BlNQZcA2aRsEU%3D&reserved=0:
///
/// An enum that indicates the status of a resource in the cleanup lifecycle
///
public enum ResourceOperationStatus
{
Succeeded,
Failed
}
public enum FailureReason
{
NoError,
WrongApiVersion,
ResourceInUse,
ResourceNotFound,
What about "resource is held by another resource"
In Shared/SubscriptionAnalyzer.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489496796&sdata=egi9zpR2Wc8ZS9s%2FEmi1Zo9%2B5HL9FS%2BlNQZcA2aRsEU%3D&reserved=0:
try
{
_tracer.TraceVerbose($"Trying to delete the resource {resource.Name} of Type {resource.Type}");
AzureResourceManagerUtil.DeleteAzureResource(m_resourceManagementClient, resource.AzureResourceIdentifier, _tracer);
_tracer.TraceVerbose($"Trying to delete the resource {resource.Name} of Type {resource.Type}");
// We don't catch exceptions since method doesn't throw but catches all possible exceptions, trace them and
// return succeeded when succeeded deleting resource or resource doesn't exist (we suppose it is deleted manually by somebody
// failed if deleting failed
ResourceOperationResult result = AzureResourceManagerUtil.DeleteAzureResource(m_resourceManagementClient, resource.AzureResourceIdentifier, _tracer);
At this point we have the resource type in our hand. You can pass it down to the delete method in order to spare all the parsing and look up per subscription. There should be a single mapping used by all subscriptions where the key is resource type, and the value is an API version (or a list of them if there are several options).
In Shared/SubscriptionAnalyzer.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489496796&sdata=egi9zpR2Wc8ZS9s%2FEmi1Zo9%2B5HL9FS%2BlNQZcA2aRsEU%3D&reserved=0:
_tracer.TraceError($"Failed to delete the resource {resource.Name} of Type {resource.Type}. Exception details: {e.Message}");
_tracer.TraceError($"Failed to delete the resource {resource.Name} of Type {resource.Type}");
You should also trace the failure type and message.
In SubMinimizerTests/SubMinimizerTests.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489496796&sdata=egi9zpR2Wc8ZS9s%2FEmi1Zo9%2B5HL9FS%2BlNQZcA2aRsEU%3D&reserved=0:
+using System.IdentityModel.Claims;
+using System.Linq;
+using System.Reflection;
+using CogsMinimizer.Shared;
+using Microsoft.Azure.Management.Authorization;
+using Microsoft.Azure.Management.Authorization.Models;
+using Microsoft.Azure.Management.ResourceManager;
+using Microsoft.Azure.Management.ResourceManager.Models;
+using ResourceManagementClient = Microsoft.Azure.Management.ResourceManager.ResourceManagementClient;
+using Subscription = CogsMinimizer.Shared.Subscription;
+using Microsoft.VisualStudio.TestTools.UnitTesting;
+
+namespace SubMinimizerTests
+{
These are not unit tests, but rather E2E tests that only you can run given your authorisation to access that specific subscription. Instead you should refactor the code to allow unit testing by using interfaces and mocks. However, not necessarily as part of this change.
In SubMinimizerTests/TestTracer.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489496796&sdata=egi9zpR2Wc8ZS9s%2FEmi1Zo9%2B5HL9FS%2BlNQZcA2aRsEU%3D&reserved=0:
+namespace CogsMinimizer.Shared
+{
using System;
using System.Collections.Generic;
///
/// Implementation of the interface that traces to nowhere.
///
public class TestTracer : ITracer
Why do you need it?
In Shared/AzureResourceManagerUtil.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489506798&sdata=4wS%2B80WKtqowV2q2tKR6XrLy7Gvt%2FENi9Nv6TmDNHeo%3D&reserved=0:
+
try
{
IEnumerable providerList = GetResourceProviderList(resourceManagementClient);
IEnumerable resourceTypes = from p in providerList where p.NamespaceProperty == providerName from t in p.ResourceTypes where t.ResourceType == resourceTypeName select t;
if (resourceTypes.Count() > 0)
{
return resourceTypes.First();
}
else
{
return null;
}
}
catch (Exception)
It is not a good idea to catch a general exception. It could hide real issues which we will never find.
In Shared/AzureResourceManagerUtil.cshttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489506798&sdata=4wS%2B80WKtqowV2q2tKR6XrLy7Gvt%2FENi9Nv6TmDNHeo%3D&reserved=0:
{
return null;
}
}
catch (Exception)
{
return null;
}
}
public static IEnumerable GetResourceProviderList(ResourceManagementClient resourceManagementClient)
{
Diagnostics.EnsureArgumentNotNull(() => resourceManagementClient);
if (!providerList.ContainsKey(resourceManagementClient.SubscriptionId))
I don't understand why you need to keep a mapping of the subscription to the providers list.
— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fshklar%2FSubMinimizer%2Fpull%2F65%23pullrequestreview-18991268&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489506798&sdata=4wS%2B80WKtqowV2q2tKR6XrLy7Gvt%2FENi9Nv6TmDNHeo%3D&reserved=0, or mute the threadhttps://na01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fnotifications%2Funsubscribe-auth%2FARCVLADSQkcyLCVY6vVKBOPau48zu8kXks5rXcX6gaJpZM4LvC_F&data=02%7C01%7Ceviten%40microsoft.com%7C91efb49f00344213674308d449011a7e%7C72f988bf86f141af91ab2d7cd011db47%7C1%7C0%7C636213716489506798&sdata=jTU3cXGWPHYs8o89OIHIoo2vpl3TJgtY8DIabuifleU%3D&reserved=0.