Merge branch 'emilyring-tf-plan-errors' into 'master'

Terraform Plan Widget, Backend Error Messages

See merge request gitlab-org/gitlab!34082

lib/gitlab/ci/parsers/terraform/tfplan.rb

@@ -8,15 +8,21 @@ module Gitlab
           TfplanParserError = Class.new(Gitlab::Ci::Parsers::ParserError)
 
           def parse!(json_data, terraform_reports, artifact:)
+            job_details = job_details(artifact.job)
+            job_id = job_details['job_id']
             plan_data = Gitlab::Json.parse(json_data)
 
-            raise TfplanParserError, 'Tfplan missing required key' unless has_required_keys?(plan_data)
-
-            terraform_reports.add_plan(artifact.job.id.to_s, tfplan(plan_data, artifact.job))
+            if has_required_keys?(plan_data)
+              terraform_reports.add_plan(job_id, valid_tfplan(plan_data, job_details))
+            else
+              terraform_reports.add_plan(job_id, invalid_tfplan(:missing_json_keys, job_details))
+            end
           rescue JSON::ParserError
-            raise TfplanParserError, 'JSON parsing failed'
+            terraform_reports.add_plan(job_id, invalid_tfplan(:invalid_json_format, job_details))
           rescue
-            raise TfplanParserError, 'Tfplan parsing failed'
+            details = job_details || {}
+            plan_name = job_id || 'failed_tf_plan'
+            terraform_reports.add_plan(plan_name, invalid_tfplan(:unknown_error, details))
           end
 
           private
@@ -25,14 +31,24 @@ module Gitlab
             (%w[create update delete] - plan_data.keys).empty?
           end
 
-          def tfplan(plan_data, artifact_job)
+          def job_details(job)
             {
+              'job_id' => job.id.to_s,
+              'job_name' => job.options.dig(:artifacts, :name).to_s,
+              'job_path' => Gitlab::Routing.url_helpers.project_job_path(job.project, job)
+            }
+          end
+
+          def invalid_tfplan(error_type, job_details)
+            job_details.merge('tf_report_error' => error_type)
+          end
+
+          def valid_tfplan(plan_data, job_details)
+            job_details.merge(
               'create' => plan_data['create'].to_i,
               'delete' => plan_data['delete'].to_i,
-              'job_name' => artifact_job.options.dig(:artifacts, :name).to_s,
-              'job_path' => Gitlab::Routing.url_helpers.project_job_path(artifact_job.project, artifact_job),
               'update' => plan_data['update'].to_i
-            }
+            )
           end
         end
       end

spec/lib/gitlab/ci/parsers/terraform/tfplan_spec.rb

@@ -4,37 +4,84 @@ require 'spec_helper'
 
 describe Gitlab::Ci::Parsers::Terraform::Tfplan do
   describe '#parse!' do
-    let_it_be(:artifact) { create(:ci_job_artifact, :terraform) }
+    let(:artifact) { create(:ci_job_artifact, :terraform) }
 
     let(:reports) { Gitlab::Ci::Reports::TerraformReports.new }
 
     context 'when data is invalid' do
-      context 'when there is no data' do
-        it 'raises an error' do
-          plan = '{}'
+      context 'when data is not a JSON file' do
+        it 'reports an invalid_json_format error' do
+          plan = 'Not a JSON file'
+
+          expect { subject.parse!(plan, reports, artifact: artifact) }.not_to raise_error
+
+          reports.plans.each do |key, hash_value|
+            expect(hash_value.keys).to match_array(%w[job_id job_name job_path tf_report_error])
+          end
 
-          expect { subject.parse!(plan, reports, artifact: artifact) }.to raise_error(
-            described_class::TfplanParserError
+          expect(reports.plans).to match(
+            a_hash_including(
+              artifact.job.id.to_s => a_hash_including(
+                'tf_report_error' => :invalid_json_format
+              )
+            )
           )
         end
       end
 
-      context 'when data is not a JSON file' do
-        it 'raises an error' do
-          plan = { 'create' => 0, 'update' => 1, 'delete' => 0 }.to_s
+      context 'when JSON is missing a required key' do
+        it 'reports an invalid_json_keys error' do
+          plan = '{ "wrong_key": 1 }'
+
+          expect { subject.parse!(plan, reports, artifact: artifact) }.not_to raise_error
+
+          reports.plans.each do |key, hash_value|
+            expect(hash_value.keys).to match_array(%w[job_id job_name job_path tf_report_error])
+          end
 
-          expect { subject.parse!(plan, reports, artifact: artifact) }.to raise_error(
-            described_class::TfplanParserError
+          expect(reports.plans).to match(
+            a_hash_including(
+              artifact.job.id.to_s => a_hash_including(
+                'tf_report_error' => :missing_json_keys
+              )
+            )
           )
         end
       end
 
-      context 'when JSON is missing a required key' do
-        it 'raises an error' do
-          plan = '{ "wrong_key": 1 }'
+      context 'when artifact is invalid' do
+        it 'reports an :unknown_error' do
+          expect { subject.parse!('{}', reports, artifact: nil) }.not_to raise_error
+
+          reports.plans.each do |key, hash_value|
+            expect(hash_value.keys).to match_array(%w[tf_report_error])
+          end
+
+          expect(reports.plans).to match(
+            a_hash_including(
+              'failed_tf_plan' => a_hash_including(
+                'tf_report_error' => :unknown_error
+              )
+            )
+          )
+        end
+      end
+
+      context 'when job is invalid' do
+        it 'reports an :unknown_error' do
+          artifact.job_id = nil
+          expect { subject.parse!('{}', reports, artifact: artifact) }.not_to raise_error
 
-          expect { subject.parse!(plan, reports, artifact: artifact) }.to raise_error(
-            described_class::TfplanParserError
+          reports.plans.each do |key, hash_value|
+            expect(hash_value.keys).to match_array(%w[tf_report_error])
+          end
+
+          expect(reports.plans).to match(
+            a_hash_including(
+              'failed_tf_plan' => a_hash_including(
+                'tf_report_error' => :unknown_error
+              )
+            )
           )
         end
       end
@@ -47,7 +94,7 @@ describe Gitlab::Ci::Parsers::Terraform::Tfplan do
         expect { subject.parse!(plan, reports, artifact: artifact) }.not_to raise_error
 
         reports.plans.each do |key, hash_value|
-          expect(hash_value.keys).to match_array(%w[create delete job_name job_path update])
+          expect(hash_value.keys).to match_array(%w[create delete job_id job_name job_path update])
         end
 
         expect(reports.plans).to match(
@@ -68,7 +115,7 @@ describe Gitlab::Ci::Parsers::Terraform::Tfplan do
         expect { subject.parse!(plan, reports, artifact: artifact) }.not_to raise_error
 
         reports.plans.each do |key, hash_value|
-          expect(hash_value.keys).to match_array(%w[create delete job_name job_path update])
+          expect(hash_value.keys).to match_array(%w[create delete job_id job_name job_path update])
         end
 
         expect(reports.plans).to match(

spec/models/ci/build_spec.rb

@@ -4095,6 +4095,10 @@ describe Ci::Build do
         it 'parses blobs and add the results to the terraform report' do
           expect { build.collect_terraform_reports!(terraform_reports) }.not_to raise_error
 
+          terraform_reports.plans.each do |key, hash_value|
+            expect(hash_value.keys).to match_array(%w[create delete job_id job_name job_path update])
+          end
+
           expect(terraform_reports.plans).to match(
             a_hash_including(
               build.id.to_s => a_hash_including(
@@ -4113,9 +4117,19 @@ describe Ci::Build do
           create(:ci_job_artifact, :terraform_with_corrupted_data, job: build, project: build.project)
         end
 
-        it 'raises an error' do
-          expect { build.collect_terraform_reports!(terraform_reports) }.to raise_error(
-            Gitlab::Ci::Parsers::Terraform::Tfplan::TfplanParserError
+        it 'adds invalid plan report' do
+          expect { build.collect_terraform_reports!(terraform_reports) }.not_to raise_error
+
+          terraform_reports.plans.each do |key, hash_value|
+            expect(hash_value.keys).to match_array(%w[job_id job_name job_path tf_report_error])
+          end
+
+          expect(terraform_reports.plans).to match(
+            a_hash_including(
+              build.id.to_s => a_hash_including(
+                'tf_report_error' => :invalid_json_format
+              )
+            )
           )
         end
       end

spec/services/ci/generate_terraform_reports_service_spec.rb

@@ -33,19 +33,36 @@ describe Ci::GenerateTerraformReportsService do
     end
 
     context 'when head pipeline has corrupted terraform reports' do
-      it 'returns status and error message' do
+      it 'returns a report with error messages' do
         build = create(:ci_build, pipeline: merge_request.head_pipeline, project: project)
         create(:ci_job_artifact, :terraform_with_corrupted_data, job: build, project: project)
 
         result = subject.execute(nil, merge_request.head_pipeline)
 
         expect(result).to match(
-          status: :error,
-          status_reason: 'An error occurred while fetching terraform reports.',
+          status: :parsed,
+          data: match(
+            a_hash_including(build.id.to_s => hash_including(
+              'tf_report_error' => :invalid_json_format
+            ))
+          ),
           key: an_instance_of(Array)
         )
       end
     end
+
+    context 'when head pipeline is corrupted' do
+      it 'returns status and error message' do
+        result = subject.execute(nil, nil)
+
+        expect(result).to match(
+          a_hash_including(
+            status: :error,
+            status_reason: 'An error occurred while fetching terraform reports.'
+          )
+        )
+      end
+    end
   end
 
   describe '#latest?' do