Merge pull request #23 from chrisgavin/fix-maintainer-sync

chrisgavin · web-flow · commit a8fab7e4987c · 2023-09-02T09:51:08.000+01:00
Fix syncing team maintainers.
diff --git a/lib/entitlements/backend/github_team/provider.rb b/lib/entitlements/backend/github_team/provider.rb
@@ -198,6 +198,23 @@ def diff_existing_updated_metadata(existing_group, group, base_diff)
               Entitlements.logger.info "CHANGE github_parent_team from #{existing_parent_team} to #{changed_parent_team} for #{existing_group.dn} in #{github.org}"
             end
           end
+
+          existing_maintainers = existing_group.metadata_fetch_if_exists("team_maintainers")
+          changed_maintainers = group.metadata_fetch_if_exists("team_maintainers")
+          if existing_maintainers != changed_maintainers
+            base_diff[:metadata] ||= {}
+            if existing_maintainers.nil? && !changed_maintainers.nil?
+              base_diff[:metadata][:team_maintainers] = "add"
+              Entitlements.logger.info "ADD github_team_maintainers #{changed_maintainers} to #{existing_group.dn} in #{github.org}"
+            elsif !existing_maintainers.nil? && changed_maintainers.nil?
+              base_diff[:metadata][:team_maintainers] = "remove"
+              Entitlements.logger.info "REMOVE (NOOP) github_team_maintainers #{existing_maintainers} from #{existing_group.dn} in #{github.org}"
+            else
+              base_diff[:metadata][:team_maintainers] = "change"
+              Entitlements.logger.info "CHANGE github_team_maintainers from #{existing_maintainers} to #{changed_maintainers} for #{existing_group.dn} in #{github.org}"
+            end
+          end
+
           base_diff
         end
 
diff --git a/lib/entitlements/backend/github_team/service.rb b/lib/entitlements/backend/github_team/service.rb
@@ -104,6 +104,10 @@ def read_team(entitlement_group)
                   end
                 end
 
+                maintainers = teamdata[:members].select { |u| teamdata[:roles][u] == "maintainer" }
+                team_metadata = team_metadata || {}
+                team_metadata = team_metadata.merge({"team_maintainers" => maintainers.any? ? maintainers.join(",") : nil})
+
                 team = Entitlements::Backend::GitHubTeam::Models::Team.new(
                   team_id: teamdata[:team_id],
                   team_name: team_identifier,
@@ -326,11 +330,12 @@ def team_by_name(org_name:, team_name:)
         # team_slug - Identifier of the team to retrieve.
         #
         # Returns a data structure with team data.
-        Contract String => { members: C::ArrayOf[String], team_id: Integer, parent_team_name: C::Or[String, nil] }
+        Contract String => { members: C::ArrayOf[String], team_id: Integer, parent_team_name: C::Or[String, nil], roles: C::HashOf[String => String] }
         def graphql_team_data(team_slug)
           cursor = nil
           team_id = nil
           result = []
+          roles = {}
           sanity_counter = 0
 
           while sanity_counter < 100
@@ -348,6 +353,7 @@ def graphql_team_data(team_slug)
                       node {
                         login
                       }
+                      role
                       cursor
                     }
                   }
@@ -375,12 +381,17 @@ def graphql_team_data(team_slug)
             buffer = edges.map { |e| e.fetch("node").fetch("login").downcase }
             result.concat buffer
 
+            edges.each do |e|
+              role = e.fetch("role").downcase
+              roles[e.fetch("node").fetch("login").downcase] = role
+            end
+
             cursor = edges.last.fetch("cursor")
             next if cursor && buffer.size == max_graphql_results
             break
           end
 
-          { members: result, team_id:, parent_team_name: }
+          { members: result, team_id:, parent_team_name:, roles: }
         end
 
         # Ensure that the given team ID actually matches up to the team slug on GitHub. This is in place
diff --git a/spec/acceptance/github-server/web.rb b/spec/acceptance/github-server/web.rb
@@ -73,7 +73,7 @@ def graphql_team_query(query)
       cursor_flag = cursor.nil?
       members.each do |m|
         next if !cursor_flag && Base64.strict_encode64(m) != cursor
-        edges << { "node" => { "login" => m }, "cursor" => Base64.strict_encode64(m) } if cursor_flag
+        edges << { "node" => { "login" => m }, "role" => "MEMBER", "cursor" => Base64.strict_encode64(m) } if cursor_flag
         cursor_flag = true
         break if edges.size >= first
       end
diff --git a/spec/acceptance/tests/01_basic_webserver_github_connectivity_spec.rb b/spec/acceptance/tests/01_basic_webserver_github_connectivity_spec.rb
@@ -51,6 +51,7 @@
                 node {
                   login
                 }
+                role
                 cursor
               }
             },
@@ -78,13 +79,13 @@
                 "databaseId" => 6,
                 "members" => {
                   "edges" => [
-                    { "node" => { "login" => "cheetoh" }, "cursor" => "Y2hlZXRvaA==" },
-                    { "node" => { "login" => "khaomanee" }, "cursor" => "a2hhb21hbmVl" },
-                    { "node" => { "login" => "nebelung" }, "cursor" => "bmViZWx1bmc=" },
-                    { "node" => { "login" => "ojosazules" }, "cursor" => "b2pvc2F6dWxlcw==" }
+                    { "node" => { "login" => "cheetoh" }, "role" => "MEMBER", "cursor" => "Y2hlZXRvaA==" },
+                    { "node" => { "login" => "khaomanee" }, "role" => "MEMBER", "cursor" => "a2hhb21hbmVl" },
+                    { "node" => { "login" => "nebelung" }, "role" => "MEMBER", "cursor" => "bmViZWx1bmc=" },
+                    { "node" => { "login" => "ojosazules" }, "role" => "MEMBER", "cursor" => "b2pvc2F6dWxlcw==" }
                   ]
                 },
-                "parentTeam" => { "slug" => nil }
+                "parentTeam" => { "slug" => nil },
               }
             }
           }
diff --git a/spec/unit/entitlements/backend/github_team/provider_spec.rb b/spec/unit/entitlements/backend/github_team/provider_spec.rb
@@ -140,7 +140,7 @@
         expect(logger).to receive(:debug).with("Loaded cn=grumpy-cats,ou=kittensinc,ou=GitHub,dc=github,dc=fake (id=1001) with 3 member(s)")
 
         allow(subject).to receive(:github).and_return(github)
-        expect(github).to receive(:graphql_team_data).with(team_identifier).and_return(members: old_members, team_id: 1001, parent_team_name: nil)
+        expect(github).to receive(:graphql_team_data).with(team_identifier).and_return(members: old_members, team_id: 1001, parent_team_name: nil, roles: Hash[*old_members.collect { |u| [u, "member"] }.flatten])
 
         expect(subject.diff(group)).to eq(empty_result)
       end
@@ -153,7 +153,7 @@
         cache[:predictive_state] = { by_dn: { team_dn => { members: old_members, metadata: nil } }, invalid: Set.new }
 
         allow(subject).to receive(:github).and_return(github)
-        expect(github).to receive(:graphql_team_data).with(team_identifier).and_return(members: old_members, team_id: 1001, parent_team_name: nil)
+        expect(github).to receive(:graphql_team_data).with(team_identifier).and_return(members: old_members, team_id: 1001, parent_team_name: nil, roles: Hash[*old_members.collect { |u| [u, "member"] }.flatten])
 
         expect(subject.diff(group)).to eq(added: Set.new(%w[mainecoon]), removed: Set.new(%w[russianblue]))
       end
@@ -360,6 +360,75 @@
         metadata: { parent_team: "remove" }
       )
     end
+
+    it "diffs team maintainers change" do
+      entitlements_group = Entitlements::Models::Group.new(
+        dn: "cn=diff-cats,ou=Github,dc=github,dc=fake",
+        members: Set.new(%w[cuddles fluffy morris WHISKERS].map { |u| "uid=#{u},ou=People,dc=kittens,dc=net" }),
+        metadata: { "team_maintainers" => "cuddles" }
+      )
+
+      github_team = Entitlements::Backend::GitHubTeam::Models::Team.new(
+        team_id: 2222,
+        team_name: "diff-cats",
+        members: Set.new(%w[cuddles fluffy morris WHISKERS].map { |u| "uid=#{u},ou=People,dc=kittens,dc=net" }),
+        ou: "ou=kittensinc,ou=GitHub,dc=github,dc=fake",
+        metadata: { "team_maintainers" => "cuddles,fluffy" }
+      )
+
+      result = subject.diff_existing_updated(entitlements_group, github_team)
+      expect(result).to eq(
+        added: Set.new,
+        removed: Set.new,
+        metadata: { team_maintainers: "change" }
+      )
+    end
+
+    it "diffs team maintainers add" do
+      entitlements_group = Entitlements::Models::Group.new(
+        dn: "cn=diff-cats,ou=Github,dc=github,dc=fake",
+        members: Set.new(%w[cuddles fluffy morris WHISKERS].map { |u| "uid=#{u},ou=People,dc=kittens,dc=net" }),
+        metadata: { }
+      )
+
+      github_team = Entitlements::Backend::GitHubTeam::Models::Team.new(
+        team_id: 2222,
+        team_name: "diff-cats",
+        members: Set.new(%w[cuddles fluffy morris WHISKERS].map { |u| "uid=#{u},ou=People,dc=kittens,dc=net" }),
+        ou: "ou=kittensinc,ou=GitHub,dc=github,dc=fake",
+        metadata: { "team_maintainers" => "cuddles,fluffy" }
+      )
+
+      result = subject.diff_existing_updated(entitlements_group, github_team)
+      expect(result).to eq(
+        added: Set.new,
+        removed: Set.new,
+        metadata: { team_maintainers: "add" }
+      )
+    end
+
+    it "diffs team maintainers removal" do
+      entitlements_group = Entitlements::Models::Group.new(
+        dn: "cn=diff-cats,ou=Github,dc=github,dc=fake",
+        members: Set.new(%w[cuddles fluffy morris WHISKERS].map { |u| "uid=#{u},ou=People,dc=kittens,dc=net" }),
+        metadata: { "team_maintainers" => "cuddles,fluffy" }
+      )
+
+      github_team = Entitlements::Backend::GitHubTeam::Models::Team.new(
+        team_id: 2222,
+        team_name: "diff-cats",
+        members: Set.new(%w[cuddles fluffy morris WHISKERS].map { |u| "uid=#{u},ou=People,dc=kittens,dc=net" }),
+        ou: "ou=kittensinc,ou=GitHub,dc=github,dc=fake",
+        metadata: { }
+      )
+
+      result = subject.diff_existing_updated(entitlements_group, github_team)
+      expect(result).to eq(
+        added: Set.new,
+        removed: Set.new,
+        metadata: { team_maintainers: "remove" }
+      )
+    end
   end
 
   describe "#create_github_team_group" do
diff --git a/spec/unit/entitlements/backend/github_team/service_spec.rb b/spec/unit/entitlements/backend/github_team/service_spec.rb
diff --git a/spec/unit/spec_helper.rb b/spec/unit/spec_helper.rb

Original file line number	Diff line number	Diff line change
`@@ -51,6 +51,7 @@`
`51`	`51`	`node {`
`52`	`52`	`login`
`53`	`53`	`}`
	`54`	`+ role`
`54`	`55`	`cursor`
`55`	`56`	`}`
`56`	`57`	`},`
`@@ -78,13 +79,13 @@`
`78`	`79`	`"databaseId" => 6,`
`79`	`80`	`"members" => {`
`80`	`81`	`"edges" => [`
`81`		`- { "node" => { "login" => "cheetoh" }, "cursor" => "Y2hlZXRvaA==" },`
`82`		`- { "node" => { "login" => "khaomanee" }, "cursor" => "a2hhb21hbmVl" },`
`83`		`- { "node" => { "login" => "nebelung" }, "cursor" => "bmViZWx1bmc=" },`
`84`		`- { "node" => { "login" => "ojosazules" }, "cursor" => "b2pvc2F6dWxlcw==" }`
	`82`	`+ { "node" => { "login" => "cheetoh" }, "role" => "MEMBER", "cursor" => "Y2hlZXRvaA==" },`
	`83`	`+ { "node" => { "login" => "khaomanee" }, "role" => "MEMBER", "cursor" => "a2hhb21hbmVl" },`
	`84`	`+ { "node" => { "login" => "nebelung" }, "role" => "MEMBER", "cursor" => "bmViZWx1bmc=" },`
	`85`	`+ { "node" => { "login" => "ojosazules" }, "role" => "MEMBER", "cursor" => "b2pvc2F6dWxlcw==" }`
`85`	`86`	`]`
`86`	`87`	`},`
`87`		`- "parentTeam" => { "slug" => nil }`
	`88`	`+ "parentTeam" => { "slug" => nil },`
`88`	`89`	`}`
`89`	`90`	`}`
`90`	`91`	`}`