Remove deprecated logging methods #6516
Conversation
|
Claude finished @justindbaur's task —— View job Code Review CompleteSummary of ChangesThis PR successfully removes deprecated logging methods that have been deprecated for 3+ months per the release notes. The changes align with ADR-0021 (Standard Output Logging) and eliminate the custom Sentry and Syslog logging integrations in favor of standard output logging with Key Changes:
Critical Issues FoundNone. Suggested Improvements1. Empty XML Documentation CommentsLocation: /// <summary>
///
/// </summary>
/// <param name="hostBuilder"></param>
/// <returns></returns>
public static IHostBuilder AddSerilogFileLogging(this IHostBuilder hostBuilder)The XML documentation for Suggested documentation/// <summary>
/// Configures file-based logging using Serilog for production environments.
/// Supports both legacy GlobalSettings configuration and new Logging:PathFormat configuration.
/// </summary>
/// <param name="hostBuilder">The host builder to configure.</param>
/// <returns>The configured host builder for method chaining.</returns>Severity: Minor - The method name is self-documenting, but proper XML docs would be more consistent with project standards. 2. Consider Adding Obsolete Warnings to Remaining NuGet Packages in Renovate ConfigLocation: The PR removes Severity: Informational - This is forward-looking and not an issue with the current changes. 3. Missing Unit Test for Legacy Configuration Without LogDirectoryLocation: The updated tests cover:
However, there's no explicit test for the early return path when legacy configuration has no Suggested test case[Fact]
public void AddSerilog_LegacyConfig_NoLogDirectory_AddsNoProviders()
{
var providers = GetProviders(new Dictionary<string, string?>
{
{ "GlobalSettings:ProjectName", "Test" },
{ "GlobalSettings:LogDirectory", "" },
});
Assert.Empty(providers);
}Severity: Minor - The code path is simple and unlikely to break, but test coverage would be more complete. Good Practices Observed
Action Items for Author
ConclusionThis is a well-executed cleanup PR that successfully removes deprecated logging infrastructure while maintaining backward compatibility for legacy configurations. The changes are consistent, comprehensive, and align with the documented ADR. The simplified code is easier to understand and maintain. Recommendation: ✅ Approve - The PR is ready to merge. The suggested improvements are minor and optional. Todo List
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #6516 +/- ##
==========================================
+ Coverage 51.86% 51.93% +0.06%
==========================================
Files 1902 1891 -11
Lines 84057 83808 -249
Branches 7501 7469 -32
==========================================
- Hits 43598 43524 -74
+ Misses 38765 38593 -172
+ Partials 1694 1691 -3 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Great job! No new security vulnerabilities introduced in this pull request |
| { | ||
| { "GlobalSettings:ProjectName", "Test" }, | ||
| { "GlobalSetting:LogDirectoryByProject", "true" }, | ||
| { "GlobalSettings:LogDirectoryByProject", "true" }, |
🎟️ Tracking
📔 Objective
Removes deprecated logging methods in accordance with https://contributing.bitwarden.com/architecture/adr/standard-output-logging and these methods having been deprecated for 3 months https://bitwarden.com/help/releasenotes/#2025-7-3
📸 Screenshots
⏰ Reminders before review
🦮 Reviewer guidelines
:+1:) or similar for great changes:memo:) or ℹ️ (:information_source:) for notes or general info:question:) for questions:thinking:) or 💭 (:thought_balloon:) for more open inquiry that's not quite a confirmed issue and could potentially benefit from discussion:art:) for suggestions / improvements:x:) or:warning:) for more significant problems or concerns needing attention:seedling:) or ♻️ (:recycle:) for future improvements or indications of technical debt:pick:) for minor or nitpick changes