Hygiene script#3327
Conversation
maryliag
requested review from
a team,
alolita,
austinlparker,
jpkrohling,
mtwo,
mx-psi,
svrnm,
tedsuo and
trask
as code owners
|
Results of the script: (latests itaration) INACTIVE MEMBERS REPORT
|
|
Besides the repo I've already excluded, I'm considering removing the following ones I would like a feedback to confirm if make sense:
|
|
For I'd like to suggest evaluating activity for an individual within any repository their group is responsible, and then we can also evaluate, separately, aggregate activity for a group in a repository. Regarding initial results though - this is good data to have. I have been putting off going to emiritus status for C++ and Java approver, but given the current intensity of semantic conventions work I just don't think I'll be able to contribute meaningfully there as an approver. |
Could you expand on this ? Where does "the SIG is shutting down" comes from ? |
This repository is very low traffic, but is it not dead: it is stable, and deliverables there (docker images) are used daily. Tooling very rarely needs to be adjusted, hence the lack of activity. |
From here: #3297 |
That is what is doing already, so if a user is part of team X and that team has 5 repos to it, if they're active into any of those 5, they're consider active in general and I don't tag that into any of the 5 repos "assigned" to that team. The example you provided, that user is not marked as active in the weaver repo as you mentioned. You can actually see that user got flagged as inactive into the 3 repos the team
I'm glad this is already helping :D |
|
Thanks for the data, it's really useful! A few questions:
|
|
Thanks for sharing the data, this is helpful. For I was also one of the early contributors to the C++ SIG and involved in the initial implementation of several signals, so I have historical context around parts of the implementation that can still be helpful when questions or design discussions come up. We also actively use otel-cpp within Microsoft and plan to continue doing so, so it would be good for me to remain on the maintainer list for those repos. |
I have some questions on this. Having a report to see activity in general and raise flags is one thing, and is good. The question then is what to do with the results. I am concerned with the workflow and how automated this will be, possibly locking everyone out of a given repository just because not much happens there. Even if there is a workflow that files PR automatically, I assume maintainers will have the final decision on what to do, correct ? |
|
A technical comment here: When someone is part of x-maintainers and x-approvers, any activity such as:
will NOT count as maintainer activity, it will be reported as approver activity for code reviews, or not at all. The only activity that will register as a maintainer task is to simply click on the merge button (and maybe make a release ?). A workflow that starts to evict maintainers who otherwise participate in a repository, but just do not perform merges, is dangerous in my opinion, activity should be evaluated globally. Edit: This is based on my understanding of github privileges and when the maintainer priviledge is checked, this is not based on the script logic to check maintainer activity. |
|
One observation on the maintainer activity check in the script: it looks like the current logic requires authored PRs in addition to reviews/comments in order to consider a maintainer active. That might be a bit strict. According to the maintainer role description, activity can also show up through reviewing proposals and PRs, contributing to discussions, helping with design direction, etc. In more mature or stable repositories, maintainers may stay active mostly through review and guidance rather than regularly authoring PRs. So requiring authored PRs as a mandatory signal may undercount maintainers who are still actively involved in the project. |
I think maybe we should check the script? Laurent just helped review our release and a few other tidbits on friday, is active in our weekly SIG and I believe has reviewed PRs within the bounds of your script (past 2 weeks). I'm actually surprised to see his name on the list for weaver and its associated repositories then, apologies I missed it was also on the otel/weaver. |
|
Thank you all for the comments! First thing, this is a script based on the proposal displayed here. That is the issue that discuss what you need to do to be consider active in each role, but I also want to acknowledge that are case by case scenarios that need changes, this is why I created the example of a result and shared here, so we can analyze these cases and make the proper updates (to the proposal and/or script). Now, let me answer the questions/comments: From @lmolkova:
From @lalitb:
From @marcalff
From @jsuereth
Hope this info has helped! 😅 |
|
The first comment is updated to the latest result after script changes (ignoring new repos, and removing If there is an agreement, I can also add |
|
I work on otel stuff every day in my role at elastic. Just haven't been contributing upstream. happy to do so more if that helps |
it's hard because there are so many edge cases that we will probably going to catch only with real data. But people can always tag me if adjustments need to be made 🙏 |
|
@jeremydvoss, @ericmustin & everyone that is messaging me asking to not be removed: This PR is not removing anyone yet. This is showing the example of results and once merged it will be executed monthly, and that will then create a PR to the respective repos for people flagged as inactive to be removed and it's up to the maintainers of each repo to approve the change or not. Once that PR is open, you can discuss with the maintainers of the repo if you should be removed. Keep in mind that we want to show a realistic state of each SIG, so if you're not contributing but have status it might give the wrong impression that the repo has a higher amount of active maintainers/approver/triagers than it actually have, and that causes problem for the SIG health. Reach out to the maintainers of your SIG on how you can better contribute, and don't forget that each role has a set of responsibilities and that you can be removed if you're not doing them: Hope that helps to clarify! |
|
Just waiting on some feedback if the following repos should be added to the ignore list: opentelemetry-proto* The way the script is now is hard to identify for low traffic repos, so a following update could be checking specific cases, per folder/ownership for these particular repos. So I'm happy to remove them for now if people think it make sense and don't keep creating false positives. |
I would rather remove them from the list and start smaller for now |
|
Another special use case here:
Doug was just nominated maintainer, and just added to cpp-maintainers and cpp-contrib-maintainers. See: Maybe check the date when someone was added in a team ? |
|
I'm happy to be moved to emeritus for my role as OTel Collector triager. I do intend on coming back to the Collector SIG, but I believe that my membership should reflect the current reality, and not a promise for the future. It's only fair to the current triagers for me to step down. Once I have enough contributions again on that SIG, I'd be proud to regain the privileges. |
|
@marcalff there is no event to know when a person was added to the team, but that shouldn't really matter because we expect somebody to be active if they're being promoted. For this particular case I can see you have In this case I have 2 suggestions: |
Thanks for the investigations. No changes to the hygiene script then. We need probably to implement (1) at some point, until then we will review PRs generated by the script and amend when needed. |
15 participants
This is analyzing which members are no longer active according to the rules, and then generating the final list. If for a particular repo, a user is part of multiple teams (x-triager, x-approver) this is listing all those teams, so it's clear they should be removed from both.
There are a few repos excluded (since it doesn't make sense), similar to particular teams and repos combination.
The first comment will add the result of this script, this way I would like to get a review if there are any other specific exceptions that need to be added.
The script also generates a PR on each respective repo with the updates on the readme file. The maintainers still need to manually remove them from the teams. The workflow in this PR executes the script monthly. Examples of a PR creation can be found:
For now, this is handling triagers, approvers and maintainers.
This script is based on the proposal found here: #3295