Skip to content

Commit c2c4c86

Browse files
yantang-msftyantang-msft
committed
Resolve some minor code review comments
1 parent 774faf0 commit c2c4c86

File tree

3 files changed

+91
-105
lines changed

3 files changed

+91
-105
lines changed

‎src/ResourceManager/Compute/Commands.Compute/Common/DiagnosticsHelper.cs

Lines changed: 45 additions & 53 deletions
Original file line numberDiff line numberDiff line change
@@ -90,15 +90,15 @@ public static Hashtable GetPublicDiagnosticsConfigurationFromFile(string configu
9090
switch (GetConfigFileType(configurationPath))
9191
{
9292
case ConfigFileType.Xml:
93-
return GetPublicConfigFromXml(configurationPath, storageAccountName);
93+
return GetPublicConfigFromXmlFile(configurationPath, storageAccountName);
9494
case ConfigFileType.Json:
95-
return GetPublicConfigFromJson(configurationPath, storageAccountName);
95+
return GetPublicConfigFromJsonFile(configurationPath, storageAccountName);
9696
default:
9797
throw new ArgumentException(Properties.Resources.DiagnosticsExtensionInvalidConfigFileFormat);
9898
}
9999
}
100100

101-
private static Hashtable GetPublicConfigFromXml(string configurationPath, string storageAccountName)
101+
private static Hashtable GetPublicConfigFromXmlFile(string configurationPath, string storageAccountName)
102102
{
103103
var config = File.ReadAllText(configurationPath);
104104

@@ -158,9 +158,9 @@ private static Hashtable GetPublicConfigFromXml(string configurationPath, string
158158
return hashTable;
159159
}
160160

161-
private static Hashtable GetPublicConfigFromJson(string configurationPath, string storageAccountName)
161+
private static Hashtable GetPublicConfigFromJsonFile(string configurationPath, string storageAccountName)
162162
{
163-
var publicConfig = GetPublicConfigFromJson(configurationPath);
163+
var publicConfig = GetPublicConfigJObjectFromJsonFile(configurationPath);
164164
var properties = publicConfig.Properties().Select(p => p.Name);
165165
var wadCfgProperty = properties.FirstOrDefault(p => p.Equals(WadCfg, StringComparison.OrdinalIgnoreCase));
166166
var xmlCfgProperty = properties.FirstOrDefault(p => p.Equals(EncodedXmlCfg, StringComparison.OrdinalIgnoreCase));
@@ -195,7 +195,7 @@ public static Hashtable GetPrivateDiagnosticsConfiguration(string storageAccount
195195
return privateConfig;
196196
}
197197

198-
public static XElement GetPublicConfigFromXml(string configurationPath)
198+
public static XElement GetPublicConfigXElementFromXmlFile(string configurationPath)
199199
{
200200
XElement publicConfig = null;
201201

@@ -218,7 +218,7 @@ public static XElement GetPublicConfigFromXml(string configurationPath)
218218
return publicConfig;
219219
}
220220

221-
public static JObject GetPublicConfigFromJson(string configurationPath)
221+
public static JObject GetPublicConfigJObjectFromJsonFile(string configurationPath)
222222
{
223223
var config = JsonConvert.DeserializeObject<JObject>(File.ReadAllText(configurationPath));
224224
var properties = config.Properties().Select(p => p.Name);
@@ -283,13 +283,13 @@ public static string InitializeStorageAccountName(AzureStorageContext storageCon
283283
}
284284
else if (configFileType == ConfigFileType.Xml)
285285
{
286-
var publicConfig = GetPublicConfigFromXml(configurationPath);
286+
var publicConfig = GetPublicConfigXElementFromXmlFile(configurationPath);
287287
var storageNode = publicConfig == null ? null : publicConfig.Elements().FirstOrDefault(ele => ele.Name.LocalName == StorageAccountElemStr);
288288
storageAccountName = storageNode == null ? null : storageNode.Value;
289289
}
290290
else if (configFileType == ConfigFileType.Json)
291291
{
292-
var publicConfig = GetPublicConfigFromJson(configurationPath);
292+
var publicConfig = GetPublicConfigJObjectFromJsonFile(configurationPath);
293293
var properties = publicConfig.Properties().Select(p => p.Name);
294294
var storageAccountProperty = properties.FirstOrDefault(p => p.Equals(StorageAccount, StringComparison.OrdinalIgnoreCase));
295295
storageAccountName = storageAccountProperty == null ? null : publicConfig[storageAccountProperty].Value<string>();
@@ -309,16 +309,7 @@ public static string InitializeStorageAccountKey(IStorageManagementClient storag
309309
string storageAccountKey = null;
310310
StorageAccount storageAccount = null;
311311

312-
try
313-
{
314-
var storageAccounts = storageClient.StorageAccounts.List().StorageAccounts;
315-
storageAccount = storageAccounts == null ? null : storageAccounts.FirstOrDefault(account => account.Name.Equals(storageAccountName));
316-
}
317-
catch
318-
{
319-
}
320-
321-
if (storageAccount != null)
312+
if (TryGetStorageAccount(storageClient, storageAccountName, out storageAccount))
322313
{
323314
// Help user retrieve the storage account key
324315
var psStorageAccount = new PSStorageAccount(storageAccount);
@@ -346,51 +337,52 @@ public static string InitializeStorageAccountEndpoint(string storageAccountName,
346337
AzureStorageContext storageContext = null, string configurationPath = null, AzureContext defaultContext = null)
347338
{
348339
string storageAccountEndpoint = null;
340+
StorageAccount storageAccount = null;
349341

350342
if (storageContext != null)
351343
{
352344
// Get value from StorageContext
353345
storageAccountEndpoint = GetEndpointFromStorageContext(storageContext);
354346
}
355-
else
347+
else if (TryGetStorageAccount(storageClient, storageAccountName, out storageAccount))
348+
{
349+
// Get value from StorageAccount
350+
var endpoints = storageAccount.PrimaryEndpoints;
351+
var context = CreateStorageContext(endpoints.Blob, endpoints.Queue, endpoints.Table, endpoints.File, storageAccountName, storageAccountKey);
352+
storageAccountEndpoint = GetEndpointFromStorageContext(context);
353+
}
354+
else if (!string.IsNullOrEmpty(
355+
storageAccountEndpoint = GetStorageAccountInfoFromPrivateConfig(configurationPath, PrivConfEndpointAttr)))
356+
{
357+
// We can get the value from PrivateConfig
358+
}
359+
else if (defaultContext != null && defaultContext.Environment != null)
356360
{
357-
// Try get the storage account from current subscription
358-
StorageAccount storageAccount = null;
361+
// Get value from default azure environment. Default to use https
362+
Uri blobEndpoint = defaultContext.Environment.GetStorageBlobEndpoint(storageAccountName);
363+
Uri queueEndpoint = defaultContext.Environment.GetStorageQueueEndpoint(storageAccountName);
364+
Uri tableEndpoint = defaultContext.Environment.GetStorageTableEndpoint(storageAccountName);
365+
Uri fileEndpoint = defaultContext.Environment.GetStorageFileEndpoint(storageAccountName);
366+
var context = CreateStorageContext(blobEndpoint, queueEndpoint, tableEndpoint, fileEndpoint, storageAccountName, storageAccountKey);
367+
storageAccountEndpoint = GetEndpointFromStorageContext(context);
368+
}
359369

360-
try
361-
{
362-
var storageAccounts = storageClient.StorageAccounts.List().StorageAccounts;
363-
storageAccount = storageAccounts == null ? null : storageAccounts.FirstOrDefault(account => account.Name.Equals(storageAccountName));
364-
}
365-
catch
366-
{
367-
}
370+
return storageAccountEndpoint;
371+
}
368372

369-
if (storageAccount != null)
370-
{
371-
// Get value from StorageAccount
372-
var endpoints = storageAccount.PrimaryEndpoints;
373-
var context = CreateStorageContext(endpoints.Blob, endpoints.Queue, endpoints.Table, endpoints.File, storageAccountName, storageAccountKey);
374-
storageAccountEndpoint = GetEndpointFromStorageContext(context);
375-
}
376-
else if (!string.IsNullOrEmpty(GetStorageAccountInfoFromPrivateConfig(configurationPath, PrivConfEndpointAttr)))
377-
{
378-
// Get value from PrivateConfig
379-
storageAccountEndpoint = GetStorageAccountInfoFromPrivateConfig(configurationPath, PrivConfEndpointAttr);
380-
}
381-
else if (defaultContext != null && defaultContext.Environment != null)
382-
{
383-
// Get value from default azure environment. Default to use https
384-
Uri blobEndpoint = defaultContext.Environment.GetStorageBlobEndpoint(storageAccountName);
385-
Uri queueEndpoint = defaultContext.Environment.GetStorageQueueEndpoint(storageAccountName);
386-
Uri tableEndpoint = defaultContext.Environment.GetStorageTableEndpoint(storageAccountName);
387-
Uri fileEndpoint = defaultContext.Environment.GetStorageFileEndpoint(storageAccountName);
388-
var context = CreateStorageContext(blobEndpoint, queueEndpoint, tableEndpoint, fileEndpoint, storageAccountName, storageAccountKey);
389-
storageAccountEndpoint = GetEndpointFromStorageContext(context);
390-
}
373+
private static bool TryGetStorageAccount(IStorageManagementClient storageClient, string storageAccountName, out StorageAccount storageAccount)
374+
{
375+
try
376+
{
377+
var storageAccounts = storageClient.StorageAccounts.List().StorageAccounts;
378+
storageAccount = storageAccounts == null ? null : storageAccounts.FirstOrDefault(account => account.Name.Equals(storageAccountName));
379+
}
380+
catch
381+
{
382+
storageAccount = null;
391383
}
392384

393-
return storageAccountEndpoint;
385+
return storageAccount != null;
394386
}
395387

396388
private static AzureStorageContext CreateStorageContext(Uri blobEndpoint, Uri queueEndpoint, Uri tableEndpoint, Uri fileEndpoint,

‎src/ServiceManagement/Compute/Commands.ServiceManagement/Common/DiagnosticsHelper.cs

Lines changed: 45 additions & 51 deletions
Original file line numberDiff line numberDiff line change
@@ -89,15 +89,15 @@ public static Hashtable GetPublicDiagnosticsConfigurationFromFile(string configu
8989
switch (GetConfigFileType(configurationPath))
9090
{
9191
case ConfigFileType.Xml:
92-
return GetPublicConfigFromXml(configurationPath, storageAccountName);
92+
return GetPublicConfigFromXmlFile(configurationPath, storageAccountName);
9393
case ConfigFileType.Json:
94-
return GetPublicConfigFromJson(configurationPath, storageAccountName);
94+
return GetPublicConfigFromJsonFile(configurationPath, storageAccountName);
9595
default:
9696
throw new ArgumentException(Properties.Resources.DiagnosticsExtensionInvalidConfigFileFormat);
9797
}
9898
}
9999

100-
private static Hashtable GetPublicConfigFromXml(string configurationPath, string storageAccountName)
100+
private static Hashtable GetPublicConfigFromXmlFile(string configurationPath, string storageAccountName)
101101
{
102102
var config = File.ReadAllText(configurationPath);
103103

@@ -157,9 +157,9 @@ private static Hashtable GetPublicConfigFromXml(string configurationPath, string
157157
return hashTable;
158158
}
159159

160-
private static Hashtable GetPublicConfigFromJson(string configurationPath, string storageAccountName)
160+
private static Hashtable GetPublicConfigFromJsonFile(string configurationPath, string storageAccountName)
161161
{
162-
var publicConfig = GetPublicConfigFromJson(configurationPath);
162+
var publicConfig = GetPublicConfigJObjectFromJsonFile(configurationPath);
163163
var properties = publicConfig.Properties().Select(p => p.Name);
164164
var wadCfgProperty = properties.FirstOrDefault(p => p.Equals(WadCfg, StringComparison.OrdinalIgnoreCase));
165165
var xmlCfgProperty = properties.FirstOrDefault(p => p.Equals(EncodedXmlCfg, StringComparison.OrdinalIgnoreCase));
@@ -194,7 +194,7 @@ public static Hashtable GetPrivateDiagnosticsConfiguration(string storageAccount
194194
return privateConfig;
195195
}
196196

197-
public static XElement GetPublicConfigFromXml(string configurationPath)
197+
public static XElement GetPublicConfigXElementFromXmlFile(string configurationPath)
198198
{
199199
XElement publicConfig = null;
200200

@@ -217,7 +217,7 @@ public static XElement GetPublicConfigFromXml(string configurationPath)
217217
return publicConfig;
218218
}
219219

220-
public static JObject GetPublicConfigFromJson(string configurationPath)
220+
public static JObject GetPublicConfigJObjectFromJsonFile(string configurationPath)
221221
{
222222
var config = JsonConvert.DeserializeObject<JObject>(File.ReadAllText(configurationPath));
223223
var properties = config.Properties().Select(p => p.Name);
@@ -282,13 +282,13 @@ public static string InitializeStorageAccountName(AzureStorageContext storageCon
282282
}
283283
else if (configFileType == ConfigFileType.Xml)
284284
{
285-
var publicConfig = GetPublicConfigFromXml(configurationPath);
285+
var publicConfig = GetPublicConfigXElementFromXmlFile(configurationPath);
286286
var storageNode = publicConfig == null ? null : publicConfig.Elements().FirstOrDefault(ele => ele.Name.LocalName == StorageAccountElemStr);
287287
storageAccountName = storageNode == null ? null : storageNode.Value;
288288
}
289289
else if (configFileType == ConfigFileType.Json)
290290
{
291-
var publicConfig = GetPublicConfigFromJson(configurationPath);
291+
var publicConfig = GetPublicConfigJObjectFromJsonFile(configurationPath);
292292
var properties = publicConfig.Properties().Select(p => p.Name);
293293
var storageAccountProperty = properties.FirstOrDefault(p => p.Equals(StorageAccount, StringComparison.OrdinalIgnoreCase));
294294
storageAccountName = storageAccountProperty == null ? null : publicConfig[storageAccountProperty].Value<string>();
@@ -308,15 +308,7 @@ public static string InitializeStorageAccountKey(IStorageManagementClient storag
308308
string storageAccountKey = null;
309309
StorageAccount storageAccount = null;
310310

311-
try
312-
{
313-
storageAccount = storageClient.StorageAccounts.Get(storageAccountName).StorageAccount;
314-
}
315-
catch
316-
{
317-
}
318-
319-
if (storageAccount != null)
311+
if (TryGetStorageAccount(storageClient, storageAccountName, out storageAccount))
320312
{
321313
// Help user retrieve the storage account key
322314
var keys = storageClient.StorageAccounts.GetKeys(storageAccount.Name);
@@ -346,50 +338,52 @@ public static string InitializeStorageAccountEndpoint(string storageAccountName,
346338
AzureStorageContext storageContext = null, string configurationPath = null, AzureContext defaultContext = null)
347339
{
348340
string storageAccountEndpoint = null;
341+
StorageAccount storageAccount = null;
349342

350343
if (storageContext != null)
351344
{
352345
// Get value from StorageContext
353346
storageAccountEndpoint = GetEndpointFromStorageContext(storageContext);
354347
}
355-
else
348+
else if (TryGetStorageAccount(storageClient, storageAccountName, out storageAccount)
349+
&& storageAccount.Properties.Endpoints.Count >= 4)
356350
{
357-
// Try get the storage account from current subscription
358-
StorageAccount storageAccount = null;
351+
// Get value from StorageAccount
352+
var endpoints = storageAccount.Properties.Endpoints;
353+
var context = CreateStorageContext(endpoints[0], endpoints[1], endpoints[2], endpoints[3], storageAccountName, storageAccountKey);
354+
storageAccountEndpoint = GetEndpointFromStorageContext(context);
355+
}
356+
else if (!string.IsNullOrEmpty(
357+
storageAccountEndpoint = GetStorageAccountInfoFromPrivateConfig(configurationPath, PrivConfEndpointAttr)))
358+
{
359+
// We can get value from PrivateConfig
360+
}
361+
else if (defaultContext != null && defaultContext.Environment != null)
362+
{
363+
// Get value from default azure environment. Default to use https
364+
Uri blobEndpoint = defaultContext.Environment.GetStorageBlobEndpoint(storageAccountName);
365+
Uri queueEndpoint = defaultContext.Environment.GetStorageQueueEndpoint(storageAccountName);
366+
Uri tableEndpoint = defaultContext.Environment.GetStorageTableEndpoint(storageAccountName);
367+
Uri fileEndpoint = defaultContext.Environment.GetStorageFileEndpoint(storageAccountName);
368+
var context = CreateStorageContext(blobEndpoint, queueEndpoint, tableEndpoint, fileEndpoint, storageAccountName, storageAccountKey);
369+
storageAccountEndpoint = GetEndpointFromStorageContext(context);
370+
}
359371

360-
try
361-
{
362-
storageAccount = storageClient.StorageAccounts.Get(storageAccountName).StorageAccount;
363-
}
364-
catch
365-
{
366-
}
372+
return storageAccountEndpoint;
373+
}
367374

368-
if (storageAccount != null && storageAccount.Properties.Endpoints.Count >= 4)
369-
{
370-
// Get value from StorageAccount
371-
var endpoints = storageAccount.Properties.Endpoints;
372-
var context = CreateStorageContext(endpoints[0], endpoints[1], endpoints[2], endpoints[3], storageAccountName, storageAccountKey);
373-
storageAccountEndpoint = GetEndpointFromStorageContext(context);
374-
}
375-
else if (!string.IsNullOrEmpty(GetStorageAccountInfoFromPrivateConfig(configurationPath, PrivConfEndpointAttr)))
376-
{
377-
// Get value from PrivateConfig
378-
storageAccountEndpoint = GetStorageAccountInfoFromPrivateConfig(configurationPath, PrivConfEndpointAttr);
379-
}
380-
else if (defaultContext != null && defaultContext.Environment != null)
381-
{
382-
// Get value from default azure environment. Default to use https
383-
Uri blobEndpoint = defaultContext.Environment.GetStorageBlobEndpoint(storageAccountName);
384-
Uri queueEndpoint = defaultContext.Environment.GetStorageQueueEndpoint(storageAccountName);
385-
Uri tableEndpoint = defaultContext.Environment.GetStorageTableEndpoint(storageAccountName);
386-
Uri fileEndpoint = defaultContext.Environment.GetStorageFileEndpoint(storageAccountName);
387-
var context = CreateStorageContext(blobEndpoint, queueEndpoint, tableEndpoint, fileEndpoint, storageAccountName, storageAccountKey);
388-
storageAccountEndpoint = GetEndpointFromStorageContext(context);
389-
}
375+
private static bool TryGetStorageAccount(IStorageManagementClient storageClient, string storageAccountName, out StorageAccount storageAccount)
376+
{
377+
try
378+
{
379+
storageAccount = storageClient.StorageAccounts.Get(storageAccountName).StorageAccount;
380+
}
381+
catch
382+
{
383+
storageAccount = null;
390384
}
391385

392-
return storageAccountEndpoint;
386+
return storageAccount != null;
393387
}
394388

395389
private static AzureStorageContext CreateStorageContext(Uri blobEndpoint, Uri queueEndpoint, Uri tableEndpoint, Uri fileEndpoint,

‎src/ServiceManagement/Compute/Commands.ServiceManagement/Extensions/Diagnostics/BaseAzureServiceDiagnosticsExtension.cs

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -115,7 +115,7 @@ protected override void ValidateConfiguration()
115115
}
116116
}
117117

118-
var publicConfigElem = DiagnosticsHelper.GetPublicConfigFromXml(this.DiagnosticsConfigurationPath);
118+
var publicConfigElem = DiagnosticsHelper.GetPublicConfigXElementFromXmlFile(this.DiagnosticsConfigurationPath);
119119
if (publicConfigElem == null)
120120
{
121121
throw new ArgumentException(Resources.DiagnosticsExtensionNullPublicConfig);

0 commit comments

Comments
 (0)