Update Github CodeTeam Plugin to enforce github.team presence

[timlkelly]

Issue

The Github CodeTeam Plugin only enforces that more than one CodeTeam does not have a duplicate github.team value. The validation error message is not specific if more than one CodeTeam do not have github.team keys.

Given two CodeTeam YAML files with no github.team key, the validation errors produces the follow error message:

irb(main):001:0>  CodeTeams.validation_errors(CodeTeams.all)
=> ["The following teams are specified multiple times:\nEach code team must have a unique GitHub team in order to write the CODEOWNERS file correctly.\n\n\n"]

It is not clear which CodeTeams are the offenders.

Solution

Update the Plugin to check that each CodeTeam has a github.team key.

irb(main):001:0> CodeTeams.validation_errors(CodeTeams.all)
=>
["The following teams are missing `github.team` entries\n\nconfig/teams/one.yml\nconfig/teams/two.yml\n",
 "The following teams are specified multiple times:\nEach code team must have a unique GitHub team in order to write the CODEOWNERS file correctly.\n\n\n"]

This still prints the original, unspecific error message, but at least there's context on the files that are missing github.team keys. If we want to remove the nil Github teams from the original error message, we could #compact on the flat_map.

Reproduction

• Create a new rails project, rails new
• bundle add code_teams
• bundle add code_ownership
• Add the following CodeTeam YAML files to config/teams

  # config/teams/one.yml
  ---
  name: one

  # config/teams/two.yml
  ---
  name: two

• Start a rails console and run CodeTeams.validation_errors(CodeTeams.all)

[alexevanczuk]

This change makes sense, but one concern is that this is technically a breaking change since it was previously allowed to be nil and now it is no longer allowed to be.

One alternative to simplify this is to just ignore teams that do not have this set when specifying teams used more than once. That way it wouldn't be considered an error and wouldn't show up in the confusing way you described in the PR description.

Let me know what ya think @timlkelly

[timlkelly]

Great call out! Thanks for bringing that up. Is there interest in a separate team plugin to require the Github key? Then it could potentially be opt-in and a non-breaking change.

[alexevanczuk]

That could be a separate plugin. Another possibility is if the code ownership configuration YML file could support an optional configuration flag, e.g. require_github_teams that errors if any are nil. That way it can be opt-in without needing to fragment the plugin.

[timlkelly]

@alexevanczuk good idea. I've updated it to include a new configuration key. I'm new to sorbet, so let me know if I missed anything there.

[alexevanczuk]

Looks great to me!

Two small things:

• could you add a quick test that checks for those validation errors
• could ya bump the minor version? I think we can consider this a non-breaking change since it only affects folks with exactly one team without a name (otherwise they'd get a duplicate name issue), and I think this is mostly a bug fix + new feature.

[alexevanczuk]

This seems good to me! @shageman could you help with merging this one 🙏🏼

[professor]

Thanks @timlkelly for the PR.

My team is dedicating two working sessions each week to review open PRs. We've been using a two-prong approach to tackle the newest open PRs and the oldest open PRs during each session.

[timlkelly]

Thanks @timlkelly for the PR.

My team is dedicating two working sessions each week to review open PRs. We've been using a two-prong approach to tackle the newest open PRs and the oldest open PRs during each session.

Okay! Thanks for letting me know. I've updated the Gemfile.lock which was likely causing the failed Github Action pipelines. If you can please kick it off again.