Merge pull request #17 from krukow/specify-team-maintainers

Support: role maintainer in team membership

Author: GrantBirki

lib/entitlements/backend/github_team/service.rb

@@ -4,6 +4,7 @@
 require_relative "../../service/github"
 
 require "base64"
+require "set"
 
 module Entitlements
   class Backend
@@ -196,14 +197,56 @@ def sync_team(desired_state, current_state)
             end
           end
 
-          added_members = desired_state.member_strings.map { |u| u.downcase } - current_state.member_strings.map { |u| u.downcase }
-          removed_members = current_state.member_strings.map { |u| u.downcase } - desired_state.member_strings.map { |u| u.downcase }
+          desired_team_members = Set.new(desired_state.member_strings.map { |u| u.downcase })
+          current_team_members = Set.new(current_state.member_strings.map { |u| u.downcase })
+
+          added_members = desired_team_members - current_team_members
+          removed_members = current_team_members - desired_team_members
 
           added_members.select! { |username| add_user_to_team(user: username, team: current_state) }
           removed_members.select! { |username| remove_user_from_team(user: username, team: current_state) }
 
+          added_maintainers = Set.new
+          removed_maintainers = Set.new
+          unless desired_metadata["team_maintainers"] == current_metadata["team_maintainers"]
+            if desired_metadata["team_maintainers"].nil?
+              # We will not delete ALL maintainers from a team and leave it without maintainers.
+              Entitlements.logger.debug "sync_team(#{current_state.team_name}=#{current_state.team_id}): IGNORING GitHub Team Maintainer DELETE"
+            else
+              desired_maintainers_str = desired_metadata["team_maintainers"] # not nil, we tested that above
+              desired_maintainers = Set.new(desired_maintainers_str.split(",").map { |u| u.strip.downcase })
+              unless desired_maintainers.subset?(desired_team_members)
+                maintainer_not_member = desired_maintainers - desired_team_members
+                Entitlements.logger.warn "sync_team(#{current_state.team_name}=#{current_state.team_id}): Maintainers must be a subset of team members. Desired maintainers: #{maintainer_not_member.to_a} are not members. Ignoring."
+                desired_maintainers = desired_maintainers.intersection(desired_team_members)
+              end
+
+              current_maintainers_str = current_metadata["team_maintainers"]
+              current_maintainers = Set.new(
+                current_maintainers_str.nil? ? [] : current_maintainers_str.split(",").map { |u| u.strip.downcase }
+              )
+              # We ignore any current maintainer who is not a member of the team according to the team spec
+              # This avoids messing with teams who have been manually modified to add a maintainer
+              current_maintainers = current_maintainers.intersection(desired_team_members)
+              added_maintainers = desired_maintainers - current_maintainers
+              removed_maintainers = current_maintainers - desired_maintainers
+              if added_maintainers.empty? && removed_maintainers.empty?
+                Entitlements.logger.debug "sync_team(#{current_state.team_name}=#{current_state.team_id}): Textual change but no semantic change in maintainers. It is remains: #{current_maintainers.to_a}."
+              else
+                Entitlements.logger.debug "sync_team(#{current_state.team_name}=#{current_state.team_id}): Maintainer members change found - From #{current_maintainers.to_a} to #{desired_maintainers.to_a}"
+                added_maintainers.select! { |username| add_user_to_team(user: username, team: current_state, role: "maintainer") }
+
+                ## We only touch previous maintainers who are actually still going to be members of the team
+                removed_maintainers = removed_maintainers.intersection(desired_team_members)
+                ## Downgrade membership to default (role: "member")
+                removed_maintainers.select! { |username| add_user_to_team(user: username, team: current_state, role: "member") }
+              end
+            end
+          end
+
+
           Entitlements.logger.debug "sync_team(#{current_state.team_name}=#{current_state.team_id}): Added #{added_members.count}, removed #{removed_members.count}"
-          added_members.any? || removed_members.any? || changed_parent_team
+          added_members.any? || removed_members.any? || added_maintainers.any? || removed_maintainers.any? || changed_parent_team
         end
 
         # Create a team
@@ -366,17 +409,23 @@ def validate_team_id_and_slug!(team_id, team_slug)
         #
         # user - String with the GitHub username
         # team - Entitlements::Backend::GitHubTeam::Models::Team object for the team.
+        # role - optional (default: "member") String with the role to assign to the user: either "member" or "maintainer"
         #
-        # Returns true if the user was added to the team, false if user was already on team.
+        # Returns true if the user was added to the team or role changed; false if user was already on team with same role
         Contract C::KeywordArgs[
           user: String,
           team: Entitlements::Backend::GitHubTeam::Models::Team,
+          role: C::Optional[String]
         ] => C::Bool
-        def add_user_to_team(user:, team:)
+        def add_user_to_team(user:, team:, role: "member")
           return false unless org_members.include?(user.downcase)
-          Entitlements.logger.debug "#{identifier} add_user_to_team(user=#{user}, org=#{org}, team_id=#{team.team_id})"
+          unless role == "member" || role == "maintainer"
+            # :nocov:
+            raise "add_user_to_team role mismatch: team_id=#{team.team_id} user=#{user} expected role=maintainer/member got=#{role}"
+          end
+          Entitlements.logger.debug "#{identifier} add_user_to_team(user=#{user}, org=#{org}, team_id=#{team.team_id}, role=#{role})"
           validate_team_id_and_slug!(team.team_id, team.team_name)
-          result = octokit.add_team_membership(team.team_id, user)
+          result = octokit.add_team_membership(team.team_id, user, role: role)
           result[:state] == "active" || result[:state] == "pending"
         end
 

spec/unit/entitlements/backend/github_team/service_spec.rb

@@ -340,6 +340,53 @@
       )
     end
 
+    let(:team_metadata_add_maintainer) do
+      Entitlements::Backend::GitHubTeam::Models::Team.new(
+        team_id: 1001,
+        team_name: "russian-blues",
+        members: Set.new(%w[blackmanx ragamuffin MAINECOON]),
+        ou: "ou=kittensinc,dc=github,dc=com",
+        metadata: {
+          "parent_team_name" => "cuddly-kittens",
+          "team_maintainers" => "blackmanx,ragamuffin"
+        }
+      )
+    end
+    let(:team_metadata_maintainer_old) do
+      Entitlements::Backend::GitHubTeam::Models::Team.new(
+        team_id: 1001,
+        team_name: "russian-blues",
+        members: Set.new(%w[blackmanx ragamuffin MAINECOON]),
+        ou: "ou=kittensinc,dc=github,dc=com",
+        metadata: {
+          "parent_team_name" => "cuddly-kittens",
+          "team_maintainers" => "ragamuffin"
+        }
+      )
+    end
+    let(:team_metadata_remove_all_maintainers) do
+      Entitlements::Backend::GitHubTeam::Models::Team.new(
+        team_id: 1001,
+        team_name: "russian-blues",
+        members: Set.new(%w[blackmanx ragamuffin MAINECOON]),
+        ou: "ou=kittensinc,dc=github,dc=com",
+        metadata: {
+          "parent_team_name" => "cuddly-kittens",
+        }
+      )
+    end
+    let(:team_metadata_add_non_member_maintainer) do
+      Entitlements::Backend::GitHubTeam::Models::Team.new(
+        team_id: 1001,
+        team_name: "russian-blues",
+        members: Set.new(%w[blackmanx ragamuffin MAINECOON]),
+        ou: "ou=kittensinc,dc=github,dc=com",
+        metadata: {
+          "parent_team_name" => "cuddly-kittens",
+          "team_maintainers" => "krukow,ragamuffin"
+        }
+      )
+    end
     let(:team_metadata_remove) do
       Entitlements::Backend::GitHubTeam::Models::Team.new(
         team_id: 1001,
@@ -401,6 +448,41 @@
       expect(result).to eq(true)
     end
 
+    it "returns true when there were metadata changes to add maintainer" do
+      allow(subject).to receive(:read_team).with(team_metadata_add_maintainer).and_return(team_metadata_maintainer_old)
+      expect(logger).to receive(:debug).with(/sync_team\(russian-blues=1001\): Maintainer members change found - From \["ragamuffin"\] to \["blackmanx", \"ragamuffin\"\]/)
+      expect(subject).to receive(:add_user_to_team).with(user: "blackmanx", team: team_metadata_maintainer_old, role: "maintainer").and_return(true)
+      expect(logger).to receive(:debug).with(/sync_team\(russian-blues=1001\): Added 0, removed 0/)
+      result = subject.sync_team(team_metadata_add_maintainer, team_metadata_maintainer_old)
+      expect(result).to eq(true)
+    end
+
+    it "returns false when there were metadata changes to remove ALL maintainers" do
+      allow(subject).to receive(:read_team).with(team_metadata_remove_all_maintainers).and_return(team_metadata_maintainer_old)
+      expect(logger).to receive(:debug).with(/sync_team\(russian-blues=1001\): IGNORING GitHub Team Maintainer DELETE/)
+      expect(logger).to receive(:debug).with(/sync_team\(russian-blues=1001\): Added 0, removed 0/)
+      result = subject.sync_team(team_metadata_remove_all_maintainers, team_metadata_maintainer_old)
+      expect(result).to eq(false)
+    end
+
+    it "returns true when there were metadata changes to remove a maintainer" do
+      allow(subject).to receive(:read_team).with(team_metadata_maintainer_old).and_return(team_metadata_add_maintainer)
+      expect(logger).to receive(:debug).with(/sync_team\(russian-blues=1001\): Maintainer members change found - From \["blackmanx", "ragamuffin"\] to \["ragamuffin"\]/)
+      expect(subject).to receive(:add_user_to_team).with(user: "blackmanx", team: team_metadata_maintainer_old, role: "member").and_return(true)
+      expect(logger).to receive(:debug).with(/sync_team\(russian-blues=1001\): Added 0, removed 0/)
+      result = subject.sync_team(team_metadata_maintainer_old, team_metadata_add_maintainer)
+      expect(result).to eq(true)
+    end
+
+    it "returns false when there were metadata changes to add maintainer who is NOT in the team" do
+      allow(subject).to receive(:read_team).with(team_metadata_add_non_member_maintainer).and_return(team_metadata_maintainer_old)
+      expect(logger).to receive(:warn).with(/sync_team\(russian-blues=1001\): Maintainers must be a subset of team members. Desired maintainers: \["krukow"\] are not members. Ignoring./)
+      expect(logger).to receive(:debug).with(/sync_team\(russian-blues=1001\): Textual change but no semantic change in maintainers. It is remains: \["ragamuffin\"\]./)
+      expect(logger).to receive(:debug).with(/sync_team\(russian-blues=1001\): Added 0, removed 0/)
+      result = subject.sync_team(team_metadata_add_non_member_maintainer, team_metadata_maintainer_old)
+      expect(result).to eq(false)
+    end
+
     # TODO: I'm hard-coding a block for deletes, for now. I'm doing that by making sure we dont set the desired parent_team_id to nil for teams where it is already set
     it "returns false while deletes are prevented" do
       allow(subject).to receive(:read_team).with(team_metadata_add).and_return(team_metadata_remove)