-
Notifications
You must be signed in to change notification settings - Fork 1
feat: implement issue #61 — Compliance: codeowners-org-leads-not-first #552
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Merged
Changes from all commits
Commits
Show all changes
26 commits
Select commit
Hold shift + click to select a range
ac8b783
feat: implement issue #61 — Compliance: codeowners-org-leads-not-first
donpetry-bot eab6d7e
chore: apply manual instructions [skip ci-relay]
donpetry-bot cab4154
Merge branch 'main' into dev-lead/issue-61-20260610-1417
don-petry 780fe17
Merge branch 'main' into dev-lead/issue-61-20260610-1417
don-petry 0fcb2ec
Merge branch 'main' into dev-lead/issue-61-20260610-1417
don-petry 9d64239
Merge branch 'main' into dev-lead/issue-61-20260610-1417
don-petry dd11300
Merge branch 'main' into dev-lead/issue-61-20260610-1417
don-petry d354c5d
Merge branch 'main' into dev-lead/issue-61-20260610-1417
don-petry f88deaf
Merge branch 'main' into dev-lead/issue-61-20260610-1417
don-petry dd3b18c
Merge branch 'main' into dev-lead/issue-61-20260610-1417
don-petry e33fdc6
Merge branch 'main' into dev-lead/issue-61-20260610-1417
don-petry 90787a4
Merge branch 'main' into dev-lead/issue-61-20260610-1417
don-petry 7de75cd
Merge branch 'main' into dev-lead/issue-61-20260610-1417
don-petry e47a6f1
Merge branch 'main' into dev-lead/issue-61-20260610-1417
don-petry 9eaf435
Merge branch 'main' into dev-lead/issue-61-20260610-1417
don-petry 979c1f5
Merge branch 'main' into dev-lead/issue-61-20260610-1417
don-petry eb7fa4c
Merge branch 'main' into dev-lead/issue-61-20260610-1417
don-petry cbabe7e
Merge branch 'main' into dev-lead/issue-61-20260610-1417
don-petry c50ff37
Merge branch 'main' into dev-lead/issue-61-20260610-1417
don-petry e94a595
Merge branch 'main' into dev-lead/issue-61-20260610-1417
don-petry 9b23306
Merge branch 'main' into dev-lead/issue-61-20260610-1417
github-actions[bot] e0ab48e
Merge branch 'main' into dev-lead/issue-61-20260610-1417
github-actions[bot] a69422d
Merge branch 'main' into dev-lead/issue-61-20260610-1417
github-actions[bot] 597c642
Merge branch 'main' into dev-lead/issue-61-20260610-1417
github-actions[bot] 340cfdf
Merge branch 'main' into dev-lead/issue-61-20260610-1417
github-actions[bot] 6616494
Merge branch 'main' into dev-lead/issue-61-20260610-1417
don-petry File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or 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
This file contains hidden or 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
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Running
printfandawkin a subshell for every line of the file is highly inefficient in Bash because it forks two new processes per line. This can make the lint script noticeably slow, especially on Windows or macOS environments.Additionally, this approach has two correctness issues:
CODEOWNERSfile has CRLF (\r\n) line endings, the trailing\rwill remain on the line and cause the validation to fail.path\\ with\\ spaces/ @owner),awkwill split on the space and incorrectly identify the second part of the path as the owner.We can resolve all of these issues efficiently using pure Bash built-ins and array splitting.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Implemented the suggestion in
scripts/dev-lead-lint.sh(lines 108–116): replaced theprintf '%s\n' "$line" | awk '{print $2}'subshell with pure Bash builtins — addedline="${line%$'\r'}"to strip CRLF,clean_line="${line//\\ /}"to handle escaped spaces in paths, andread -r -a parts <<< "$clean_line"to split on whitespace without forking processes. Also added three new bats tests intests/dev-lead/unit/test_dev_lead_lint.batscovering CRLF handling, escaped-space paths, and CRLF-with-wrong-owner; all 22 tests pass.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Confirmed implemented.
scripts/dev-lead-lint.shlines 108–119 now use pure Bash builtins matching the suggestion exactly:line="${line%$'\r'}"strips CRLF,clean_line="${line//\\ /}"collapses escaped spaces before splitting, andread -r -a parts <<< "$clean_line"replaces the per-lineprintf | awksubshell. Three corresponding bats tests (lines 256–279 intests/dev-lead/unit/test_dev_lead_lint.bats) cover CRLF-clean, escaped-space paths, and CRLF-with-wrong-owner. ShellCheck passes with no warnings.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Verified as implemented:
scripts/dev-lead-lint.shlines 108–119 now match the suggestion exactly —line="${line%$'\r'}"strips CRLF,clean_line="${line//\\ /}"collapses escaped spaces, andread -r -a parts <<< "$clean_line"replaces the per-lineprintf | awksubshell. Three bats tests (lines 256–279 intests/dev-lead/unit/test_dev_lead_lint.bats) cover CRLF-clean, escaped-space paths, and CRLF-with-wrong-owner. ShellCheck passes with no warnings.