Skip to content

Commit 066954d

Browse files
benbpbenbp
committed
Use logger factory instead of console for codeowners library logging (Azure#12022)
1 parent cbbe9d6 commit 066954d

12 files changed

Lines changed: 136 additions & 44 deletions

File tree

‎tools/azsdk-cli/Azure.Sdk.Tools.Cli.Contract/MCPMultiCommandTool.cs‎

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,12 @@
1+
// Copyright (c) Microsoft Corporation.
2+
// Licensed under the MIT License.
3+
using System.CommandLine;
4+
5+
namespace Azure.Sdk.Tools.Cli.Contract;
6+
7+
public abstract class MCPMultiCommandTool : MCPToolBase
8+
{
9+
public abstract List<Command> GetCommands();
10+
11+
public override List<Command> GetCommandInstances() => GetCommands();
12+
}

‎tools/azsdk-cli/Azure.Sdk.Tools.Cli.Contract/MCPToolBase.cs‎

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
// Copyright (c) Microsoft Corporation.
2+
// Licensed under the MIT License.
3+
using System.CommandLine;
4+
using System.CommandLine.Invocation;
5+
6+
namespace Azure.Sdk.Tools.Cli.Contract;
7+
8+
/// <summary>
9+
/// This is the base class defining how an MCP enabled tool will interface with the server.
10+
///
11+
/// This covers:
12+
/// - route registration/disambiguation
13+
/// - compilation trim avoidance for reflection-included MCP tools
14+
/// </summary>
15+
public abstract class MCPToolBase
16+
{
17+
public MCPToolBase() { }
18+
19+
public Command? Command;
20+
21+
public int ExitCode { get; set; } = 0;
22+
23+
public void SetFailure(int exitCode = 1)
24+
{
25+
ExitCode = exitCode;
26+
}
27+
28+
public abstract List<Command> GetCommandInstances();
29+
30+
public virtual CommandGroup[] CommandHierarchy { get; set; } = [];
31+
32+
public abstract Task HandleCommand(InvocationContext ctx, CancellationToken ct);
33+
}

‎tools/azsdk-cli/Azure.Sdk.Tools.Cli.Tests/Tools/CodeownersToolsTests.cs‎

Lines changed: 7 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -31,11 +31,11 @@ public void Setup()
3131
_mockLogger = new Mock<ILogger<CodeownersTools>>();
3232
_mockCodeownersValidator = new Mock<ICodeownersValidatorHelper>();
3333

34-
3534
_tool = new CodeownersTools(
3635
_mockGithub,
3736
_mockOutput.Object,
3837
_mockLogger.Object,
38+
null,
3939
_mockCodeownersValidator.Object);
4040
}
4141

@@ -63,7 +63,7 @@ public async Task Update_LabelMissing_NotInReview_NoPath_Throws()
6363
gh.Setup(s => s.GetContentsSingleAsync(Constants.AZURE_OWNER_PATH, "repo", Constants.AZURE_CODEOWNERS_PATH, It.IsAny<string>()))
6464
.ReturnsAsync(new RepositoryContent("CODEOWNERS", ".github/CODEOWNERS", "shaCode", 0, ContentType.File, null, null, null, null, "utf-8", Convert.ToBase64String(System.Text.Encoding.UTF8.GetBytes("")), null, null));
6565

66-
var tool = new CodeownersTools(gh.Object, _mockOutput.Object, _mockLogger.Object, _mockCodeownersValidator.Object);
66+
var tool = new CodeownersTools(gh.Object, _mockOutput.Object, _mockLogger.Object, null, _mockCodeownersValidator.Object);
6767

6868
var result = await tool.UpdateCodeowners("repo", false, "", "NonExistService", new List<string>(), new List<string>(), true);
6969

@@ -92,7 +92,7 @@ public async Task Update_AddOwnersToExistingEntry_Success()
9292
var validator = new Mock<ICodeownersValidatorHelper>();
9393
validator.Setup(v => v.ValidateCodeOwnerAsync(It.IsAny<string>(), It.IsAny<bool>())).ReturnsAsync(new CodeownersValidationResult { Username = "user", IsValidCodeOwner = true });
9494

95-
var tool = new CodeownersTools(gh.Object, _mockOutput.Object, _mockLogger.Object, validator.Object);
95+
var tool = new CodeownersTools(gh.Object, _mockOutput.Object, _mockLogger.Object, null, validator.Object);
9696
var result = await tool.UpdateCodeowners("repoName", false, "/sdk/myservice/", "MyService", new List<string> { "@oldowner", "@newowner" }, new List<string>() { "@newowner", "@newowner2" }, true);
9797
Assert.IsNotNull(result);
9898
Assert.IsTrue(result.Contains("URL:") || result.Contains("Created"));
@@ -119,7 +119,7 @@ public async Task Update_RemoveOwnersFromExistingEntry_Success()
119119
var validator = new Mock<ICodeownersValidatorHelper>();
120120
validator.Setup(v => v.ValidateCodeOwnerAsync(It.IsAny<string>(), It.IsAny<bool>())).ReturnsAsync(new CodeownersValidationResult { Username = "user", IsValidCodeOwner = true });
121121

122-
var tool = new CodeownersTools(gh.Object, _mockOutput.Object, _mockLogger.Object, validator.Object);
122+
var tool = new CodeownersTools(gh.Object, _mockOutput.Object, _mockLogger.Object, null, validator.Object);
123123
// Remove @removeowner, keep @oldowner
124124
var result = await tool.UpdateCodeowners("repoName", false, "/sdk/myservice/", "MyService", new List<string> { "@oldowner" }, new List<string>(), false);
125125
Assert.IsNotNull(result);
@@ -160,7 +160,7 @@ public async Task Update_CreateNewEntry_PRSuccess()
160160
validator.Setup(v => v.ValidateCodeOwnerAsync(It.IsAny<string>(), It.IsAny<bool>()))
161161
.ReturnsAsync(new CodeownersValidationResult { Username = "user", IsValidCodeOwner = true });
162162

163-
var tool = new CodeownersTools(gh.Object, _mockOutput.Object, _mockLogger.Object, validator.Object);
163+
var tool = new CodeownersTools(gh.Object, _mockOutput.Object, _mockLogger.Object, null, validator.Object);
164164

165165
var result = await tool.UpdateCodeowners("repoName", false, "/sdk/newsvc/", "NewSvc", new List<string> { "@a", "@b" }, new List<string> { "@s1", "@s2" }, true);
166166
Assert.IsNotNull(result);
@@ -196,7 +196,7 @@ public async Task Update_CreateNewEntry_PRFailure_ReturnsPRFailureMessage()
196196
validator.Setup(v => v.ValidateCodeOwnerAsync(It.IsAny<string>(), It.IsAny<bool>()))
197197
.ReturnsAsync(new CodeownersValidationResult { Username = "user", IsValidCodeOwner = true });
198198

199-
var tool = new CodeownersTools(gh.Object, _mockOutput.Object, _mockLogger.Object, validator.Object);
199+
var tool = new CodeownersTools(gh.Object, _mockOutput.Object, _mockLogger.Object, null, validator.Object);
200200

201201
var result = await tool.UpdateCodeowners("repoName", false, "/sdk/newsvc/", "NewSvc", new List<string> { "@a", "@b" }, new List<string> { "@s1", "@s2" }, true);
202202
Assert.IsNotNull(result);
@@ -239,7 +239,7 @@ public async Task Validate_CODEOWNERS_Missing_ReturnsError()
239239
var output = new Mock<IOutputHelper>();
240240
var validator = new Mock<ICodeownersValidatorHelper>();
241241

242-
var tool = new CodeownersTools(gh.Object, output.Object, _mockLogger.Object, validator.Object);
242+
var tool = new CodeownersTools(gh.Object, output.Object, _mockLogger.Object, null, validator.Object);
243243

244244
var result = await tool.ValidateCodeownersEntryForService("test-repo", "Any", null);
245245
Assert.IsNotNull(result);

‎tools/azsdk-cli/Azure.Sdk.Tools.Cli/Tools/EngSys/CodeownersTools/CodeownersTools.cs‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@
99
using Azure.Sdk.Tools.Cli.Helpers;
1010
using Azure.Sdk.Tools.Cli.Services;
1111
using Azure.Sdk.Tools.Cli.Contract;
12-
using Azure.Sdk.Tools.CodeownersUtils.Parsing;
1312
using Azure.Sdk.Tools.CodeownersUtils.Editing;
13+
using Azure.Sdk.Tools.CodeownersUtils.Parsing;
1414
using Azure.Sdk.Tools.Cli.Configuration;
1515
using Azure.Sdk.Tools.Cli.Models.Responses;
1616
using Azure.Sdk.Tools.Cli.Commands;
@@ -47,13 +47,16 @@ public CodeownersTools(
4747
IGitHubService githubService,
4848
IOutputHelper output,
4949
ILogger<CodeownersTools> logger,
50+
ILoggerFactory? loggerFactory,
5051
ICodeownersValidatorHelper codeownersValidator) : base()
5152
{
5253
this.githubService = githubService;
5354
this.output = output;
5455
this.logger = logger;
5556
this.codeownersValidator = codeownersValidator;
5657

58+
CodeownersUtils.Utils.Log.Configure(loggerFactory);
59+
5760
CommandHierarchy =
5861
[
5962
SharedCommandGroups.EngSys

‎tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/TestHelpers.cs‎

Lines changed: 8 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -8,6 +8,7 @@
88
using Azure.Sdk.Tools.CodeownersUtils.Caches;
99
using Azure.Sdk.Tools.CodeownersUtils.Parsing;
1010
using Azure.Sdk.Tools.CodeownersUtils.Utils;
11+
using Microsoft.Extensions.Logging;
1112

1213
namespace Azure.Sdk.Tools.CodeownersUtils.Tests
1314
{
@@ -26,7 +27,7 @@ public static class TestHelpers
2627
/// The user/org visibility data will consist of 5 users, TestOwner0..TestOwner4, with only even users
2728
/// being visible.
2829
/// The team/user data will consist of 5 teams with the number of users in each team equal to the
29-
/// team number, TestTeam0...TestTeam4.The users in each team will consist of the same users in
30+
/// team number, TestTeam0...TestTeam4.The users in each team will consist of the same users in
3031
/// the user/org data.
3132
/// </summary>
3233
/// <returns>Populated OwnerDataUtils</returns>
@@ -38,7 +39,7 @@ public static OwnerDataUtils SetupOwnerData()
3839
Dictionary<string, List<string>> teamUserDict = new Dictionary<string, List<string>>(StringComparer.InvariantCultureIgnoreCase);
3940

4041
// Create 5 teams
41-
for (int i = 0;i < _maxItems; i++)
42+
for (int i = 0; i < _maxItems; i++)
4243
{
4344
string team = $"{TestTeamNamePartial}{i}";
4445
List<string> users = new List<string>();
@@ -58,7 +59,7 @@ public static OwnerDataUtils SetupOwnerData()
5859
UserOrgVisibilityCache userOrgVisibilityCache = new UserOrgVisibilityCache(null);
5960
Dictionary<string, bool> userOrgVisDict = new Dictionary<string, bool>(StringComparer.InvariantCultureIgnoreCase);
6061
// Make every even user visible
61-
for (int i = 0;i < 5;i++)
62+
for (int i = 0; i < 5; i++)
6263
{
6364
string user = $"{TestOwnerNamePartial}{i}";
6465
if (i % 2 == 0)
@@ -82,7 +83,7 @@ public static RepoLabelDataUtils SetupRepoLabelData()
8283
{
8384
Dictionary<string, HashSet<string>> repoLabelDict = new Dictionary<string, HashSet<string>>(StringComparer.InvariantCultureIgnoreCase);
8485
HashSet<string> repoLabels = new HashSet<string>(StringComparer.InvariantCultureIgnoreCase);
85-
for (int i = 0;i < 5;i++)
86+
for (int i = 0; i < 5; i++)
8687
{
8788
string label = $"{TestLabelNamePartial}{i}";
8889
repoLabels.Add(label);
@@ -112,7 +113,7 @@ public static bool StringListsAreEqual(List<string> actuaList, List<string> expe
112113
/// <param name="actualCodeownerEntries">The actual, or parsed, list of Codeowners entries.</param>
113114
/// <param name="expectedCodeownerEntries">The expected list of Codeowners entries</param>
114115
/// <returns>True if they're equal, false otherwise</returns>
115-
public static bool CodeownersEntryListsAreEqual(List<CodeownersEntry> actualCodeownerEntries,
116+
public static bool CodeownersEntryListsAreEqual(List<CodeownersEntry> actualCodeownerEntries,
116117
List<CodeownersEntry> expectedCodeownerEntries)
117118
{
118119
// SequenceEqual determines whether two sequences are equal by comparing the length and
@@ -190,8 +191,8 @@ public static void TempGetSerializedString(List<CodeownersEntry> codeownersEntri
190191
WriteIndented = true
191192
};
192193
string jsonString = JsonSerializer.Serialize(codeownersEntries, jsonSerializerOptions);
193-
Console.WriteLine(jsonString);
194-
Console.WriteLine("something to break on");
194+
Log.Logger.LogInformation("{Json}", jsonString);
195+
Log.Logger.LogInformation("something to break on");
195196
}
196197
}
197198
}

‎tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter.Tests/Utils/BaselineUtilsTests.cs‎

Lines changed: 5 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99
using Azure.Sdk.Tools.CodeownersUtils.Utils;
1010
using Azure.Sdk.Tools.CodeownersUtils.Verification;
1111
using NUnit.Framework;
12+
using Microsoft.Extensions.Logging;
1213

1314
namespace Azure.Sdk.Tools.CodeownersUtils.Tests.Utils
1415
{
@@ -43,7 +44,7 @@ public void CleanupTestData()
4344
// Cleanup any temporary files created by the tests
4445
if (_tempFiles.Count > 0)
4546
{
46-
foreach(string tempFile in _tempFiles)
47+
foreach (string tempFile in _tempFiles)
4748
{
4849
int tryNumber = 0;
4950
while (tryNumber < maxTries)
@@ -55,7 +56,7 @@ public void CleanupTestData()
5556
}
5657
catch (Exception ex)
5758
{
58-
Console.WriteLine($"Exception trying to delete {tempFile}. Ex={ex}");
59+
Log.Logger.LogError(ex, "Exception trying to delete {TempFile}.", tempFile);
5960
// sleep 100ms and try again
6061
System.Threading.Thread.Sleep(100);
6162
}
@@ -64,7 +65,7 @@ public void CleanupTestData()
6465
// Report that the test wasn't able to delete the file after successive tries
6566
if (tryNumber == maxTries)
6667
{
67-
Console.WriteLine($"Unable to delete {tempFile} after {maxTries}, see above for exception details.");
68+
Log.Logger.LogWarning("Unable to delete {TempFile} after {MaxTries}, see above for exception details.", tempFile, maxTries);
6869
}
6970
}
7071
}
@@ -89,7 +90,7 @@ public void TestBaselineGenerationAndFiltering(string testCodeownerFile,
8990
// Load the expected baseline file and, for sanity's sake, ensure that all errors are filtered
9091
BaselineUtils expectedBaselineUtils = new BaselineUtils(expectedBaselineFile);
9192
var filteredWithExpected = expectedBaselineUtils.FilterErrorsUsingBaseline(actualErrors);
92-
93+
9394
// This piece is for sanity, to verify that the the filter only filters out SingleLineErrors and leaves
9495
// the BlockFormattingErrors.
9596
if (expectedNumberOfBlockErrors > 0)

‎tools/codeowners-utils/Azure.Sdk.Tools.CodeownersLinter/Azure.Sdk.Tools.CodeownersLinter.csproj‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,7 @@
1010

1111
<ItemGroup>
1212
<PackageReference Include="System.CommandLine" Version="2.0.0-beta4.22272.1" />
13+
<PackageReference Include="Microsoft.Extensions.Logging.Console" Version="8.0.0" />
1314
</ItemGroup>
1415

1516
<ItemGroup>

0 commit comments

Comments
 (0)