Bugfix: when looking for a match in CODEOWNERS, do not emit error message for commented out paths
#5366
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
konrad-jamrozik
added
the
Central-EngSys
konrad-jamrozik
changed the title
CODEOWNERS paths that are commented out.CODEOWNERS paths that are commented out.
konrad-jamrozik
changed the title
CODEOWNERS paths that are commented out.CODEOWNERS, do not emit error message for commented out paths
added 2 commits
February 9, 2023 18:43
konrad-jamrozik
commented
Feb 10, 2023
...code-owners-parser/Azure.Sdk.Tools.RetrieveCodeOwners.Tests/CodeownersManualAnalysisTests.cs
Outdated
Show resolved
Hide resolved
…sts/CodeownersManualAnalysisTests.cs
benbp
reviewed
Feb 13, 2023
| @@ -352,6 +352,7 @@ private void WriteLangRepoOwnersDiffToCsv(string langName) | |||
| outputLines.AddRange(PathsWithMissingPrefixSlash(entries)); | |||
| outputLines.AddRange(PathsWithMissingSuffixSlash(targetDir, entries, paths)); | |||
| outputLines.AddRange(InvalidPaths(entries)); | |||
| // TODO: add a check here for CODEOWNERS paths that do not match any dir or file. | |||
There was a problem hiding this comment.
Do we need an issue tracking this?
There was a problem hiding this comment.
@benbp not really. I put it here more as a reminder that this validation is currently missing, in the unlikely scenario I (or somebody else) will have to use it again. I forgot at first to include it. But I kind of expect for it to be actually picked up when working on:
This kind of validation is mentioned here:
benbp
approved these changes
Feb 13, 2023
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Before this change, commented out paths in
CODEOWNERSviolated the rule that the paths should start with/. As a result, theConsole.Errwas spammed with error messages, resulting inConsole.Outtruncation forazure-sdk-for-jsandazure-sdk-for-javarepos, making it impossible to deserialize the result written toConsole.Outinto valid json. This brokeCodeownersManualAnalysisTestswithOutOfMemoryExceptionupon getting value fromConsole.Err.This bug was introduced by:
CODEOWNERSmatcher. #5330as that PR stopped prepending missing
/and instead started outputting message toConsole.Errif they were missing.Secondary fix
This PR also fixes a bug related to validating paths.