diff --git a/lib/entitlements/backend/github_team/service.rb b/lib/entitlements/backend/github_team/service.rb index dba731d..8d639d0 100644 --- a/lib/entitlements/backend/github_team/service.rb +++ b/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( diff --git a/spec/acceptance/github-server/data/team_map.json b/spec/acceptance/github-server/data/team_map.json index 47cf91e..a571be5 100644 --- a/spec/acceptance/github-server/data/team_map.json +++ b/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 } } diff --git a/spec/acceptance/github-server/web.rb b/spec/acceptance/github-server/web.rb index 7124f05..b406e6b 100644 --- a/spec/acceptance/github-server/web.rb +++ b/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) } diff --git a/spec/acceptance/tests/20_modify_and_delete_spec.rb b/spec/acceptance/tests/20_modify_and_delete_spec.rb index 5ffb86c..16f45e8 100644 --- a/spec/acceptance/tests/20_modify_and_delete_spec.rb +++ b/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 diff --git a/spec/unit/entitlements/backend/github_team/provider_spec.rb b/spec/unit/entitlements/backend/github_team/provider_spec.rb index d3df3dc..bfae979 100644 --- a/spec/unit/entitlements/backend/github_team/provider_spec.rb +++ b/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])) } diff --git a/spec/unit/entitlements/backend/github_team/service_spec.rb b/spec/unit/entitlements/backend/github_team/service_spec.rb index bc430af..f391e6b 100644 --- a/spec/unit/entitlements/backend/github_team/service_spec.rb +++ b/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