Merge pull request zendesk#931 from zendesk/grosser/roles

reuse local fetching code for role file lookups

app/models/build.rb

@@ -57,10 +57,8 @@ def docker_image=(image)
     @docker_image = image
   end
 
-  def file_from_repo(path, ttl: 1.hour)
-    Rails.cache.fetch([self, path], expire_in: ttl) do
-      project.repository.file_content git_sha, path
-    end
+  def file_from_repo(path)
+    project.repository.file_content path, git_sha
   end
 
   def url

app/models/git_repository.rb

@@ -45,14 +45,14 @@ def clone!(from: repository_url, to: repo_cache_dir, mirror: false)
   add_method_tracer :clone!
 
   def commit_from_ref(git_reference, length: 7)
-    ensure_local_cache!
+    return unless ensure_local_cache!
     command = ['git', 'describe', '--long', '--tags', '--all', "--abbrev=#{length || 40}", git_reference]
     return unless output = capture_stdout(*command)
     output.split('-').last.sub(/^g/, '')
   end
 
   def tag_from_ref(git_reference)
-    ensure_local_cache!
+    return unless ensure_local_cache!
     capture_stdout 'git', 'describe', '--tags', git_reference
   end
 
@@ -61,15 +61,15 @@ def repo_cache_dir
   end
 
   def tags
-    ensure_local_cache!
+    return unless ensure_local_cache!
     command = ["git", "for-each-ref", "refs/tags", "--sort=-authordate", "--format=%(refname)", "--count=600"]
     return [] unless output = capture_stdout(*command)
     output.gsub! 'refs/tags/', ''
     output.split("\n")
   end
 
   def branches
-    ensure_local_cache!
+    return unless ensure_local_cache!
     return [] unless output = capture_stdout('git', 'branch', '--list', '--no-column')
     output.delete!('* ')
     output.split("\n")
@@ -92,9 +92,13 @@ def executor
   end
 
   # will update the repo if sha is not found
-  def file_content(sha, file)
-    raise ArgumentError, "Need a sha, but #{sha} (#{sha.size}) given" unless sha =~ Build::SHA1_REGEX
-    (locally_cached? && sha_exist?(sha)) || update_local_cache!
+  def file_content(file, sha)
+    if sha =~ Build::SHA1_REGEX
+      (locally_cached? && sha_exist?(sha)) || update_local_cache!
+    else
+      update_local_cache!
+      return unless sha = commit_from_ref(sha, length: nil)
+    end
     capture_stdout "git", "show", "#{sha}:#{file}"
   end
 
@@ -111,7 +115,7 @@ def sha_exist?(sha)
   end
 
   def ensure_local_cache!
-    update_local_cache! unless locally_cached?
+    locally_cached? || update_local_cache!
   end
 
   def checkout!(git_reference, pwd: repo_cache_dir)

config/application.rb

@@ -25,24 +25,27 @@ class Application < Rails::Application
 
     config.autoload_paths += Dir["#{config.root}/lib/**/"]
 
-    servers = []
-    options = { value_max_bytes: 3000000, compress: true, expires_in: 1.day }
-
-    # support memcachier env used by heroku
-    # https://devcenter.heroku.com/articles/memcachier#rails-3-and-4
-    if ENV["MEMCACHIER_SERVERS"]
-      servers = (ENV["MEMCACHIER_SERVERS"]).split(",")
-      options.merge!(
-        username: ENV["MEMCACHIER_USERNAME"],
-        password: ENV["MEMCACHIER_PASSWORD"],
-        failover: true,
-        socket_timeout: 1.5,
-        socket_failure_delay: 0.2
-      )
+    if Rails.env.test?
+      config.cache_store = :memory_store
+    else
+      servers = []
+      options = { value_max_bytes: 3000000, compress: true, expires_in: 1.day }
+
+      # support memcachier env used by heroku
+      # https://devcenter.heroku.com/articles/memcachier#rails-3-and-4
+      if ENV["MEMCACHIER_SERVERS"]
+        servers = (ENV["MEMCACHIER_SERVERS"]).split(",")
+        options.merge!(
+          username: ENV["MEMCACHIER_USERNAME"],
+          password: ENV["MEMCACHIER_PASSWORD"],
+          failover: true,
+          socket_timeout: 1.5,
+          socket_failure_delay: 0.2
+        )
+      end
+      config.cache_store = :dalli_store, servers, options
     end
 
-    config.cache_store = :dalli_store, servers, options
-
     # Raise exceptions
     config.active_record.raise_in_transactional_callbacks = true
 

plugins/kubernetes/app/decorators/project_decorator.rb

@@ -2,51 +2,40 @@
   has_many :kubernetes_releases, class_name: 'Kubernetes::Release'
   has_many :roles, class_name: 'Kubernetes::Role'
 
-  def file_from_repo(path, git_ref, ttl: 1.hour)
-    # Caching by Git reference
-    Rails.cache.fetch([git_ref, path], expire_in: ttl) do
-      data = GITHUB.contents(github_repo, path: path, ref: git_ref)
-      Base64.decode64(data[:content])
-    end
-  rescue Octokit::NotFound
-    nil
+  def file_from_repo(path, git_ref)
+    repository.file_content path, git_ref
   end
 
-  def directory_contents_from_repo(path, git_ref, ttl: 1.hour)
-    # Caching by Git reference
-    Rails.cache.fetch([git_ref, path], expire_in: ttl) do
-      GITHUB.contents(github_repo, path: path, ref: git_ref).map(&:path).select { |file_name|
-        file_name.ends_with?('.yml', '.yaml', '.json')
-      }
-    end
-  rescue Octokit::NotFound
-    nil
+  def kubernetes_config_files_in_repo(git_ref)
+    path = 'kubernetes'
+    return [] unless files = repository.file_content(path, git_ref)
+    files.split("\n").grep(/\.(yml|yaml|json)$/).map { |f| "#{path}/#{f}" }
   end
 
   # Imports the new kubernetes roles. This operation is atomic: if one role fails to be imported, none
   # of them will be persisted and the soft deletion will be rollbacked.
   def refresh_kubernetes_roles!(git_ref)
-    config_files = directory_contents_from_repo('kubernetes', git_ref)
-
-    unless config_files.to_a.empty?
-      Project.transaction do
-        roles.each(&:soft_delete!)
-
-        kubernetes_config_files(config_files, git_ref) { |config_file|
-          roles.create!(
-            config_file: config_file.file_path,
-            name: config_file.deployment.metadata.labels.role,
-            service_name: config_file.service.metadata.name,
-            ram: config_file.deployment.ram_mi,
-            cpu: config_file.deployment.cpu_m,
-            replicas: config_file.deployment.spec.replicas,
-            deploy_strategy: config_file.deployment.strategy_type)
-        }
-
-        # Need to reload the project to refresh the association otherwise
-        # the soft deleted roles will be rendered by the JSON serializer
-        reload.roles.not_deleted
+    config_files = kubernetes_config_files_in_repo(git_ref)
+    return if config_files.to_a.empty?
+
+    Project.transaction do
+      roles.each(&:soft_delete!)
+
+      kubernetes_config_files(config_files, git_ref) do |config_file|
+        roles.create!(
+          config_file: config_file.file_path,
+          name: config_file.deployment.metadata.labels.role,
+          service_name: config_file.service.metadata.name,
+          ram: config_file.deployment.ram_mi,
+          cpu: config_file.deployment.cpu_m,
+          replicas: config_file.deployment.spec.replicas,
+          deploy_strategy: config_file.deployment.strategy_type
+        )
       end
+
+      # Need to reload the project to refresh the association otherwise
+      # the soft deleted roles will be rendered by the JSON serializer
+      reload.roles.not_deleted
     end
   end
 

plugins/kubernetes/app/models/kubernetes/deploy_executor.rb

@@ -186,7 +186,6 @@ def ensure_build_is_successful(build)
     def create_release(build)
       # build config for every cluster and role we want to deploy to
       group_config = @job.deploy.stage.deploy_groups.map do |group|
-        # raise "#{group.name} needs to be on kubernetes" unless group.
         roles = Kubernetes::Role.where(project_id: @job.project_id).map do |role|
           {id: role.id, replicas: role.replicas} # TODO make replicas configureable
         end

plugins/kubernetes/test/controllers/kubernetes_roles_controller_test.rb

@@ -240,7 +240,7 @@ def assert_errors(response_body)
     let(:contents) { parse_role_config_file('kubernetes_role_config_file') }
 
     before do
-      Project.any_instance.stubs(:directory_contents_from_repo).returns(['some_folder/file_name.yml'])
+      Project.any_instance.stubs(:kubernetes_config_files_in_repo).returns(['some_folder/file_name.yml'])
       Project.any_instance.stubs(:file_from_repo).returns(contents)
     end
 
@@ -283,7 +283,7 @@ def assert_errors(response_body)
 
     as_a_admin do
       before do
-        Project.any_instance.stubs(:directory_contents_from_repo).returns([])
+        Project.any_instance.stubs(:kubernetes_config_files_in_repo).returns([])
       end
 
       it 'responds with a 404 if no config files where found' do

plugins/kubernetes/test/decorators/project_decorator_test.rb

@@ -3,41 +3,57 @@
 SingleCov.covered! uncovered: 3
 
 describe Project do
+  include GitRepoTestHelper
+
   let(:project) { projects(:test) }
   let(:contents) { parse_role_config_file('kubernetes_role_config_file') }
 
   describe '#file_from_repo' do
+    it 'returns the file contents' do
+      create_repo_without_tags
+      project.repository_url = repo_temp_dir
+      project.file_from_repo('foo', 'HEAD').must_equal 'monkey'
+    end
+  end
+
+  describe '#kubernetes_config_files_in_repo' do
     before do
-      Rails.cache.clear
-      Octokit::Client.any_instance.stubs(:contents).returns({ content: Base64.encode64(contents) })
+      create_repo_without_tags
+      project.repository_url = repo_temp_dir
     end
 
-    it 'returns the decode file contents' do
-      file_contents = project.file_from_repo('some_folder/other_file_name.rb', 'some_ref')
-      assert_equal contents, file_contents
+    it 'returns config files' do
+      execute_on_remote_repo <<-SHELL
+        mkdir nope
+        touch nope/config.yml
+        touch kuernetes.yml
+
+        mkdir kubernetes
+        touch kubernetes/config.yml
+        touch kubernetes/config.yaml
+        touch kubernetes/config.json
+        touch kubernetes/config.foo
+        touch kubernetes/config.y2k
+
+        git add .
+        git commit -m "second commit"
+      SHELL
+
+      project.kubernetes_config_files_in_repo('HEAD').must_equal ["kubernetes/config.json", "kubernetes/config.yaml", "kubernetes/config.yml"]
     end
-  end
 
-  describe '#directory_contents_from_repo' do
-    before do
-      Rails.cache.clear
-      Octokit::Client.any_instance.stubs(:contents).returns([
-        OpenStruct.new(name: 'file_name.yml', path: 'some_folder/file_name.yml'),
-        OpenStruct.new(name: 'other_file_name.rb', path: 'some_folder/other_file_name.rb')
-      ])
+    it "returns empty array when nothing was found" do
+      project.kubernetes_config_files_in_repo('HEAD').must_equal []
     end
 
-    it 'returns the relevant files from the API response' do
-      files = project.directory_contents_from_repo('some_folder', 'some_ref')
-      assert_includes files, 'some_folder/file_name.yml'
-      refute_includes files, 'some_folder/other_file_name.rb'
-      assert_equal 1, files.size
+    it "returns empty array on error" do
+      project.kubernetes_config_files_in_repo('DSFFSDJKDFSHDSFHSFDHJ').must_equal []
     end
   end
 
   describe '#refresh_kubernetes_roles' do
     before do
-      Project.any_instance.stubs(:directory_contents_from_repo).returns(['some_folder/file_name.yml'])
+      Project.any_instance.stubs(:kubernetes_config_files_in_repo).returns(['some_folder/file_name.yml'])
       Project.any_instance.stubs(:file_from_repo).returns(contents)
     end
 

test/models/git_repository_test.rb

@@ -125,7 +125,7 @@
       execute_on_remote_repo <<-SHELL
         git checkout test_branch
         echo "blah blah" >> bar.txt
-        git add bar
+        git add bar.txt
         git commit -m "created bar.txt"
         git tag -a annotated_tag -m "This is really worth tagging"
         git checkout master
@@ -241,31 +241,31 @@
     let(:sha) { repository.commit_from_ref('master', length: nil) }
 
     it 'finds content' do
-      repository.file_content(sha, 'foo').must_equal "monkey"
+      repository.file_content('foo', sha).must_equal "monkey"
     end
 
     it 'returns nil when file does not exist' do
-      repository.file_content(sha, 'foox').must_equal nil
+      repository.file_content('foox', sha).must_equal nil
     end
 
     it 'returns nil when sha does not exist' do
-      repository.file_content('a' * 40, 'foox').must_equal nil
+      repository.file_content('foox', 'a' * 40).must_equal nil
     end
 
-    it "does not support non-shas" do
-      assert_raises ArgumentError do
-        repository.file_content('a' * 41, 'foox')
-      end
+    it "always updates for non-shas" do
+      repository.expects(:sha_exist?).never
+      repository.expects(:update!)
+      repository.file_content('foox', 'a' * 41).must_equal nil
     end
 
     it "does not update when sha exists to save time" do
       repository.expects(:update!).never
-      repository.file_content(sha, 'foo').must_equal "monkey"
+      repository.file_content('foo', sha).must_equal "monkey"
     end
 
     it "updates when sha is missing" do
       repository.expects(:update!)
-      repository.file_content('a' * 40, 'foo').must_equal nil
+      repository.file_content('foo', 'a' * 40).must_equal nil
     end
   end
 end

test/models/user_test.rb

@@ -319,8 +319,6 @@
   end
 
   describe "#starred_project?" do
-    before { Rails.cache.clear }
-
     let(:user) { users(:viewer) }
     let(:project) { projects(:test) }
 

test/support/git_repo_test_support.rb

@@ -4,7 +4,9 @@ def repo_temp_dir
   end
 
   def execute_on_remote_repo(cmds)
-    `exec 2> /dev/null; cd #{repo_temp_dir}; #{cmds}`
+    result = `exec 2>&1; set -e; cd #{repo_temp_dir}; #{cmds}`
+    raise "FAIL: #{result}" unless $?.success?
+    result
   end
 
   def create_repo_without_tags