Skip to content

Update Github CodeTeam Plugin to enforce github.team presence #79

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.

Sign up for GitHub

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

Merged
merged 10 commits into from
Nov 29, 2023
Merged

Update Github CodeTeam Plugin to enforce github.team presence #79

Changes from 2 commits
Commits
Show all changes
10 commits
Select commit Hold shift + click to select a range
23f996e
Update Github CodeTeam Plugin to enforce `github.team` presence
timlkelly Nov 15, 2023
d8148c6
Use GithubStruct
timlkelly Nov 15, 2023
91dbe38
Ignore CodeTeams without a `github.team` key
timlkelly Nov 15, 2023
694b464
Add `require_github_teams` configuration option
timlkelly Nov 15, 2023
f8e3e1e
Update Github Plugin to enforce required_github_teams
timlkelly Nov 15, 2023
538d035
Fix github_codeowners_up_to_date_spec.rb:759
timlkelly Nov 16, 2023
428b44f
Add spec for require_github_teams configuration
timlkelly Nov 16, 2023
9675d51
Update uniqueness of github teams spec
timlkelly Nov 16, 2023
6406107
Bump minor version to 1.35.0
timlkelly Nov 16, 2023
77338ad
Update Gemfile.lock
timlkelly Nov 28, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 9 additions & 0 deletions lib/code_ownership/private/team_plugins/github.rb
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,22 @@ def github
sig { override.params(teams: T::Array[CodeTeams::Team]).returns(T::Array[String]) }
def self.validation_errors(teams)
all_github_teams = teams.flat_map { |team| self.for(team).github.team }
missing_github_teams = teams.select { |team| self.for(team).github.team.nil? }
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@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.


teams_used_more_than_once = all_github_teams.tally.select do |_team, count|
count > 1
end

errors = T.let([], T::Array[String])

if missing_github_teams.any?
errors << <<~ERROR
The following teams are missing `github.team` entries

#{missing_github_teams.map(&:config_yml).join("\n")}
ERROR
end

if teams_used_more_than_once.any?
errors << <<~ERROR
The following teams are specified multiple times:
Expand Down