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

Author: parkerbxyz

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
 

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)