Skip to content
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

Revert "Simplify codeowners" #4860

Open
wants to merge 1 commit into
base: develop
Choose a base branch
from

Conversation

Saransh-cpp
Copy link
Member

Reverts #4850

The CODEOWNER change did exactly what I thought. Requesting a review "subscribes" me to the PR and then I get notification for everything happening on that PR (even after someone reviews it).

I guess a PR editing just the tests can request a review manually (as these PRs are rare). Alternatively, we can add the tests/ directory in CODEOWNERS.

@Saransh-cpp Saransh-cpp requested a review from a team as a code owner February 18, 2025 12:11
Copy link

codecov bot commented Feb 18, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 98.70%. Comparing base (0d5af5c) to head (2dfdfb2).

Additional details and impacted files
@@           Coverage Diff            @@
##           develop    #4860   +/-   ##
========================================
  Coverage    98.70%   98.70%           
========================================
  Files          304      304           
  Lines        23432    23432           
========================================
  Hits         23129    23129           
  Misses         303      303           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

Copy link
Member

@agriyakhetarpal agriyakhetarpal left a comment

Choose a reason for hiding this comment

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

Requesting a review "subscribes" me to the PR and then I get notification for everything happening on that PR (even after someone reviews it)

I had the same experience. Removing other maintainers from review requests is apparently not implemented yet and is something GitHub should consider as a feature request, (as I mentioned).

Reverting sounds good to me. Thanks!

Copy link
Contributor

@kratman kratman left a comment

Choose a reason for hiding this comment

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

Can we give this a week or so to try it out? It only really works on new PRs. My change was merged 2 days ago and only 4 PRs were created since then. 2 are un-reviewed, one was merged immediately after review, and then this one

@kratman
Copy link
Contributor

kratman commented Feb 18, 2025

Here is an example of how it is working:
image

After Valentin's review, other review requests are gone:
image

@Saransh-cpp
Copy link
Member Author

Ah, sorry, I did not explain it well. Even when the maintainer team disappeared from the review section, GitHub did not "unsubscribe" me from the PR (at the bottom of the screenshot), which means that I get a notification for everything going inside that PR. For instance, getting a notification that the PR was merged -

image

An issue for which I do not get any notifications because I am not "subscribed" automatically -

image

I am okay with waiting for a few more PRs if I am missing something!

@Saransh-cpp
Copy link
Member Author

@kratman can you please look at this? I have been getting notifications for every event on every PR. See my comment above for reference.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants