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

Feature: Team mentor #28

Merged
merged 1 commit into from
Jan 22, 2025
Merged

Conversation

jspaleta
Copy link
Contributor

@jspaleta jspaleta commented Dec 6, 2024

This pull request provides a working implementation to make it possible to use team-management tool in a more transparent way for Cilium project team memberships.

Highlights

  1. Changes to introduce a mentor list to the TeamConfig to encode team members who want to be excluded from the review notification, simplifying the review exclusion process.

  2. Introduction of a team override configuration intended to encode public team member/mentor information on a team by team basis while leaving the full configuration in a private repository.

  3. This is strictly additive changes to how team manager tool operations. It does not interfere with the existing org wide excludeCodeReviewAssignmentFromAllTeams config or the team specific codeReviewAssignment.excludedMembers. The intent is that these other mechanisms can be deprecated after configs are converted to using the team mentor concept, to ensure a smooth transition. Note: the github API doesn't let you query back which userIDs are currently excluded from review, so this is really just a matter of configuration state as a source of truth.

  4. Enhanced user status reporting by the status to indicate where in the team management config users are excluded.

@jspaleta
Copy link
Contributor Author

jspaleta commented Dec 6, 2024

I should note. I've tested locally using the MillionDollarIdea github org.

@jspaleta
Copy link
Contributor Author

jspaleta commented Dec 6, 2024

This work is related to cilium/community#178

@jspaleta jspaleta force-pushed the jspaleta/feature/team-mentors branch from 6910cae to e32cf9b Compare December 10, 2024 18:15
@jspaleta
Copy link
Contributor Author

I've extended this PR to include an optional team override file in the configuration.

The team override file is a subset of the full Team configuration meant to be held in the public view while the full configuration is kept private.

The TeamOverrideConfig includes only team names, and github usernames for team members and mentors.

The team override file is a strict override of team member and mentor lists only on a per team basis, for teams intended to be managed in the public view. Once the CI build is complete I'll update the MillionDollarIdea repo with an example of the override in use.

@jspaleta jspaleta changed the title Feature: Team mentor status Feature: Team mentor Dec 11, 2024
@jspaleta
Copy link
Contributor Author

I now have the MillionDollarIdeas Org updated to make use of the proposed team override from the latest CI build of this PR.

Public team information located at:
https://github.com/MillionDollarIdeas/team-assignments

Full Team management configuration located in private repo:
https://github.com/MillionDollarIdeas/team-management

repo-dispatch event is used to trigger private team-management workflow when public team membership info is updated.

@jspaleta jspaleta force-pushed the jspaleta/feature/team-mentors branch from d6ead62 to 8e2a662 Compare December 11, 2024 23:40
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

I made a few small suggestions. The sorting of the members vs mentors should be fixed before merge. We can discuss whether to fix up the error handling for ineffective overrides or patch that up on top.

If this was in cilium/cilium I'd push for a bit more git hygiene around splitting the commits into each feature (overrides, mentors) and avoiding extra commits to sync with the main branch, but for this repo I think it's less critical since we aren't likely to end up bisecting the git tree to find some bug. I'll probably end up squashing all the commits together.

README.md Outdated Show resolved Hide resolved
pkg/config/sort.go Outdated Show resolved Hide resolved
pkg/config/config.go Outdated Show resolved Hide resolved
@aanm aanm self-requested a review January 17, 2025 18:52
@jspaleta
Copy link
Contributor Author

If you prefer I can squash this down to a single commit or break out override support from mentor support.

…am mentor roles to help teams self-service team members and PR auto-assignment to members

Signed-off-by: Jef Spaleta <[email protected]>
@jspaleta jspaleta force-pushed the jspaleta/feature/team-mentors branch from 5aa472b to 33c4e60 Compare January 17, 2025 20:27
Copy link
Member

@joestringer joestringer left a comment

Choose a reason for hiding this comment

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

LGTM. I see @aanm wanted to take a look too so I'll defer to him for further review / merge.

Copy link
Member

@aanm aanm left a comment

Choose a reason for hiding this comment

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

Thank you!

@aanm aanm merged commit f887243 into cilium:main Jan 22, 2025
2 checks passed
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