Skip to content

Conversation

@grahamking
Copy link
Contributor

@grahamking grahamking commented Sep 19, 2025

Manually adding and removing people from these lists doesn't really scale.

Summary by CodeRabbit

  • Chores
    • Consolidated repository ownership settings in CODEOWNERS from individual maintainers to team-based groups across multiple areas.
    • Standardized ownership coverage for examples, multimodal examples, planner components, benchmarks, tests, and container-related code.
    • No changes to CI/CD or deployment ownership mappings.
    • No impact on product functionality or user experience.

Manually adding and removing people from these lists doesn't really
scale.

Signed-off-by: Graham King <[email protected]>
@grahamking grahamking requested review from a team and nnshah1 as code owners September 19, 2025 15:37
@github-actions github-actions bot added the chore label Sep 19, 2025
@coderabbitai
Copy link
Contributor

coderabbitai bot commented Sep 19, 2025

Walkthrough

Consolidates CODEOWNERS entries by replacing long per-user owner lists with team-based ownership for specific directories: container, examples (including multimodal), and planner-related paths (components/planner, benchmarks/profiler, tests/planner). Other paths remain unchanged.

Changes

Cohort / File(s) Summary of Changes
Container
/container/
Owners changed from multiple individual users to teams: @ai-dynamo/Devops @ai-dynamo/dynamo-rust-codeowners @ai-dynamo/python-codeowners.
Examples (root)
/examples/
Owners consolidated to teams: @ai-dynamo/Devops @ai-dynamo/dynamo-rust-codeowners @ai-dynamo/python-codeowners (replacing individual users).
Examples — Multimodal
/examples/multimodal/
Owners replaced with team groups: @ai-dynamo/python-codeowners @ai-dynamo/Devops (removing individual users).
Planner-related
/components/planner/, /benchmarks/profiler/, /tests/planner/
Owners switched from several individuals to: @ai-dynamo/python-codeowners @ai-dynamo/Devops.

Sequence Diagram(s)

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

Poem

I thump my paw, approvals near,
Teams take the helm, the paths are clear.
No more long lists, just tidy rows—
Devops, Rust, and Python pros.
In CODEOWNERS fields I softly cheer,
Hop-hop, the map is crystal here. 🐇✨

Pre-merge checks

❌ Failed checks (1 warning)
Check name Status Explanation Resolution
Description Check ⚠️ Warning The PR description is a single sentence and does not follow the repository's required template; it omits the Overview, Details, "Where should the reviewer start?", and Related Issues sections, so it lacks a clear summary of which CODEOWNERS files/paths changed, the rationale for each change, and reviewer guidance. Because those sections are missing, reviewers won't have pointers to the modified files, any linked issue numbers, or notes on potential impacts to CI or review workflows. This makes it difficult to assess scope and prioritize review. Please update the PR to use the repository description template: add an Overview stating the intent to migrate ownership from individuals to teams, a Details section enumerating the exact CODEOWNERS paths changed and the rationale, a "Where should the reviewer start?" pointing to the CODEOWNERS file(s) and key diffs, and a Related Issues line linking or closing any relevant issue; also mention any expected impact on CI or review flows and an estimated review effort.
✅ Passed checks (2 passed)
Check name Status Explanation
Title Check ✅ Passed The PR title "chore(CODEOWNERS): Manage permissions by team not by individual" concisely and accurately summarizes the primary change of converting CODEOWNERS entries from individual users to team-based owners across multiple paths; it follows conventional-commit style and is specific enough for a reviewer scanning history.
Docstring Coverage ✅ Passed No functions found in the changes. Docstring coverage check skipped.

Tip

👮 Agentic pre-merge checks are now available in preview!

Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.

  • Built-in checks – Quickly apply ready-made checks to enforce title conventions, require pull request descriptions that follow templates, validate linked issues for compliance, and more.
  • Custom agentic checks – Define your own rules using CodeRabbit’s advanced agentic capabilities to enforce organization-specific policies and workflows. For example, you can instruct CodeRabbit’s agent to verify that API documentation is updated whenever API schema files are modified in a PR. Note: Upto 5 custom checks are currently allowed during the preview period. Pricing for this feature will be announced in a few weeks.

Please see the documentation for more information.

Example:

reviews:
  pre_merge_checks:
    custom_checks:
      - name: "Undocumented Breaking Changes"
        mode: "warning"
        instructions: |
          Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).

Please share your feedback with us on this Discord post.


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.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 0

🧹 Nitpick comments (1)
CODEOWNERS (1)

29-31: *Planner rules override global .rs; include Rust team if needed.

These directory rules supersede the earlier “*.rs → rust team.” If any Rust files live under these paths, Rust owners won’t be auto‑requested. Add the Rust team if that’s unintended.

-/components/planner/ @ai-dynamo/python-codeowners @ai-dynamo/Devops
+/components/planner/ @ai-dynamo/python-codeowners @ai-dynamo/dynamo-rust-codeowners @ai-dynamo/Devops
-/benchmarks/profiler/ @ai-dynamo/python-codeowners @ai-dynamo/Devops
+/benchmarks/profiler/ @ai-dynamo/python-codeowners @ai-dynamo/dynamo-rust-codeowners @ai-dynamo/Devops
-/tests/planner/ @ai-dynamo/python-codeowners @ai-dynamo/Devops
+/tests/planner/ @ai-dynamo/python-codeowners @ai-dynamo/dynamo-rust-codeowners @ai-dynamo/Devops
📜 Review details

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between a8fd127 and 93485d6.

📒 Files selected for processing (1)
  • CODEOWNERS (1 hunks)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
  • GitHub Check: Build and Test - vllm
  • GitHub Check: Build and Test - dynamo
🔇 Additional comments (2)
CODEOWNERS (2)

13-13: Team-based owners for /container/ — LGTM; confirm intent on broad language coverage.

Change aligns with the PR goal. Please confirm you want both Rust and Python teams pinged for all container changes; if not, consider narrowing by subdirs (e.g., /container/python/, /container/rust/) or keeping Devops only.


19-21: Replace individual owners under /examples/*/deploy/ with team(s); confirm team slugs & repo perms

  • CODEOWNERS (lines 19–21): line 20 lists individual users — switch to team ownership (examples: /examples/*/deploy/ -> @ai-dynamo/Devops or @ai-dynamo/Devops @ai-dynamo/python-codeowners @ai-dynamo/dynamo-rust-codeowners).
  • Verification failed: the provided gh API check returned 404 for ai-dynamo team "devops"; cannot confirm team slugs or repo write permissions. Confirm exact team slugs and that those teams have write access to ai-dynamo/dynamo before applying the change.

@grahamking grahamking enabled auto-merge (squash) September 19, 2025 16:55
@grahamking grahamking disabled auto-merge September 19, 2025 16:56
Now 100% individual free!

Signed-off-by: Graham King <[email protected]>
Copy link
Contributor

@nv-anants nv-anants left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

love the branch name!! ❤️

@grahamking grahamking merged commit 8534d17 into main Sep 19, 2025
13 of 15 checks passed
@grahamking grahamking deleted the no-i-in-dynamo branch September 19, 2025 18:13
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants