markbind icon indicating copy to clipboard operation
markbind copied to clipboard

Standardize and document steps wrt to GitHub teams and PR review procedure

Open tlylt opened this issue 3 years ago • 14 comments

Please confirm that you have searched existing issues in the repo

Yes, I have searched the existing issues

Any related issues?

No response

What is the area that this feature belongs to?

No response

Is your feature request related to a problem? Please describe.

We probably don't need an extensive/stage-wise PR review pipeline, but I do think that it could be useful if we have up-to-date GitHub teams to allow for requesting mass PR reviews (more than 1 person) So

  • a cs3281-2023 team containing all latest students in cs3281 (including people of similar capacity)
  • a cs3282-2023 team containing all latest students in cs3282 (including people of similar capacity)
  • an active team (which is what we have in active-devs, though may not be entirely up-to-date)

Describe the solution you'd like

Probably simple documentation on the steps to achieve the above in our project management docs, including a ~ once-a-year maintenance process to review the team settings/make updates every year.

Describe alternatives you've considered

Best if we can automate the above via GitHub Actions or other tools.

Additional context

Suggestions/feedback on the plan.

tlylt avatar Jan 15 '23 03:01 tlylt

I agree this is super useful. Though part of the difficulty is that even the 3282 team members (like myself) do not have access to update github teams with members... Good protection to have that kind of role-based access, but we would also need to amend role permissions since this is not something handled by most.

(And if it's only handled by whoever is the maintainer at the time + @damithc , I am not sure if it should be on the developer docs)

kaixin-hc avatar Feb 24 '24 09:02 kaixin-hc

After looking at the way this is set up, I have the following suggestion for the next batch of mantainers like @lhw-1 -

The current organisation is 6 teams: Screenshot 2024-05-08 at 12 40 50 PM Admins have the admin role, contributors have the triage role, and developers have the write role (push access); the others have admin access. CS3282 is an old team (not current). CS32821 is a new team (I made it). Child teams seem to inherit the permissions of the parent but do not have to inherit the members of the parent; the members seem totally independent. I am not sure if pinging the parent group pings the child group but I would think not.

CS3281 students gradually gain more access to the repository as they become more familiar with it, so it is easier to make them part of a team and adjust the permissions for the whole group as the timeline dictates. I would keep this team and something to create fresh every year when there are multiple students to manage in this way.

Admin probably shouldn't be touched aside from adding at least one person who is more active to the group (and perhaps if @damithc wants to remove those who are not active and move them to a different group)

Active devs is useful and I think all 3282 members should be made mantainers of that team so they can make sure it is updated with new active devs. I have made this change to permissions. But since this is a child of contributors, you don't get push access from being in this group, and 3282 students need that.

I suggest that developer is given maintainer access and CS3282 students are placed under developer; contributor can be used for new contributors, driveby contributors, and non-active alumni.

kaixin-hc avatar May 08 '24 05:05 kaixin-hc

(and perhaps if @damithc wants to remove those who are not active and move them to a different group)

Removed.

damithc avatar May 08 '24 15:05 damithc

@kaixin-hc I have summarized and adjusted the plan according to your comment, perhaps can help verify/let me know what you think?

I plan to remove the dedicated team for cs3281/cs3282 to reduce the number of teams. Though creating a new team for each batch might be easier to upgrade the permissions, I think keeping the number of teams small and remove/add members to the teams with correct permission might be easier to follow. As for pinging purposes, I think we can just use 'active-dev' whenever we need everyone's attention, as long as we keep that team up-to-date

Teams

  • admin: admin access, for members who have admin access to the markbind repo/org -> for Mentor/Team Lead
    • release managers: admin access, for 3282 students who will be making new releases
  • senior developers: push access, renamed from developers -> for cs3282 students (past and present)
  • contributors: triage access, for cs3281 students (past and present) and non-cs3281 active devs
    • active devs: triage access, for everyone active

Workflows:

  • new contributors (non-cs3281): for more than a few contributions -> add to contributors's active dev team
  • new cs3281 students: add to contributor and contributors' active dev team
  • new cs3282 students: add to senior developer team and contributors' active dev team
  • new release manager (from cs3282) -> add to admin's release managers team
  • new team lead -> add to admin team

tlylt avatar May 18 '24 04:05 tlylt

This makes sense to me as well!

kaixin-hc avatar May 31 '24 15:05 kaixin-hc

@tlylt After reading through the issue chain, the suggested team set-up and workflow make sense!

I believe that the necessary changes to the current team set-up is yet to be done - shall we go ahead with it to test it out for this semester @tlylt @damithc? As of right now, we definitely need to update the teams (e.g. cs3281 and cs3282 developers teams are outdated), so it would be a good idea to implement the suggestions in this thread and assess its effectiveness at the end of the semester.

lhw-1 avatar Jan 18 '25 02:01 lhw-1

shall we go ahead with it to test it out for this semester

Hi @lhw-1, im ok. I think you should have access to perform the updates?

so it would be a good idea to implement the suggestions in this thread and assess its effectiveness at the end of the semester.

Sounds good to me, once we have decided we probably should update the dev docs to note this down formally, and close this issue.

tlylt avatar Jan 19 '25 02:01 tlylt

shall we go ahead with it to test it out for this semester @tlylt @damithc?

@lhw-1 I'm OK as well. Go ahead.

damithc avatar Jan 19 '25 02:01 damithc

I've made the necessary changes - will be updating this thread & the dev docs accordingly at the end of the semester.

lhw-1 avatar Jan 19 '25 15:01 lhw-1