Merge pull request #143 from github/github-service-improvements

GrantBirki · web-flow · commit 6472bf3c6ee0 · 2025-04-16T07:50:06.000-07:00
GitHub Service Improvements
diff --git a/Gemfile.lock b/Gemfile.lock
@@ -1,11 +1,12 @@
 PATH
   remote: .
   specs:
-    entitlements-github-plugin (1.1.0)
+    entitlements-github-plugin (1.2.0)
       contracts (~> 0.17.0)
       faraday (~> 2.0)
       faraday-retry (~> 2.0)
       octokit (~> 4.25)
+      retryable (~> 3.0, >= 3.0.5)
 
 GEM
   remote: https://rubygems.org/
@@ -79,6 +80,7 @@ GEM
     rbs (3.6.1)
       logger
     regexp_parser (2.9.2)
+    retryable (3.0.5)
     rexml (3.3.9)
     rspec (3.13.0)
       rspec-core (~> 3.13.0)
diff --git a/entitlements-github-plugin.gemspec b/entitlements-github-plugin.gemspec
@@ -20,6 +20,7 @@ Gem::Specification.new do |s|
   s.add_dependency "faraday", "~> 2.0"
   s.add_dependency "faraday-retry", "~> 2.0"
   s.add_dependency "octokit", "~> 4.25"
+  s.add_dependency "retryable", "~> 3.0", ">= 3.0.5"
 
   s.add_development_dependency "entitlements-app", "~> 1.0"
   s.add_development_dependency "rake", "~> 13.2", ">= 13.2.1"
diff --git a/lib/entitlements/backend/github_org.rb b/lib/entitlements/backend/github_org.rb
@@ -24,3 +24,5 @@ class DuplicateUserError < RuntimeError; end
 require_relative "github_org/controller"
 require_relative "github_org/provider"
 require_relative "github_org/service"
+require_relative "../config/retry"
+Retry.setup!
diff --git a/lib/entitlements/backend/github_org/service.rb b/lib/entitlements/backend/github_org/service.rb
@@ -46,7 +46,9 @@ def add_user_to_organization(user, role)
           Entitlements.logger.debug "#{identifier} add_user_to_organization(user=#{user}, org=#{org}, role=#{role})"
 
           begin
-            new_membership = octokit.update_organization_membership(org, user:, role:)
+            new_membership = Retryable.with_context(:default, not: [Octokit::NotFound]) do
+              octokit.update_organization_membership(org, user:, role:)
+            end
           rescue Octokit::NotFound => e
             raise e unless ignore_not_found
 
@@ -78,7 +80,10 @@ def add_user_to_organization(user, role)
         Contract String => C::Bool
         def remove_user_from_organization(user)
           Entitlements.logger.debug "#{identifier} remove_user_from_organization(user=#{user}, org=#{org})"
-          result = octokit.remove_organization_membership(org, user:)
+
+          result = Retryable.with_context(:default) do
+            octokit.remove_organization_membership(org, user:)
+          end
 
           # If we removed the user, remove them from the cache of members, so that any GitHub team
           # operations in this organization will ignore this user.
diff --git a/lib/entitlements/backend/github_team.rb b/lib/entitlements/backend/github_team.rb
@@ -4,3 +4,5 @@
 require_relative "github_team/models/team"
 require_relative "github_team/provider"
 require_relative "github_team/service"
+require_relative "../config/retry"
+Retry.setup!
diff --git a/lib/entitlements/backend/github_team/service.rb b/lib/entitlements/backend/github_team/service.rb
@@ -279,10 +279,12 @@ def create_team(entitlement_group:)
                 team_options[:parent_team_id] = parent_team_data[:team_id]
               rescue TeamNotFound
                 # if the parent team does not exist, create it (think `mkdir -p` logic here)
-                result = octokit.create_team(
-                  org,
-                  { name: entitlement_metadata["parent_team_name"], repo_names: [], privacy: "closed" }
-                )
+                result = Retryable.with_context(:default, not: [Octokit::UnprocessableEntity]) do
+                  octokit.create_team(
+                    org,
+                    { name: entitlement_metadata["parent_team_name"], repo_names: [], privacy: "closed" }
+                  )
+                end
 
                 Entitlements.logger.debug "created parent team #{entitlement_metadata["parent_team_name"]} with id #{result[:id]}"
 
@@ -296,7 +298,11 @@ def create_team(entitlement_group:)
           end
 
           Entitlements.logger.debug "create_team(team=#{team_name})"
-          result = octokit.create_team(org, team_options)
+
+          result = Retryable.with_context(:default, not: [Octokit::UnprocessableEntity]) do
+            octokit.create_team(org, team_options)
+          end
+
           Entitlements.logger.debug "created team #{team_name} with id #{result[:id]}"
           true
         rescue Octokit::UnprocessableEntity => e
@@ -317,7 +323,10 @@ def update_team(team:, metadata: {})
           Entitlements.logger.debug "update_team(team=#{team.team_name})"
           options = { name: team.team_name, repo_names: [], privacy: "closed",
                       parent_team_id: metadata[:parent_team_id] }
-          octokit.update_team(team.team_id, options)
+          Retryable.with_context(:default, not: [Octokit::UnprocessableEntity]) do
+            octokit.update_team(team.team_id, options)
+          end
+
           true
         rescue Octokit::UnprocessableEntity => e
           Entitlements.logger.debug "update_team(team=#{team.team_name}) ERROR - #{e.message}"
@@ -334,7 +343,9 @@ def update_team(team:, metadata: {})
                    team_name: String
                  ] => Sawyer::Resource
         def team_by_name(org_name:, team_name:)
-          octokit.team_by_name(org_name, team_name)
+          Retryable.with_context(:default) do
+            octokit.team_by_name(org_name, team_name)
+          end
         end
 
         private
@@ -426,7 +437,10 @@ def validate_team_id_and_slug!(team_id, team_slug)
           @validation_cache ||= {}
           @validation_cache[team_id] ||= begin
             Entitlements.logger.debug "validate_team_id_and_slug!(#{team_id}, #{team_slug.inspect})"
-            team_data = octokit.team(team_id)
+            team_data = Retryable.with_context(:default) do
+              octokit.team(team_id)
+            end
+
             team_data[:slug]
           end
           return if @validation_cache[team_id] == team_slug
@@ -457,7 +471,10 @@ def add_user_to_team(user:, team:, role: "member")
           validate_team_id_and_slug!(team.team_id, team.team_name)
 
           begin
-            result = octokit.add_team_membership(team.team_id, user, role:)
+            result = Retryable.with_context(:default, not: [Octokit::UnprocessableEntity, Octokit::NotFound]) do
+              octokit.add_team_membership(team.team_id, user, role:)
+            end
+
             result[:state] == "active" || result[:state] == "pending"
           rescue Octokit::UnprocessableEntity => e
             raise e unless ignore_not_found && e.message =~ /Enterprise Managed Users must be part of the organization to be assigned to the team/
@@ -487,7 +504,10 @@ def remove_user_from_team(user:, team:)
 
           Entitlements.logger.debug "#{identifier} remove_user_from_team(user=#{user}, org=#{org}, team_id=#{team.team_id})"
           validate_team_id_and_slug!(team.team_id, team.team_name)
-          octokit.remove_team_membership(team.team_id, user)
+
+          Retryable.with_context(:default) do
+            octokit.remove_team_membership(team.team_id, user)
+          end
         end
       end
     end
diff --git a/lib/entitlements/config/retry.rb b/lib/entitlements/config/retry.rb
@@ -0,0 +1,21 @@
+# frozen_string_literal: true
+
+require "retryable"
+
+module Retry
+  # This method should be called as early as possible in the startup of your application
+  # It sets up the Retryable gem with custom contexts and passes through a few options
+  # Should the number of retries be reached without success, the last exception will be raised
+  def self.setup!
+    ######## Retryable Configuration ########
+    # All defaults available here:
+    # https://github.com/nfedyashev/retryable/blob/6a04027e61607de559e15e48f281f3ccaa9750e8/lib/retryable/configuration.rb#L22-L33
+    Retryable.configure do |config|
+      config.contexts[:default] = {
+        on: [StandardError],
+        sleep: 1,
+        tries: 3
+      }
+    end
+  end
+end
diff --git a/lib/entitlements/service/github.rb b/lib/entitlements/service/github.rb
@@ -1,5 +1,7 @@
 # frozen_string_literal: true
 
+require_relative "../config/retry"
+
 require "net/http"
 require "octokit"
 require "uri"
@@ -36,6 +38,9 @@ class GitHub
         ignore_not_found: C::Maybe[C::Bool],
       ] => C::Any
       def initialize(addr: nil, org:, token:, ou:, ignore_not_found: false)
+        # init the retry module
+        Retry.setup!
+
         # Save some parameters for the connection but don't actually connect yet.
         @addr = addr
         @org = org
@@ -94,7 +99,10 @@ def org_members
       # Returns true if the github instance is an enterprise server instance
       Contract C::None => C::Bool
       def enterprise?
-        meta = octokit.github_meta
+        meta = Retryable.with_context(:default) do
+          octokit.github_meta
+        end
+
         meta.key? :installed_version
       end
 
@@ -163,6 +171,7 @@ def octokit
           client = Octokit::Client.new(access_token: token)
           client.api_endpoint = addr if addr
           client.auto_paginate = true
+          client.per_page = 100
           Entitlements.logger.debug "Setting up GitHub API connection to #{client.api_endpoint}"
           client
         end
@@ -246,11 +255,22 @@ def members_and_roles_from_graphql
       def members_and_roles_from_rest
         Entitlements.logger.debug "Loading organization members and roles for #{org}"
         result = {}
-        members = octokit.organization_members(org, { role: "admin" })
-        members.each do |member|
+
+        # fetch all the admin members from the org
+        admin_members = Retryable.with_context(:default) do
+          octokit.organization_members(org, { role: "admin" })
+        end
+
+        # fetch all the regular members from the org
+        regular_members = Retryable.with_context(:default) do
+          octokit.organization_members(org, { role: "member" })
+        end
+
+        admin_members.each do |member|
           result[member[:login].downcase] = "ADMIN"
         end
-        octokit.organization_members(org, { role: "member" }).each do |member|
+
+        regular_members.each do |member|
           result[member[:login].downcase] = "MEMBER"
         end
 
diff --git a/lib/version.rb b/lib/version.rb
@@ -2,6 +2,6 @@
 
 module Entitlements
   module Version
-    VERSION = "1.1.0"
+    VERSION = "1.2.0"
   end
 end
diff --git a/spec/unit/entitlements/backend/github_team/service_spec.rb b/spec/unit/entitlements/backend/github_team/service_spec.rb
@@ -845,7 +845,7 @@
     it "does not handle octokit error" do
       exc = StandardError.new("Whoops!")
       allow(subject).to receive(:octokit).and_return(octokit)
-      expect(octokit).to receive(:team).with(1234).and_raise(exc)
+      expect(octokit).to receive(:team).with(1234).and_raise(exc).exactly(3).times
 
       expect do
         subject.send(:validate_team_id_and_slug!, 1234, "my-slug")
diff --git a/spec/unit/entitlements/service/github_spec.rb b/spec/unit/entitlements/service/github_spec.rb
@@ -223,6 +223,54 @@
         expect(result).to eq({"ocicat"=>"MEMBER", "blackmanx"=>"MEMBER", "toyger"=>"MEMBER", "highlander"=>"MEMBER", "russianblue"=>"MEMBER", "ragamuffin"=>"MEMBER", "monalisa"=>"ADMIN", "peterbald"=>"MEMBER", "mainecoon"=>"MEMBER", "laperm"=>"MEMBER"})
       end
     end
+
+    context "when there are no admins" do
+      let(:members) { %w[ocicat blackmanx] }
+      let(:octokit) { instance_double(Octokit::Client) }
+      it "returns only members" do
+        expect(subject).to receive(:octokit).and_return(octokit).twice
+        expect(octokit).to receive(:organization_members).with("kittensinc", { role: "admin" }).and_return([])
+        expect(octokit).to receive(:organization_members).with("kittensinc", { role: "member" }).and_return(members.map { |login| { login: } })
+        result = subject.send(:members_and_roles_from_rest)
+        expect(result).to eq({"ocicat"=>"MEMBER", "blackmanx"=>"MEMBER"})
+      end
+    end
+
+    context "when there are no members" do
+      let(:admins) { %w[monalisa] }
+      let(:octokit) { instance_double(Octokit::Client) }
+      it "returns only admins" do
+        expect(subject).to receive(:octokit).and_return(octokit).twice
+        expect(octokit).to receive(:organization_members).with("kittensinc", { role: "admin" }).and_return(admins.map { |login| { login: } })
+        expect(octokit).to receive(:organization_members).with("kittensinc", { role: "member" }).and_return([])
+        result = subject.send(:members_and_roles_from_rest)
+        expect(result).to eq({"monalisa"=>"ADMIN"})
+      end
+    end
+
+    context "when usernames have different cases" do
+      let(:admins) { ["Monalisa"] }
+      let(:members) { ["OCICAT", "BlackManx"] }
+      let(:octokit) { instance_double(Octokit::Client) }
+      it "downcases all usernames" do
+        expect(subject).to receive(:octokit).and_return(octokit).twice
+        expect(octokit).to receive(:organization_members).with("kittensinc", { role: "admin" }).and_return(admins.map { |login| { login: } })
+        expect(octokit).to receive(:organization_members).with("kittensinc", { role: "member" }).and_return(members.map { |login| { login: } })
+        result = subject.send(:members_and_roles_from_rest)
+        expect(result).to eq({"monalisa"=>"ADMIN", "ocicat"=>"MEMBER", "blackmanx"=>"MEMBER"})
+      end
+    end
+
+    context "when organization is empty" do
+      let(:octokit) { instance_double(Octokit::Client) }
+      it "returns an empty hash" do
+        expect(subject).to receive(:octokit).and_return(octokit).twice
+        expect(octokit).to receive(:organization_members).with("kittensinc", { role: "admin" }).and_return([])
+        expect(octokit).to receive(:organization_members).with("kittensinc", { role: "member" }).and_return([])
+        result = subject.send(:members_and_roles_from_rest)
+        expect(result).to eq({})
+      end
+    end
   end
 
   describe "#pending_members_from_graphql" do
diff --git a/spec/unit/spec_helper.rb b/spec/unit/spec_helper.rb
@@ -184,6 +184,11 @@ def instance_double(klass, *args)
 
   config.before :each do
     allow(Time).to receive(:now).and_return(Time.utc(2018, 4, 1, 12, 0, 0))
+
+    allow(Kernel).to receive(:sleep)
+    allow_any_instance_of(Kernel).to receive(:sleep)
+    allow_any_instance_of(Object).to receive(:sleep)
+
     allow(Entitlements).to receive(:cache).and_return(cache)
     if entitlements_config_hash
       Entitlements.config = entitlements_config_hash
diff --git a/vendor/cache/retryable-3.0.5.gem b/vendor/cache/retryable-3.0.5.gem
