Fix static analysis and tests related to YAML processing

5 files changed, 103 insertions, 67 deletions

diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb
index acc2dbb59fd..9642c94a231 100644
--- a/app/models/ci/pipeline.rb
+++ b/app/models/ci/pipeline.rb
@@ -363,7 +363,7 @@ module Ci
       return [] unless config_processor
 
       strong_memoize(:stage_seeds) do
-        seeds = config_processor.stages.map do |attributes|
+        seeds = config_processor.stages_attributes.map do |attributes|
           Gitlab::Ci::Pipeline::Seed::Stage.new(self, attributes)
         end
 
diff --git a/lib/gitlab/ci/yaml_processor.rb b/lib/gitlab/ci/yaml_processor.rb
index 6074829e152..bc2a6f98dae 100644
--- a/lib/gitlab/ci/yaml_processor.rb
+++ b/lib/gitlab/ci/yaml_processor.rb
@@ -27,7 +27,7 @@ module Gitlab
       end
 
       def build_attributes(name)
-        job = @jobs[name.to_sym] || {}
+        job = @jobs.fetch(name.to_sym, {})
 
         { stage_idx: @stages.index(job[:stage]),
           stage: job[:stage],
@@ -53,39 +53,23 @@ module Gitlab
           }.compact }
       end
 
-      # REFACTORING, this needs improvement, and specs
-      #
-      def stage_attributes(stage)
-        selected = @jobs.values.select do |job|
-          job[:stage] == stage
-        end
-
-        selected.map do |job|
-          build_attributes(job[:name])
-        end
+      def stage_builds_attributes(stage)
+        @jobs.values
+          .select { |job| job[:stage] == stage }
+          .map { |job| build_attributes(job[:name]) }
       end
 
-      # REFACTORING, needs specs
-      #
-      def seed_attributes(stage)
-        seeds = stage_attributes(stage).map do |attributes|
-          job = @jobs.fetch(attributes[:name].to_sym)
-
-          attributes
-            .merge(only: job.fetch(:only, {}))
-            .merge(except: job.fetch(:except, {}))
-      end
+      def stages_attributes
+        @stages.uniq.map do |stage|
+          seeds = stage_builds_attributes(stage).map do |attributes|
+            job = @jobs.fetch(attributes[:name].to_sym)
 
-        { name: stage,
-          index: @stages.index(stage),
-          builds: seeds }
-      end
+            attributes
+              .merge(only: job.fetch(:only, {}))
+              .merge(except: job.fetch(:except, {}))
+          end
 
-      # REFACTORING, needs specs
-      #
-      def stages
-        @stages.uniq.map do |stage|
-          seed_attributes(stage)
+          { name: stage, index: @stages.index(stage), builds: seeds }
         end
       end
 
diff --git a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb
index 94ead73e875..116573379e0 100644
--- a/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb
+++ b/spec/lib/gitlab/ci/pipeline/seed/build_spec.rb
@@ -89,13 +89,13 @@ describe Gitlab::Ci::Pipeline::Seed::Build do
 
     context 'when branch policy matches' do
       context 'when using only' do
-        let(:attributes) { { name: 'rspec', only: { refs: ['deploy', 'master'] } } }
+        let(:attributes) { { name: 'rspec', only: { refs: %w[deploy master] } } }
 
         it { is_expected.to be_included }
       end
 
       context 'when using except' do
-        let(:attributes) { { name: 'rspec', except: { refs: ['deploy', 'master'] } } }
+        let(:attributes) { { name: 'rspec', except: { refs: %w[deploy master] } } }
 
         it { is_expected.not_to be_included }
       end
@@ -130,12 +130,12 @@ describe Gitlab::Ci::Pipeline::Seed::Build do
     end
 
     context 'when keywords and pipeline source policy matches' do
-      possibilities = [['pushes', 'push'],
-                       ['web', 'web'],
-                       ['triggers', 'trigger'],
-                       ['schedules', 'schedule'],
-                       ['api', 'api'],
-                       ['external', 'external']]
+      possibilities = [%w[pushes push],
+                       %w[web web],
+                       %w[triggers trigger],
+                       %w[schedules schedule],
+                       %w[api api],
+                       %w[external external]]
 
       context 'when using only' do
         possibilities.each do |keyword, source|
@@ -167,12 +167,12 @@ describe Gitlab::Ci::Pipeline::Seed::Build do
     end
 
     context 'when keywords and pipeline source does not match' do
-      possibilities = [['pushes', 'web'],
-                       ['web', 'push'],
-                       ['triggers', 'schedule'],
-                       ['schedules', 'external'],
-                       ['api', 'trigger'],
-                       ['external', 'api']]
+      possibilities = [%w[pushes web],
+                       %w[web push],
+                       %w[triggers schedule],
+                       %w[schedules external],
+                       %w[api trigger],
+                       %w[external api]]
 
       context 'when using only' do
         possibilities.each do |keyword, source|
diff --git a/spec/lib/gitlab/ci/yaml_processor_spec.rb b/spec/lib/gitlab/ci/yaml_processor_spec.rb
index 19dd89f0b6d..ce941197ce1 100644
--- a/spec/lib/gitlab/ci/yaml_processor_spec.rb
+++ b/spec/lib/gitlab/ci/yaml_processor_spec.rb
@@ -133,6 +133,57 @@ module Gitlab
         end
       end
 
+      describe '#stages_attributes' do
+        let(:config) do
+          YAML.dump(
+            rspec: { script: 'rspec', stage: 'test', only: ['branches'] },
+            prod: { script: 'cap prod', stage: 'deploy', only: ['tags'] }
+          )
+        end
+
+        let(:attributes) do
+          [{ name: "build",
+             index: 0,
+             builds: [] },
+           { name: "test",
+             index: 1,
+             builds:
+               [{ stage_idx: 1,
+                  stage: "test",
+                  commands: "rspec",
+                  tag_list: [],
+                  name: "rspec",
+                  allow_failure: false,
+                  when: "on_success",
+                  environment: nil,
+                  coverage_regex: nil,
+                  yaml_variables: [],
+                  options: { script: ["rspec"] },
+                  only: { refs: ["branches"] },
+                  except: {} }] },
+           { name: "deploy",
+             index: 2,
+             builds:
+               [{ stage_idx: 2,
+                  stage: "deploy",
+                  commands: "cap prod",
+                  tag_list: [],
+                  name: "prod",
+                  allow_failure: false,
+                  when: "on_success",
+                  environment: nil,
+                  coverage_regex: nil,
+                  yaml_variables: [],
+                  options: { script: ["cap prod"] },
+                  only: { refs: ["tags"] },
+                  except: {} }] }]
+        end
+
+        it 'returns stages seed attributes' do
+          expect(subject.stages_attributes).to eq attributes
+        end
+      end
+
       describe 'only / except policies validations' do
         context 'when `only` has an invalid value' do
           let(:config) { { rspec: { script: "rspec", type: "test", only: only } } }
@@ -203,7 +254,7 @@ module Gitlab
         let(:config_data) { YAML.dump(config) }
         let(:config_processor) { Gitlab::Ci::YamlProcessor.new(config_data) }
 
-        subject { config_processor.stage_attributes('test').first }
+        subject { config_processor.stage_builds_attributes('test').first }
 
         describe "before_script" do
           context "in global context" do
@@ -286,8 +337,8 @@ module Gitlab
 
             config_processor = Gitlab::Ci::YamlProcessor.new(config)
 
-            expect(config_processor.stage_attributes("test").size).to eq(1)
-            expect(config_processor.stage_attributes("test").first).to eq({
+            expect(config_processor.stage_builds_attributes("test").size).to eq(1)
+            expect(config_processor.stage_builds_attributes("test").first).to eq({
               stage: "test",
               stage_idx: 1,
               name: "rspec",
@@ -321,8 +372,8 @@ module Gitlab
 
             config_processor = Gitlab::Ci::YamlProcessor.new(config)
 
-            expect(config_processor.stage_attributes("test").size).to eq(1)
-            expect(config_processor.stage_attributes("test").first).to eq({
+            expect(config_processor.stage_builds_attributes("test").size).to eq(1)
+            expect(config_processor.stage_builds_attributes("test").first).to eq({
               stage: "test",
               stage_idx: 1,
               name: "rspec",
@@ -354,8 +405,8 @@ module Gitlab
 
             config_processor = Gitlab::Ci::YamlProcessor.new(config)
 
-            expect(config_processor.stage_attributes("test").size).to eq(1)
-            expect(config_processor.stage_attributes("test").first).to eq({
+            expect(config_processor.stage_builds_attributes("test").size).to eq(1)
+            expect(config_processor.stage_builds_attributes("test").first).to eq({
               stage: "test",
               stage_idx: 1,
               name: "rspec",
@@ -383,8 +434,8 @@ module Gitlab
 
             config_processor = Gitlab::Ci::YamlProcessor.new(config)
 
-            expect(config_processor.stage_attributes("test").size).to eq(1)
-            expect(config_processor.stage_attributes("test").first).to eq({
+            expect(config_processor.stage_builds_attributes("test").size).to eq(1)
+            expect(config_processor.stage_builds_attributes("test").first).to eq({
               stage: "test",
               stage_idx: 1,
               name: "rspec",
@@ -529,7 +580,7 @@ module Gitlab
                                })
 
             config_processor = Gitlab::Ci::YamlProcessor.new(config)
-            builds = config_processor.stage_attributes("test")
+            builds = config_processor.stage_builds_attributes("test")
 
             expect(builds.size).to eq(1)
             expect(builds.first[:when]).to eq(when_state)
@@ -561,8 +612,8 @@ module Gitlab
 
           config_processor = Gitlab::Ci::YamlProcessor.new(config)
 
-          expect(config_processor.stage_attributes("test").size).to eq(1)
-          expect(config_processor.stage_attributes("test").first[:options][:cache]).to eq(
+          expect(config_processor.stage_builds_attributes("test").size).to eq(1)
+          expect(config_processor.stage_builds_attributes("test").first[:options][:cache]).to eq(
             paths: ["logs/", "binaries/"],
             untracked: true,
             key: 'key',
@@ -580,8 +631,8 @@ module Gitlab
 
           config_processor = Gitlab::Ci::YamlProcessor.new(config)
 
-          expect(config_processor.stage_attributes("test").size).to eq(1)
-          expect(config_processor.stage_attributes("test").first[:options][:cache]).to eq(
+          expect(config_processor.stage_builds_attributes("test").size).to eq(1)
+          expect(config_processor.stage_builds_attributes("test").first[:options][:cache]).to eq(
             paths: ["logs/", "binaries/"],
             untracked: true,
             key: 'key',
@@ -600,8 +651,8 @@ module Gitlab
 
           config_processor = Gitlab::Ci::YamlProcessor.new(config)
 
-          expect(config_processor.stage_attributes("test").size).to eq(1)
-          expect(config_processor.stage_attributes("test").first[:options][:cache]).to eq(
+          expect(config_processor.stage_builds_attributes("test").size).to eq(1)
+          expect(config_processor.stage_builds_attributes("test").first[:options][:cache]).to eq(
             paths: ["test/"],
             untracked: false,
             key: 'local',
@@ -629,8 +680,8 @@ module Gitlab
 
           config_processor = Gitlab::Ci::YamlProcessor.new(config)
 
-          expect(config_processor.stage_attributes("test").size).to eq(1)
-          expect(config_processor.stage_attributes("test").first).to eq({
+          expect(config_processor.stage_builds_attributes("test").size).to eq(1)
+          expect(config_processor.stage_builds_attributes("test").first).to eq({
             stage: "test",
             stage_idx: 1,
             name: "rspec",
@@ -666,7 +717,7 @@ module Gitlab
                                })
 
             config_processor = Gitlab::Ci::YamlProcessor.new(config)
-            builds = config_processor.stage_attributes("test")
+            builds = config_processor.stage_builds_attributes("test")
 
             expect(builds.size).to eq(1)
             expect(builds.first[:options][:artifacts][:when]).to eq(when_state)
@@ -682,7 +733,7 @@ module Gitlab
         end
 
         let(:processor) { Gitlab::Ci::YamlProcessor.new(YAML.dump(config)) }
-        let(:builds) { processor.stage_attributes('deploy') }
+        let(:builds) { processor.stage_builds_attributes('deploy') }
 
         context 'when a production environment is specified' do
           let(:environment) { 'production' }
@@ -839,7 +890,7 @@ module Gitlab
 
       describe "Hidden jobs" do
         let(:config_processor) { Gitlab::Ci::YamlProcessor.new(config) }
-        subject { config_processor.stage_attributes("test") }
+        subject { config_processor.stage_builds_attributes("test") }
 
         shared_examples 'hidden_job_handling' do
           it "doesn't create jobs that start with dot" do
@@ -887,7 +938,7 @@ module Gitlab
 
       describe "YAML Alias/Anchor" do
         let(:config_processor) { Gitlab::Ci::YamlProcessor.new(config) }
-        subject { config_processor.stage_attributes("build") }
+        subject { config_processor.stage_builds_attributes("build") }
 
         shared_examples 'job_templates_handling' do
           it "is correctly supported for jobs" do
diff --git a/spec/views/ci/lints/show.html.haml_spec.rb b/spec/views/ci/lints/show.html.haml_spec.rb
index 7724d54c569..ded320793ea 100644
--- a/spec/views/ci/lints/show.html.haml_spec.rb
+++ b/spec/views/ci/lints/show.html.haml_spec.rb
@@ -5,6 +5,7 @@ describe 'ci/lints/show' do
 
   describe 'XSS protection' do
     let(:config_processor) { Gitlab::Ci::YamlProcessor.new(YAML.dump(content)) }
+
     before do
       assign(:status, true)
       assign(:builds, config_processor.builds)