Address MR suggestions

CE mirror of c4578b951e331fe8c75cd4f948ce74cec6587bad

app/models/blob_viewer/gitlab_ci_yml.rb

@@ -10,16 +10,16 @@ module BlobViewer
     self.file_types = %i(gitlab_ci)
     self.binary = false
 
-    def validation_message(project, ref)
+    def validation_message(project, sha)
       return @validation_message if defined?(@validation_message)
 
       prepare!
 
-      @validation_message = Gitlab::Ci::YamlProcessor.validation_message(blob.data, { project: project, branch_name: ref })
+      @validation_message = Gitlab::Ci::YamlProcessor.validation_message(blob.data, { project: project, sha: sha })
     end
 
-    def valid?(project, ref)
-      validation_message(project, ref).blank?
+    def valid?(project, sha)
+      validation_message(project, sha).blank?
     end
   end
 end

app/models/ci/pipeline.rb

@@ -475,7 +475,7 @@ module Ci
     end
 
     def initialize_yaml_processor
-      Gitlab::Ci::YamlProcessor.new(ci_yaml_file, { project: project, branch_name: ref })
+      ::Gitlab::Ci::YamlProcessor.new(ci_yaml_file, { project: project, sha: sha })
     end
 
     def ci_yaml_file_path

app/views/projects/blob/viewers/_gitlab_ci_yml.html.haml

-- if viewer.valid?(@project, @ref)
+- if viewer.valid?(@project, @commit.sha)
   = icon('check fw')
   This GitLab CI configuration is valid.
 - else
   = icon('warning fw')
   This GitLab CI configuration is invalid:
-  = viewer.validation_message(@project, @ref)
+  = viewer.validation_message(@project, @commit.sha)
 
 = link_to 'Learn more', help_page_path('ci/yaml/README')

lib/gitlab/ci/config.rb

@@ -76,8 +76,8 @@ module Gitlab
       end
 
       def process_external_files(config, project, opts)
-        branch_name = opts.fetch(:branch_name, project.default_branch)
-        ::Gitlab::Ci::External::Processor.new(config, project, branch_name).perform
+        sha = opts.fetch(:sha, project.repository.commit.sha)
+          ::Gitlab::Ci::External::Processor.new(config, project, sha).perform
       end
     end
   end

lib/gitlab/ci/external/file/local.rb

@@ -3,16 +3,16 @@ module Gitlab
     module External
       module File
         class Local
-          attr_reader :value, :project, :branch_name
+          attr_reader :location, :project, :branch_name
 
-          def initialize(value, project, branch_name)
-            @value = value
-            @project = project
-            @branch_name = branch_name
+          def initialize(location, opts = {})
+            @location = location
+            @project = opts.fetch(:project)
+            @sha = opts.fetch(:sha)
           end
 
           def valid?
-            commit && local_file_content
+            local_file_content
           end
 
           def content
@@ -21,12 +21,8 @@ module Gitlab
 
           private
 
-          def commit
-            @commit ||= project.repository.commit(branch_name)
-          end
-
           def local_file_content
-            @local_file_content ||= project.repository.blob_data_at(commit.sha, value)
+            @local_file_content ||= project.repository.blob_data_at(sha, location)
           end
         end
       end

lib/gitlab/ci/external/file/remote.rb

@@ -3,20 +3,24 @@ module Gitlab
     module External
       module File
         class Remote
-          attr_reader :value
+          attr_reader :location
 
-          def initialize(value)
-            @value = value
+          def initialize(location, opts = {})
+            @location = location
           end
 
           def valid?
-            ::Gitlab::UrlSanitizer.valid?(value) && content
+            ::Gitlab::UrlSanitizer.valid?(location) && content
           end
 
           def content
-            HTTParty.get(value)
-          rescue HTTParty::Error, Timeout::Error
-            false
+            return @content if defined?(@content)
+
+            @content ||= begin
+                           HTTParty.get(location)
+                         rescue HTTParty::Error, Timeout::Error
+                           false
+                         end
           end
         end
       end

lib/gitlab/ci/external/mapper.rb

@@ -2,27 +2,28 @@ module Gitlab
   module Ci
     module External
       class Mapper
-        def initialize(values, project, branch_name)
-          @paths = Array(values.fetch(:include, []))
+        def initialize(values, project, sha)
+          @locations = Array(values.fetch(:include, []))
           @project = project
-          @branch_name = branch_name
+          @sha = sha
         end
 
         def process
-          paths.map { |path| build_external_file(path) }
+          locations.map { |location| build_external_file(location) }
         end
 
         private
 
-        attr_reader :paths, :project, :branch_name
+        attr_reader :locations, :project, :sha
 
-        def build_external_file(path)
-          remote_file = Gitlab::Ci::External::File::Remote.new(path)
+        def build_external_file(location)
+          remote_file = Gitlab::Ci::External::File::Remote.new(location)
 
           if remote_file.valid?
             remote_file
           else
-            ::Gitlab::Ci::External::File::Local.new(path, project, branch_name)
+            options = { project: project, sha: sha }
+            Gitlab::Ci::External::File::Local.new(location, options)
           end
         end
       end

lib/gitlab/ci/external/processor.rb

@@ -4,9 +4,9 @@ module Gitlab
       class Processor
         FileError = Class.new(StandardError)
 
-        def initialize(values, project, branch_name)
+        def initialize(values, project, sha)
           @values = values
-          @external_files = ::Gitlab::Ci::External::Mapper.new(values, project, branch_name).process
+          @external_files = Gitlab::Ci::External::Mapper.new(values, project, sha).process
           @content = {}
         end
 
@@ -18,7 +18,7 @@ module Gitlab
             @content.merge!(content_of(external_file))
           end
 
-          append_external_content
+          append_inline_content
           remove_include_keyword
         end
 
@@ -28,7 +28,7 @@ module Gitlab
 
         def validate_external_file(external_file)
           unless external_file.valid?
-            raise FileError, "External file: '#{external_file.value}' should be a valid local or remote file"
+            raise FileError, "External file: '#{external_file.location}' should be a valid local or remote file"
           end
         end
 
@@ -36,7 +36,7 @@ module Gitlab
           ::Gitlab::Ci::Config::Loader.new(external_file.content).load!
         end
 
-        def append_external_content
+        def append_inline_content
           @content.merge!(@values)
         end
 

spec/lib/gitlab/ci/config_spec.rb

@@ -5,7 +5,7 @@ require_dependency 'active_model'
 describe Gitlab::Ci::Config do
   let(:project) { create(:project, :repository) }
   let(:config) do
-    described_class.new(gitlab_ci_yml, { project: project, branch_name: 'testing' })
+    described_class.new(gitlab_ci_yml, { project: project, sha: '12345' })
   end
 
   context 'when config is valid' do
@@ -149,7 +149,6 @@ describe Gitlab::Ci::Config do
     end
 
     before do
-      allow_any_instance_of(::Gitlab::Ci::External::File::Local).to receive(:commit).and_return('12345')
       allow_any_instance_of(::Gitlab::Ci::External::File::Local).to receive(:local_file_content).and_return(local_file_content)
       allow(HTTParty).to receive(:get).and_return(http_file_content)
     end

spec/lib/gitlab/ci/external/file/local_spec.rb

@@ -2,14 +2,13 @@ require 'fast_spec_helper'
 
 describe Gitlab::Ci::External::File::Local do
   let(:project) { create(:project, :repository) }
-  let(:local_file) { described_class.new(value, project, 'testing') }
+  let(:local_file) { described_class.new(location, { project: project, sha: '12345' }) }
 
   describe "#valid?" do
     context 'when is a valid local path' do
-      let(:value) { '/vendor/gitlab-ci-yml/existent-file.yml' }
+      let(:location) { '/vendor/gitlab-ci-yml/existent-file.yml' }
 
       before do
-        allow_any_instance_of(described_class).to receive(:commit).and_return('12345')
         allow_any_instance_of(described_class).to receive(:local_file_content).and_return("image: 'ruby2:2'")
       end
 
@@ -19,7 +18,7 @@ describe Gitlab::Ci::External::File::Local do
     end
 
     context 'when is not a valid local path' do
-      let(:value) { '/vendor/gitlab-ci-yml/non-existent-file.yml' }
+      let(:location) { '/vendor/gitlab-ci-yml/non-existent-file.yml' }
 
       it 'should return false' do
         expect(local_file.valid?).to be_falsy
@@ -39,7 +38,7 @@ describe Gitlab::Ci::External::File::Local do
       end
 
       context 'with a local file' do
-        let(:value) { '/vendor/gitlab-ci-yml/non-existent-file.yml' }
+        let(:location) { '/vendor/gitlab-ci-yml/non-existent-file.yml' }
 
         before do
           allow_any_instance_of(described_class).to receive(:local_file_content).and_return(local_file_content)

spec/lib/gitlab/ci/external/file/remote_spec.rb

 require 'fast_spec_helper'
 
 describe Gitlab::Ci::External::File::Remote do
-  let(:remote_file) { described_class.new(value) }
-  let(:value) { 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
+  let(:remote_file) { described_class.new(location) }
+  let(:location) { 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
   let(:remote_file_content) do
     <<~HEREDOC
       before_script:
@@ -17,7 +17,7 @@ describe Gitlab::Ci::External::File::Remote do
   describe "#valid?" do
     context 'when is a valid remote url' do
       before do
-        allow(HTTParty).to receive(:get).and_return(remote_file_content)
+        WebMock.stub_request(:get, location).to_return(body: remote_file_content)
       end
 
       it 'should return true' do
@@ -26,7 +26,7 @@ describe Gitlab::Ci::External::File::Remote do
     end
 
     context 'when is not a valid remote url' do
-      let(:value) { 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
+      let(:location) { 'not-valid://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
 
       it 'should return false' do
         expect(remote_file.valid?).to be_falsy
@@ -47,11 +47,11 @@ describe Gitlab::Ci::External::File::Remote do
   describe "#content" do
     context 'with a valid remote file' do
       before do
-        allow(HTTParty).to receive(:get).and_return(remote_file_content)
+        WebMock.stub_request(:get, location).to_return(body: remote_file_content)
       end
 
       it 'should return the content of the file' do
-        expect(remote_file.content).to eq(remote_file_content)
+        expect(remote_file.content).to eql(remote_file_content)
       end
     end
 

spec/lib/gitlab/ci/external/mapper_spec.rb

@@ -9,7 +9,7 @@ describe Gitlab::Ci::External::Mapper do
   end
 
   describe '#process' do
-    subject { described_class.new(values, project, 'testing').process }
+    subject { described_class.new(values, project, '123456').process }
 
     context "when 'include' keyword is defined as string" do
       context 'when the string is a local file' do
@@ -30,15 +30,16 @@ describe Gitlab::Ci::External::Mapper do
       end
 
       context 'when the string is a remote file' do
+        let(:remote_url) { 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
         let(:values) do
           {
-            include: 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml',
+            include: remote_url,
             image: 'ruby:2.2'
           }
         end
 
         before do
-          allow(HTTParty).to receive(:get).and_return(file_content)
+          WebMock.stub_request(:get, remote_url).to_return(body: file_content)
         end
 
         it 'returns an array' do
@@ -52,11 +53,12 @@ describe Gitlab::Ci::External::Mapper do
     end
 
     context "when 'include' is defined as an array" do
+      let(:remote_url) { 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
       let(:values) do
         {
           include:
           [
-            'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml',
+            remote_url,
             '/vendor/gitlab-ci-yml/template.yml'
           ],
           image: 'ruby:2.2'
@@ -64,7 +66,7 @@ describe Gitlab::Ci::External::Mapper do
       end
 
       before do
-        allow(HTTParty).to receive(:get).and_return(file_content)
+        WebMock.stub_request(:get, remote_url).to_return(body: file_content)
       end
 
       it 'returns an array' do

spec/lib/gitlab/ci/external/processor_spec.rb

@@ -2,7 +2,7 @@ require 'fast_spec_helper'
 
 describe Gitlab::Ci::External::Processor do
   let(:project) { create(:project, :repository) }
-  let(:processor) { described_class.new(values, project, 'testing') }
+  let(:processor) { described_class.new(values, project, '12345') }
 
   describe "#perform" do
     context 'when no external files defined' do
@@ -36,7 +36,8 @@ describe Gitlab::Ci::External::Processor do
     end
 
     context 'with a valid remote external file is defined' do
-      let(:values) { { include: 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml', image: 'ruby:2.2' } }
+      let(:remote_url) { 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
+      let(:values) { { include: remote_url, image: 'ruby:2.2' } }
       let(:external_file_content) do
         <<-HEREDOC
         before_script:
@@ -57,7 +58,7 @@ describe Gitlab::Ci::External::Processor do
       end
 
       before do
-        allow(HTTParty).to receive(:get).and_return(external_file_content)
+        WebMock.stub_request(:get, remote_url).to_return(body: external_file_content)
       end
 
       it 'should append the file to the values' do
@@ -84,7 +85,6 @@ describe Gitlab::Ci::External::Processor do
       end
 
       before do
-        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:commit).and_return('12345')
         allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:local_file_content).and_return(local_file_content)
       end
 
@@ -99,10 +99,11 @@ describe Gitlab::Ci::External::Processor do
     end
 
     context 'with multiple external files are defined' do
+      let(:remote_url) { 'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml' }
       let(:external_files) do
         [
           "/spec/ee/fixtures/gitlab/ci/external_files/.gitlab-ci-template-1.yml",
-          'https://gitlab.com/gitlab-org/gitlab-ce/blob/1234/.gitlab-ci-1.yml'
+          remote_url
         ]
       end
       let(:values) do
@@ -123,9 +124,8 @@ describe Gitlab::Ci::External::Processor do
 
       before do
         local_file_content = File.read("#{Rails.root}/spec/ee/fixtures/gitlab/ci/external_files/.gitlab-ci-template-1.yml")
-        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:commit).and_return('12345')
         allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:local_file_content).and_return(local_file_content)
-        allow(HTTParty).to receive(:get).and_return(remote_file_content)
+        WebMock.stub_request(:get, remote_url).to_return(body: remote_file_content)
       end
 
       it 'should append the files to the values' do
@@ -143,7 +143,6 @@ describe Gitlab::Ci::External::Processor do
       let(:local_file_content) { 'invalid content file ////' }
 
       before do
-        allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:commit).and_return('12345')
         allow_any_instance_of(Gitlab::Ci::External::File::Local).to receive(:local_file_content).and_return(local_file_content)
       end