Set the parent of an existing team that has none

lib/entitlements/backend/github_team/service.rb

@@ -56,10 +56,19 @@ def read_team(entitlement_group)
               entitlement_metadata = nil
             end
 
-            if (cached_members = Entitlements::Data::Groups::Cached.members(dn))
+            cached_members = Entitlements::Data::Groups::Cached.members(dn)
+            cached_metadata = Entitlements::Data::Groups::Cached.metadata(dn) if cached_members
+
+            # The cache can't tell us the current parent unless it records one. When the entitlement
+            # wants a parent, read the team from the API rather than assume the parent is already set.
+            if cached_members && entitlement_metadata&.dig("parent_team_name") && !cached_metadata&.key?("parent_team_name")
+              Entitlements.logger.debug "Parent team for GitHub team #{identifier}:#{org}/#{team_identifier} is not in the cache"
+              cached_members = nil
+            end
+
+            if cached_members
               Entitlements.logger.debug "Loading GitHub team #{identifier}:#{org}/#{team_identifier} from cache"
 
-              cached_metadata = Entitlements::Data::Groups::Cached.metadata(dn)
               # If both the cached and entitlement metadata are nil, our team metadata is nil
               # If one of the cached or entitlement metadata is nil, we use the other populated metadata hash as-is
               # If both cached and entitlement metadata exist, we combine the two hashes with the cached metadata taking precedence
@@ -91,23 +100,11 @@ def read_team(entitlement_group)
               begin
                 teamdata = graphql_team_data(team_identifier)
                 # The entitlement metadata may have GitHub.com Team metadata which it wants to set, so we must
-                # overwrite that metadata with what we get from the API
-                if teamdata[:parent_team_name].nil?
-                  team_metadata = entitlement_metadata
-                else
-                  parent_team_metadata = {
-                    "parent_team_name" => teamdata[:parent_team_name]
-                  }
-                  if entitlement_metadata.nil?
-                    team_metadata = parent_team_metadata
-                  else
-                    # Always merge the current state metadata (cached or API call) into the entitlement metadata, so that the current state takes precedent
-                    team_metadata = entitlement_metadata.merge(parent_team_metadata)
-                  end
-                end
+                # overwrite that metadata with what we get from the API. This includes a nil parent, so that
+                # a team without a parent doesn't look like it already has the parent the entitlement wants.
+                team_metadata = (entitlement_metadata || {}).merge("parent_team_name" => teamdata[:parent_team_name])
 
                 maintainers = teamdata[:members].select { |u| teamdata[:roles][u] == "maintainer" }
-                team_metadata ||= {}
                 team_metadata = team_metadata.merge({ "team_maintainers" => maintainers.any? ? maintainers.join(",") : nil })
 
                 team = Entitlements::Backend::GitHubTeam::Models::Team.new(

spec/acceptance/github-server/data/team_map.json

@@ -1,6 +1,6 @@
 {
-  "employees": { "id": 4, "parent_team_name": "nil", "parent_team_id": "nil" },
-  "grumpy-cat": { "id": 5, "parent_team_name": "nil", "parent_team_id": "nil" },
-  "colonel-meow": { "id": 6, "parent_team_name": "nil", "parent_team_id": "nil" },
+  "employees": { "id": 4, "parent_team_name": null, "parent_team_id": null },
+  "grumpy-cat": { "id": 5, "parent_team_name": null, "parent_team_id": null },
+  "colonel-meow": { "id": 6, "parent_team_name": null, "parent_team_id": null },
   "child-team": { "id": 7, "parent_team_name": "employees", "parent_team_id": 4 }
 }

spec/acceptance/github-server/web.rb

@@ -72,7 +72,7 @@ def graphql_team_query(query)
 
     if team_map.key?(slug)
       team_id = team_map[slug]["id"]
-      parent_team_name = team_map[slug][:parent_team_name]
+      parent_team_name = team_map[slug]["parent_team_name"]
 
       member_dir = File.join(BASE_DIR, "members", team_map[slug]["id"].to_s)
       members = Dir.glob(File.join(member_dir, "*")).sort.map { |filename| File.basename(filename) }

spec/acceptance/tests/20_modify_and_delete_spec.rb

@@ -47,6 +47,11 @@
     expect(@result.stderr).not_to match(/Please add team_id=\d+ for team/)
   end
 
+  it "changes the parent of an existing GitHub team" do
+    expect(@result.stderr).to match(log("INFO", "CHANGE github_parent_team from employees to grumpy-cat for cn=child-team,ou=meowsister,ou=GitHub,dc=github,dc=fake in meowsister"))
+    expect(@result.stderr).to match(log("DEBUG", Regexp.escape("sync_team(child-team=7): Parent team change found - From employees to grumpy-cat")))
+  end
+
   it "logs appropriate informational messages to STDERR for memberOf" do
     expect(@result.stderr).to match(log("DEBUG", "Calculating memberOf attributes for configured groups"))
     expect(@result.stderr).not_to match(log("INFO", "Person blackmanx attribute change:"))
@@ -89,7 +94,7 @@
     # Hey there! If you are here because this test is failing, please don't blindly update the number.
     # Figure out what change you made that caused this number to increase or decrease, and add log checks
     # for it above.
-    expect(@result.stderr).to match(log("INFO", "Successfully applied 28 change\\(s\\)!"))
+    expect(@result.stderr).to match(log("INFO", "Successfully applied 29 change\\(s\\)!"))
   end
 
   it "has no 'DID NOT APPLY' warnings" do

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

@@ -189,6 +189,101 @@
     end
   end
 
+  describe "parent team changes through #diff and #commit" do
+    let(:team_identifier) { "grumpy-cats" }
+    let(:team_dn) { "cn=#{team_identifier},ou=kittensinc,ou=GitHub,dc=github,dc=fake" }
+    let(:team_members) { Set.new(%w[blackmanx ragamuffin russianblue]) }
+    let(:roles) { team_members.map { |u| [u, "member"] }.to_h }
+    let(:no_changes) { { added: Set.new, removed: Set.new } }
+    let(:add_parent) { { added: Set.new, removed: Set.new, metadata: { parent_team: "add" } } }
+    let(:group) do
+      Entitlements::Models::Group.new(
+        dn: team_dn,
+        description: ":smile_cat:",
+        members: team_members,
+        metadata: { "parent_team_name" => "parent-cats" }
+      )
+    end
+
+    before do
+      allow(subject).to receive(:github).and_return(github)
+    end
+
+    def api_team(parent_team_name)
+      { members: team_members.to_a, team_id: 1001, parent_team_name:, roles: }
+    end
+
+    def expect_parent_update
+      expect(github).to receive(:team_by_name).with(org_name: "kittensinc", team_name: "parent-cats").and_return(id: 2002)
+      expect(github).to receive(:update_team) do |team:, metadata:|
+        expect(team.team_id).to eq(1001)
+        expect(metadata).to eq(parent_team_id: 2002)
+        true
+      end
+    end
+
+    it "sets the parent when the team exists without one and there is no cache" do
+      expect(github).to receive(:graphql_team_data).with(team_identifier).once.and_return(api_team(nil))
+      expect_parent_update
+
+      expect(subject.diff(group)).to eq(add_parent)
+      expect(subject.commit(group)).to eq(true)
+    end
+
+    it "reads the team from the API when the cache doesn't record a parent" do
+      cache[:predictive_state] = { by_dn: { team_dn => { members: team_members, metadata: {} } }, invalid: Set.new }
+      expect(github).to receive(:graphql_team_data).with(team_identifier).once.and_return(api_team(nil))
+      expect_parent_update
+
+      expect(subject.diff(group)).to eq(add_parent)
+      expect(subject.commit(group)).to eq(true)
+    end
+
+    it "refreshes a cached parent that differs and sets the parent from the API state" do
+      cache[:predictive_state] = {
+        by_dn: { team_dn => { members: team_members, metadata: { "parent_team_name" => "old-parent" } } },
+        invalid: Set.new
+      }
+      expect(github).to receive(:graphql_team_data).with(team_identifier).once.and_return(api_team(nil))
+      expect_parent_update
+
+      expect(subject.diff(group)).to eq(add_parent)
+      expect(subject.commit(group)).to eq(true)
+    end
+
+    it "refreshes a cached parent that differs and makes no change when the API has the desired parent" do
+      cache[:predictive_state] = {
+        by_dn: { team_dn => { members: team_members, metadata: { "parent_team_name" => "old-parent" } } },
+        invalid: Set.new
+      }
+      expect(github).to receive(:graphql_team_data).with(team_identifier).once.and_return(api_team("parent-cats"))
+      expect(github).not_to receive(:update_team)
+
+      expect(subject.diff(group)).to eq(no_changes)
+      expect(subject.commit(group)).to eq(false)
+    end
+
+    it "uses the cache when it records the desired parent" do
+      cache[:predictive_state] = {
+        by_dn: { team_dn => { members: team_members, metadata: { "parent_team_name" => "parent-cats" } } },
+        invalid: Set.new
+      }
+      expect(github).not_to receive(:graphql_team_data)
+      expect(github).not_to receive(:update_team)
+
+      expect(subject.diff(group)).to eq(no_changes)
+    end
+
+    it "still ignores removing a parent when no parent is wanted" do
+      group_without_parent = Entitlements::Models::Group.new(dn: team_dn, description: ":smile_cat:", members: team_members)
+      expect(github).to receive(:graphql_team_data).with(team_identifier).once.and_return(api_team("parent-cats"))
+      expect(github).not_to receive(:update_team)
+
+      expect(subject.diff(group_without_parent)).to eq(added: Set.new, removed: Set.new, metadata: { parent_team: "remove" })
+      expect(subject.commit(group_without_parent)).to eq(false)
+    end
+  end
+
   describe "#change_ignored?" do
     let(:group1) { Entitlements::Models::Group.new(dn:, description: ":smile_cat:", members: Set.new([snowshoe])) }
     let(:group2) { Entitlements::Models::Group.new(dn:, description: ":smile_cat:", members: Set.new([russian_blue])) }

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

@@ -223,6 +223,96 @@
       expect(result.metadata.key?("application_owner")).to eq(true)
       expect(result.metadata["application_owner"]).to eq("cheetoh")
     end
+
+    context "with a parent team in the entitlement" do
+      let(:entitlement_group_wants_parent) do
+        Entitlements::Models::Group.new(
+          dn: team_dn,
+          description: ":smile_cat:",
+          members: Set.new(%w[russian-blue snowshoe tabby siamese housecat tiger]),
+          metadata: { "application_owner" => "russian_blue", "parent_team_name" => "parent-cats" }
+        )
+      end
+      let(:people) { Set.new(%w[blackmanx ragamuffin russianblue]) }
+      let(:api_team) { { members: people.to_a, team_id: 1001, parent_team_name: nil, roles: {} } }
+
+      def cache_with_metadata(metadata)
+        Entitlements.cache[:predictive_state] =
+          { by_ou: {}, by_dn: { team_dn => { members: people, metadata: } }, invalid: Set.new }
+      end
+
+      it "uses the parent from the API, even when the team has none" do
+        expect(subject).to receive(:graphql_team_data).with(team_identifier).and_return(api_team)
+
+        result = subject.read_team(entitlement_group_wants_parent)
+        expect(result.team_id).to eq(1001)
+        expect(result.metadata).to eq(
+          "application_owner" => "russian_blue",
+          "parent_team_name" => nil,
+          "team_maintainers" => nil
+        )
+      end
+
+      it "reads from the API when the cached metadata is nil" do
+        cache_with_metadata(nil)
+        expect(subject).to receive(:graphql_team_data).with(team_identifier).and_return(api_team)
+
+        result = subject.read_team(entitlement_group_wants_parent)
+        expect(result.team_id).to eq(1001)
+        expect(result.metadata["parent_team_name"]).to eq(nil)
+        expect(subject.from_predictive_cache?(entitlement_group_wants_parent)).to eq(false)
+      end
+
+      it "reads from the API when the cache doesn't record a parent" do
+        cache_with_metadata("application_owner" => "cheetoh")
+        allow(logger).to receive(:debug)
+        expect(logger).to receive(:debug).with("Parent team for GitHub team github.fake:kittensinc/cuddly-kittens is not in the cache")
+        expect(subject).to receive(:graphql_team_data).with(team_identifier).and_return(api_team)
+
+        result = subject.read_team(entitlement_group_wants_parent)
+        expect(result.team_id).to eq(1001)
+        expect(result.metadata["parent_team_name"]).to eq(nil)
+        expect(subject.from_predictive_cache?(entitlement_group_wants_parent)).to eq(false)
+      end
+
+      it "uses the cache when it records no parent" do
+        cache_with_metadata("parent_team_name" => nil)
+        expect(subject).not_to receive(:graphql_team_data)
+
+        result = subject.read_team(entitlement_group_wants_parent)
+        expect(result.team_id).to eq(-1)
+        expect(result.metadata["parent_team_name"]).to eq(nil)
+        expect(result.metadata["application_owner"]).to eq("russian_blue")
+      end
+
+      it "uses the cache when it records a parent" do
+        cache_with_metadata("parent_team_name" => "parent-cats")
+        expect(subject).not_to receive(:graphql_team_data)
+
+        result = subject.read_team(entitlement_group_wants_parent)
+        expect(result.team_id).to eq(-1)
+        expect(result.metadata["parent_team_name"]).to eq("parent-cats")
+      end
+    end
+
+    it "doesn't add a parent from the API when there is no entitlement metadata" do
+      expect(subject).to receive(:graphql_team_data).with(team_identifier)
+        .and_return(members: [], team_id: 1001, parent_team_name: nil, roles: {})
+
+      result = subject.read_team(entitlement_group_exists_no_metadata)
+      expect(result.metadata).to eq("parent_team_name" => nil, "team_maintainers" => nil)
+    end
+
+    it "uses the cache without a parent when the entitlement doesn't want one" do
+      people = Set.new(%w[blackmanx ragamuffin russianblue])
+      Entitlements.cache[:predictive_state] =
+        { by_ou: {}, by_dn: { team_dn => { members: people, metadata: {} } }, invalid: Set.new }
+      expect(subject).not_to receive(:graphql_team_data)
+
+      result = subject.read_team(entitlement_group_exists)
+      expect(result.team_id).to eq(-1)
+      expect(result.metadata["parent_team_name"]).to eq(nil)
+    end
   end
 
   describe "#from_predictive_cache?" do