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

Permit subteams to have subteams #1046

Merged
merged 1 commit into from
Aug 28, 2023
Merged

Conversation

fmease
Copy link
Member

@fmease fmease commented Aug 17, 2023

This would allow us to create the team rustdoc-contributors (#1040).

Let me know if I should add a test anywhere.
Should we enforce a depth limit?

Outdated information

According to #1040 (comment), nothing should be negatively affected except for the website. I've built its server locally with a patched rust_team_data package (part of this repo) and a patched rust_team_data static API (that contains the new team) and the only consequence is that rustdoc-contributors doesn't show up anywhere on the website (it doesn't crash the server or anything like that). Therefore, updating the website is not a hard blocker.

I've opened rust-lang/www.rust-lang.org#1854 proposing to add proper website support for subsubteams (etc.). I plan on implementing it (or GuillaumeGomez will).

@fmease
Copy link
Member Author

fmease commented Aug 17, 2023

I'm not super familiar with the intricate details of our infra and don't know 100% which tools and services depend on the team repo. As I've said I've only tested the website (of course, you can check this for yourself in case I've overlooked something). Please let me know if I should double-check anything else.

@fmease fmease force-pushed the subsubteams branch 2 times, most recently from 47a4ce6 to c71b888 Compare August 17, 2023 18:40
@fmease fmease marked this pull request as draft August 22, 2023 14:26
@fmease fmease marked this pull request as ready for review August 23, 2023 23:02
@fmease
Copy link
Member Author

fmease commented Aug 23, 2023

Alrighty, website support for subsubteams (etc.) has been merged (rust-lang/www.rust-lang.org#1857).
All blockers should now be resolved for this PR to land, right?

I've updated the crate to handle cycles gracefully and to flag them as errors during validation.
On master, it chokes on any sort of cycle and loops indefinitely.
For example, for the complex cycle alpha ↦ beta, beta ↦ gamma, gamma ↦ alpha, we now emit

[ERROR rust_team::validate] validation error: team `alpha` is part of a cycle
[ERROR rust_team::validate] validation error: team `beta` is part of a cycle
[ERROR rust_team::validate] validation error: team `gamma` is part of a cycle

I still disallow non-teams (working groups, project groups, etc.) to be “subteams” of subteams,
mostly since the website can't support them nicely (see the linked PR as to why).

@fmease
Copy link
Member Author

fmease commented Aug 24, 2023

@rylev, do the changes in this PR look good to you?

(Website with the new changes has now been deployed, rust-lang/www.rust-lang.org#1859)

rylev
rylev previously approved these changes Aug 25, 2023
Copy link
Member

@rylev rylev left a comment

Choose a reason for hiding this comment

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

I've got a small concern about an error message, but otherwise I agree this is ready to merge.

src/validate.rs Outdated Show resolved Hide resolved
@fmease
Copy link
Member Author

fmease commented Aug 25, 2023

alpha ↦ beta, beta ↦ gamma, gamma ↦ alpha now errors with:

[ERROR rust_team::validate] validation error: team `alpha` is a subteam of itself: alpha => beta => gamma => alpha
[ERROR rust_team::validate] validation error: team `beta` is a subteam of itself: beta => gamma => alpha => beta
[ERROR rust_team::validate] validation error: team `gamma` is a subteam of itself: gamma => alpha => beta => gamma

Edit: If you would like me to, I can deduplicate error messages by cycle in order to only show a single message in the case above. Not sure if it's worth it though.

@fmease
Copy link
Member Author

fmease commented Aug 26, 2023

Once this is merged, could you kick off a CI run for #1040 and also merge the latter once it's green (only if you deem the PR sufficiently approved ofc)? Thanks a lot! :)

@rylev
Copy link
Member

rylev commented Aug 28, 2023

Thanks a bunch for working on this!

@rylev rylev merged commit 538e041 into rust-lang:master Aug 28, 2023
1 check passed
@fmease fmease deleted the subsubteams branch August 28, 2023 10:21
@ehuss ehuss mentioned this pull request Jan 14, 2024
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.

2 participants