Various improvements to team member validation

[steffahn]

• refactor the duplication check
• add checks for duplicate roles of a person in a team
• check against invalid roles of alumni

supersedes both #2701 and #2702

[github-actions]

Dry-run check results

[WARN  rust_team::sync] sync-team is running in dry mode, no changes will be applied.
[INFO  rust_team::sync] synchronizing crates-io
[INFO  rust_team::sync] synchronizing github

[steffahn]

By running the overlap check only on the already individually-deduplicated list of members+alumni, something like e.g. a duplicate alumni won't result in a sequence of multiple of error messages as it currently does

before

[ERROR rust_team::validate] validation error: team `myteam` has duplicate alumni: foobar
[ERROR rust_team::validate] validation error: team `myteam` has duplicate leads + alumni: foobar
[ERROR rust_team::validate] validation error: team `myteam` has duplicate members + alumni: foobar

after

[ERROR rust_team::validate] validation error: team `myteam` has duplicate alumni: `foobar`

---

The choice of IndexMap and BTreeSet in the new no_duplicates code produces less arbitrary orderings, no longer based on hashes but on order of (first) occurrence in the files.

[steffahn]

Prepending a revert commit of #2701 and rebasing to remove the merge conflict.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[steffahn]

All code that #2701 (or its revert) touches are changed here anyway, so I might as well squash it then.

[ubiratansoares]

Hi @steffahn , apologies for being late to this review.

I've pulled this code, and tried adding the following new team to our integration tests.

name = "transitive-inclusions"
top-level = true

[people]
leads = [
    "user-0"
]
members = [
    "user-3",
]
alumni = [
    "user-0"
]
included-teams = [
    "foo"
]

[website]
name = "Demo Team"
description = "Why do you care about the description of test teams?"
page = "demo"
email = "foo@example.com"
repo = "https://github.com/ghost/foo"
zulip-stream = "t-foo"
matrix-room = "#t-foo:example.com"
weight = 1000

We no longer catch the overlap : rather than a validation error, we have a new version of a fake team_api to bless

{
  "name": "transitive-inclusions",
  "kind": "team",
  "subteam_of": null,
  "top_level": true,
  "members": [
    {
      "name": "Zeroth user",
      "github": "user-0",
      "github_id": 0,
      "is_lead": true
    },
    {
      "name": "First user",
      "github": "user-1",
      "github_id": 0,
      "is_lead": false
    },
    {
      "name": "Third user",
      "github": "user-3",
      "github_id": 3,
      "is_lead": false
    }
  ],
  "alumni": [
    {
      "name": "Zeroth user",
      "github": "user-0",
      "github_id": 0,
      "is_lead": false
    }
  ],

The main branch will report a validation error.

# previous output
[ERROR rust_team::validate] validation error: team `transitive-inclusions` has duplicate leads + alumni: user-0
[ERROR rust_team] 1 validation errors found
Error: command returned a non-zero exit code!


failures:
    static_api

test result: FAILED. 1 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out; finished in 4.24s

I assume the main branch holds the correct behaviour, since include-teams can contribute users. I also tested a marker team just in case.

Hope I'm not missing anything. In any case, I think a few tests could be useful to avoid regressions.

[ubiratansoares]

Probably a typo here?

mems_and_alumni

[steffahn]

If I remember correctly, I was gaming rustfmt not to add more line breaks somewhere (don't recall if that was here or where it's used) and needed to save a letter

[steffahn]

Oh, that's unfortunate that I produced a logic regression. Thank you for pointing out the regression tests, I'll add some more tests (e.g. so for the new checks introduced here) and fix the logic!

[steffahn]

@rustbot author