OWNERS: sync master to release-8.5 - #70406
ti-chi-bot wants to merge 0 commit into
Conversation
ti-chi-bot
added
approved
cherry-pick-approved
|
Adding the "do-not-merge/release-note-label-needed" label because no release-note block was detected, please follow our release note process to remove it. DetailsInstructions for interacting with me using PR comments are available here. If you have questions or suggestions related to my behavior, please file an issue against the kubernetes-sigs/prow repository. |
ti-chi-bot
Bot
added
do-not-merge/release-note-label-needed
📝 WalkthroughWalkthroughThe PR replaces static ownership lists with path-based ChangesOwnership migration
Estimated code review effort: 4 (Complex) | ~45 minutes Possibly related issues
Possibly related PRs
Suggested labels: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1🛠️ Fix failing CI checks 💡
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/executor/OWNERS`:
- Around line 8-10: Add a catch-all approver rule to the pkg/executor OWNERS
configuration using sig-approvers-executor, while preserving the existing
analyze.*\.go$ rule and no_parent_owners behavior so all otherwise-unmatched
executor files receive default coverage.
In `@pkg/sessionctx/vardef/OWNERS`:
- Around line 5-10: Add a catch-all approver rule to the OWNERS configuration
for files not matched by the existing BUILD.bazel or critical sysvar/tidb_vars
rule, using the intended package approver alias. Preserve the specialized
sig-critical-approvers-tidb-server rule unchanged and ensure the default pattern
matches all remaining files under pkg/sessionctx/vardef.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Repository UI
Review profile: CHILL
Plan: Pro Plus
Run ID: f228d3f3-b6c9-4f35-afd5-16376d5a3814
📒 Files selected for processing (47)
OWNERSOWNERS_ALIASESbr/OWNERSbr/pkg/storage/OWNERSdumpling/OWNERSlightning/OWNERSlightning/cmd/tidb-lightning-ctl/OWNERSlightning/cmd/tidb-lightning/OWNERSpkg/autoid_service/OWNERSpkg/bindinfo/OWNERSpkg/config/OWNERSpkg/ddl/OWNERSpkg/distsql/OWNERSpkg/disttask/OWNERSpkg/domain/OWNERSpkg/dumpformat/OWNERSpkg/dxf/OWNERSpkg/executor/OWNERSpkg/executor/aggfuncs/OWNERSpkg/executor/aggregate/OWNERSpkg/executor/importer/OWNERSpkg/executor/join/OWNERSpkg/executor/mppcoordmanager/OWNERSpkg/executor/sortexec/OWNERSpkg/executor/test/analyzetest/OWNERSpkg/executor/unionexec/OWNERSpkg/expression/OWNERSpkg/infoschema/OWNERSpkg/ingestor/OWNERSpkg/lightning/OWNERSpkg/lightning/config/OWNERSpkg/lock/OWNERSpkg/meta/OWNERSpkg/meta/metadef/OWNERSpkg/metrics/OWNERSpkg/objstore/OWNERSpkg/owner/OWNERSpkg/parser/OWNERSpkg/planner/OWNERSpkg/resourcemanager/OWNERSpkg/session/OWNERSpkg/sessionctx/vardef/OWNERSpkg/sessionctx/variable/OWNERSpkg/statistics/OWNERSpkg/table/OWNERSpkg/tablecodec/OWNERSpkg/util/naming/OWNERS
💤 Files with no reviewable changes (2)
- br/pkg/storage/OWNERS
- pkg/disttask/OWNERS
| "analyze.*\\.go$": | ||
| approvers: | ||
| - sig-approvers-stats |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files 'pkg/executor/OWNERS' 'pkg/executor/aggfuncs/OWNERS' 'pkg/executor/aggregate/OWNERS' 'pkg/executor/*_*.go' 'pkg/executor/*.go' 2>/dev/null | sed -n '1,120p'
echo
echo "== pkg/executor OWNERS =="
if [ -f pkg/executor/OWNERS ]; then cat -n pkg/executor/OWNERS; fi
echo
echo "== sibling OWNERS snippets =="
for f in pkg/executor/aggfuncs/OWNERS pkg/executor/aggregate/OWNERS; do
[ -f "$f" ] && { echo "--- $f"; cat -n "$f"; }
done
echo
echo "== root-level execute go filters/search =="
python3 - <<'PY'
import pathlib, re
root = pathlib.Path('pkg/executor')
files = sorted([p for p in root.iterdir() if p.is_file() and p.name.endswith('.go')])
patterns = {
'BUILD.bazel': re.compile(r'^BUILD\.bazel$'),
'analyze.*\.go$': re.compile(r'^analyze.*\.go$'),
}
for f in files:
hits = [k for k,v in patterns.items() if v.search(f.name)]
print(f.name, hits or ["NO_FILTER"])
PYRepository: pingcap/tidb
Length of output: 10550
🌐 Web query:
kubernetes owners no_parent_owners filter matching parent directories OWNERS file
💡 Result:
In Kubernetes, the no_parent_owners option is used within an OWNERS file to control inheritance of reviewer and approver permissions from parent directories [1][2]. When set to true, no_parent_owners excludes all parent OWNERS files from having any influence over files adjacent to or underneath the directory where that OWNERS file is located [1][2][3]. By default, if the option is not present, it is treated as false, meaning the configuration inherits permissions from parent directories as usual [1][2]. Key aspects of this feature include: - Functionality: It enables scenarios where a nested OWNERS file needs to completely override or isolate the permissions of its parent directories, effectively "breaking" the inheritance chain for that path [1][2]. - Configuration: It is specified within an options map in the OWNERS file [1][2]. - Interaction with Filters: The options key (including no_parent_owners) is permitted at the top level of the OWNERS file, even when using the filters key [1][2]. While other owner-related keys like approvers and reviewers must be placed inside the filters block, the options configuration remains an exception and is defined outside of it [1][2]. - Implementation: This logic is processed by Prow's repoowners package, which traverses the directory structure and stops inheriting permissions from higher levels once it encounters a directory where no_parent_owners is enabled [3]. Common use cases include locking down specific directories to prevent broader organizational or root-level owners from having automatic approval authority over sensitive code [4][5][6].
Citations:
- 1: https://www.kubernetes.dev/docs/guide/owners/
- 2: https://github.com/kubernetes/community/blob/main/contributors/guide/owners.md
- 3: https://github.com/kubernetes-sigs/prow/blob/master/pkg/repoowners/repoowners.go
- 4: Allow OWNERS files to exclude parent OWNERS kubernetes/test-infra#5197
- 5: https://github.com/kubernetes/kubernetes/blob/master/third_party/OWNERS
- 6: Make root approvers list non-recursive, shift from individuals to sig/subproject aliases kubernetes/kubernetes#112952
Add default approver coverage for pkg/executor.
no_parent_owners: true prevents upstream OWNERS fallback. With only BUILD.bazel and root-level analyze*.go filters, the remaining pkg/executor/*.go files have no matching approver group. Since sibling pkg/executor/aggfuncs/OWNERS and pkg/executor/aggregate/OWNERS use sig-approvers-executor for catch-all coverage, add the same default rule here.
Proposed fix
"analyze.*\\.go$":
approvers:
- sig-approvers-stats
+ ".*":
+ approvers:
+ - sig-approvers-executor📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "analyze.*\\.go$": | |
| approvers: | |
| - sig-approvers-stats | |
| "analyze.*\\.go$": | |
| approvers: | |
| - sig-approvers-stats | |
| ".*": | |
| approvers: | |
| - sig-approvers-executor |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/executor/OWNERS` around lines 8 - 10, Add a catch-all approver rule to
the pkg/executor OWNERS configuration using sig-approvers-executor, while
preserving the existing analyze.*\.go$ rule and no_parent_owners behavior so all
otherwise-unmatched executor files receive default coverage.
Source: MCP tools
| "(^|/)BUILD\\.bazel$": | ||
| approvers: | ||
| - sig-community-approvers | ||
| "(OWNERS|(sysvar|tidb_vars)\\.go)$": | ||
| approvers: | ||
| - sig-critical-approvers-tidb-server |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟠 Major | ⚡ Quick win
Add a default approver rule.
Line 3 disables parent ownership. The current filters do not match other files in pkg/sessionctx/vardef/. Changes to those files will have no approvers.
Add a " .* " equivalent catch-all filter with the intended package approver alias. The specialized rule must remain for the critical files.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/sessionctx/vardef/OWNERS` around lines 5 - 10, Add a catch-all approver
rule to the OWNERS configuration for files not matched by the existing
BUILD.bazel or critical sysvar/tidb_vars rule, using the intended package
approver alias. Preserve the specialized sig-critical-approvers-tidb-server rule
unchanged and ensure the default pattern matches all remaining files under
pkg/sessionctx/vardef.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## release-8.5 #70406 +/- ##
================================================
Coverage ? 42.9554%
================================================
Files ? 221
Lines ? 41848
Branches ? 0
================================================
Hits ? 17976
Misses ? 21441
Partials ? 2431
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
acf9147 to
2a2b259
Compare
ti-chi-bot
Bot
added
size/XS
|
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here. DetailsNeeds approval from an approver in each of these files:Approvers can indicate their approval by writing |
1 participant
What problem does this PR solve?
Sync
OWNERS*files frommastertorelease-8.5.What changed and how does it work?
This PR was auto-generated by Prow and only updates
OWNERSandOWNERS_ALIASESfiles./release-note-none
skip-issue-check
lgtm
approved
cherry-pick-approved
Summary by CodeRabbit