From 7626525d507b7a167673c224980810ea7b3e4fca Mon Sep 17 00:00:00 2001 From: Parker Brown <17183625+parkerbxyz@users.noreply.github.com> Date: Wed, 14 Oct 2026 15:51:31 -0700 Subject: [PATCH] Warn on failed parent team updates and don't count them as changes update_team now logs a rejected (422) update as a warning with the error from GitHub instead of a debug message. sync_team uses the value update_team returns, so a failed parent update is no longer reported as a change. Other successful changes still make sync_team return true. Part of #6 --- .../backend/github_team/service.rb | 5 ++- .../backend/github_team/service_spec.rb | 32 ++++++++++++++++++- 2 files changed, 33 insertions(+), 4 deletions(-) diff --git a/lib/entitlements/backend/github_team/service.rb b/lib/entitlements/backend/github_team/service.rb index dba731d..7885de4 100644 --- a/lib/entitlements/backend/github_team/service.rb +++ b/lib/entitlements/backend/github_team/service.rb @@ -197,9 +197,8 @@ def sync_team(desired_state, current_state) desired_parent_team_id = team_by_name(org_name: org, team_name: desired_metadata["parent_team_name"])[:id] unless desired_parent_team_id.nil? # 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 - update_team(team: current_state, metadata: { parent_team_id: desired_parent_team_id }) + changed_parent_team = update_team(team: current_state, metadata: { parent_team_id: desired_parent_team_id }) end - changed_parent_team = true end end @@ -329,7 +328,7 @@ def update_team(team:, metadata: {}) true rescue Octokit::UnprocessableEntity => e - Entitlements.logger.debug "update_team(team=#{team.team_name}) ERROR - #{e.message}" + Entitlements.logger.warn "update_team(team=#{team.team_name}) ERROR - #{e.message}" false end diff --git a/spec/unit/entitlements/backend/github_team/service_spec.rb b/spec/unit/entitlements/backend/github_team/service_spec.rb index bc430af..08add9b 100644 --- a/spec/unit/entitlements/backend/github_team/service_spec.rb +++ b/spec/unit/entitlements/backend/github_team/service_spec.rb @@ -459,6 +459,36 @@ expect(result).to eq(true) end + it "returns false when the parent team update fails" do + expect(subject).to receive(:team_by_name).with(org_name: "kittensinc", + team_name: "cuddly-kittens").and_return({ id: 10 }) + expect(subject).to receive(:update_team).with(team: team_data_old, + metadata: { parent_team_id: 10 }).and_return(false) + expect(logger).to receive(:debug).with(/sync_team\(russian-blues=1001\): Parent team change found - From No Parent Team to cuddly-kittens/) + expect(logger).to receive(:debug).with(/sync_team\(russian-blues=1001\): Added 0, removed 0/) + result = subject.sync_team(team_metadata_add, team_data_old) + expect(result).to eq(false) + end + + it "returns true when the parent team update fails but members change" do + team_metadata_add_member = Entitlements::Backend::GitHubTeam::Models::Team.new( + team_id: 1001, + team_name: "russian-blues", + members: Set.new(%w[blackmanx highlander ragamuffin MAINECOON]), + ou: "ou=kittensinc,dc=github,dc=com", + metadata: { "parent_team_name" => "cuddly-kittens" } + ) + expect(subject).to receive(:team_by_name).with(org_name: "kittensinc", + team_name: "cuddly-kittens").and_return({ id: 10 }) + expect(subject).to receive(:update_team).with(team: team_data_old, + metadata: { parent_team_id: 10 }).and_return(false) + expect(subject).to receive(:add_user_to_team).with(user: "highlander", team: team_data_old).and_return(true) + expect(logger).to receive(:debug).with(/sync_team\(russian-blues=1001\): Parent team change found - From No Parent Team to cuddly-kittens/) + expect(logger).to receive(:debug).with(/sync_team\(russian-blues=1001\): Added 1, removed 0/) + result = subject.sync_team(team_metadata_add_member, team_data_old) + 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"\]/) @@ -971,7 +1001,7 @@ allow(subject).to receive(:octokit).and_return(octokit) expect(octokit).to receive(:update_team).and_raise(Octokit::UnprocessableEntity) expect(logger).to receive(:debug).with("update_team(team=mainecoon)") - expect(logger).to receive(:debug).with("update_team(team=mainecoon) ERROR - Octokit::UnprocessableEntity") + expect(logger).to receive(:warn).with("update_team(team=mainecoon) ERROR - Octokit::UnprocessableEntity") created = subject.update_team(team: team_data_new) expect(created).to eq(false)