Use Stepable in PodLogsService

- This will simplify the service and make it easier to modify in the
future.

app/models/concerns/stepable.rb

@@ -11,15 +11,15 @@ module Stepable
     initial_result = {}
 
     steps.inject(initial_result) do |previous_result, callback|
-      result = method(callback).call
+      result = method(callback).call(previous_result)
 
-      if result[:status] == :error
-        result[:failed_step] = callback
+      if result[:status] != :success
+        result[:last_step] = callback
 
         break result
       end
 
-      previous_result.merge(result)
+      result
     end
   end
 

ee/app/controllers/ee/projects/environments_controller.rb

@@ -20,7 +20,7 @@ module EE
 
             result = PodLogsService.new(environment, params: params.permit!).execute
 
-            if result.nil?
+            if result[:status] == :processing
               head :accepted
             elsif result[:status] == :success
               render json: {

ee/app/services/pod_logs_service.rb

 # frozen_string_literal: true
 
 class PodLogsService < ::BaseService
+  include Stepable
+
   attr_reader :environment
 
   K8S_NAME_MAX_LENGTH = 253
 
   PARAMS = %w(pod_name container_name).freeze
 
+  SUCCESS_RETURN_KEYS = [:status, :logs].freeze
+
+  steps :check_param_lengths,
+    :check_deployment_platform,
+    :check_pod_name,
+    :pod_logs,
+    :split_logs,
+    :filter_return_keys
+
   def initialize(environment, params: {})
     @environment = environment
     @params = filter_params(params.dup).to_hash
   end
 
   def execute
+    execute_steps
+  end
+
+  private
+
+  def check_param_lengths(_result)
     pod_name = params['pod_name'].presence
     container_name = params['container_name'].presence
 
@@ -24,34 +41,54 @@ class PodLogsService < ::BaseService
         ' %{max_length} chars' % { max_length: K8S_NAME_MAX_LENGTH }))
     end
 
+    success(pod_name: pod_name, container_name: container_name)
+  end
+
+  def check_deployment_platform(result)
     unless environment.deployment_platform
-      return error('No deployment platform')
+      return error(_('No deployment platform available'))
     end
 
+    success(result)
+  end
+
+  def check_pod_name(result)
     # If pod_name is not received as parameter, get the pod logs of the first
     # pod of this environment.
-    pod_name ||= environment.pod_names&.first
+    result[:pod_name] ||= environment.pod_names&.first
 
-    pod_logs(pod_name, container_name)
+    unless result[:pod_name]
+      return error(_('No pods available'))
     end
 
-  private
+    success(result)
+  end
 
-  def pod_logs(pod_name, container_name)
-    result = environment.deployment_platform.read_pod_logs(
-      pod_name,
+  def pod_logs(result)
+    response = environment.deployment_platform.read_pod_logs(
+      result[:pod_name],
       namespace,
-      container: container_name
+      container: result[:container_name]
     )
 
-    return unless result
+    return { status: :processing } unless response
+
+    result[:logs] = response[:logs]
 
-    if result[:status] == :error
-      error(result[:error])
+    if response[:status] == :error
+      error(response[:error])
     else
+      success(result)
+    end
+  end
+
+  def split_logs(result)
     logs = split_by_newline(result[:logs])
     success(logs: logs)
   end
+
+  def filter_return_keys(result)
+    result.slice(*SUCCESS_RETURN_KEYS)
   end
 
   def filter_params(params)

ee/spec/controllers/projects/environments_controller_spec.rb

@@ -170,8 +170,8 @@ describe Projects::EnvironmentsController do
         it_behaves_like 'resource not found', 'Pod not found'
       end
 
-      context 'when service returns nil' do
-        let(:service_result) { nil }
+      context 'when service returns status processing' do
+        let(:service_result) { { status: :processing } }
 
         it 'renders accepted' do
           get :logs, params: environment_params(pod_name: pod_name, format: :json)

ee/spec/services/pod_logs_service_spec.rb

@@ -79,7 +79,7 @@ describe PodLogsService do
     end
 
     context 'without deployment platform' do
-      it_behaves_like 'error', 'No deployment platform'
+      it_behaves_like 'error', 'No deployment platform available'
     end
 
     context 'with deployment platform' do
@@ -137,6 +137,18 @@ describe PodLogsService do
 
           subject.execute
         end
+
+        context 'when there are no pods' do
+          before do
+            allow_any_instance_of(Gitlab::Kubernetes::RolloutStatus).to receive(:instances)
+              .and_return([])
+          end
+
+          it 'returns error' do
+            expect(result[:status]).to eq(:error)
+            expect(result[:message]).to eq('No pods available')
+          end
+        end
       end
 
       context 'when error is returned' do
@@ -152,8 +164,8 @@ describe PodLogsService do
             .and_return(nil)
         end
 
-        it 'returns nil' do
-          expect(result).to eq(nil)
+        it 'returns processing' do
+          expect(result).to eq(status: :processing, last_step: :pod_logs)
         end
       end
     end

lib/gitlab/database_importers/self_monitoring/project/create_service.rb

@@ -33,7 +33,7 @@ module Gitlab
 
             if result[:status] == :success
               result
-            elsif STEPS_ALLOWED_TO_FAIL.include?(result[:failed_step])
+            elsif STEPS_ALLOWED_TO_FAIL.include?(result[:last_step])
               success
             else
               raise StandardError, result[:message]
@@ -42,121 +42,124 @@ module Gitlab
 
           private
 
-          def validate_application_settings
+          def validate_application_settings(_result)
             return success if application_settings
 
             log_error('No application_settings found')
             error(_('No application_settings found'))
           end
 
-          def validate_project_created
-            return success unless project_created?
+          def validate_project_created(result)
+            return success(result) unless project_created?
 
             log_error('Project already created')
             error(_('Project already created'))
           end
 
-          def validate_admins
+          def validate_admins(result)
             unless instance_admins.any?
               log_error('No active admin user found')
               return error(_('No active admin user found'))
             end
 
-            success
+            success(result)
           end
 
-          def create_group
+          def create_group(result)
             if project_created?
               log_info(_('Instance administrators group already exists'))
-              @group = application_settings.instance_administration_project.owner
-              return success(group: @group)
+              result[:group] = application_settings.instance_administration_project.owner
+              return success(result)
             end
 
-            @group = ::Groups::CreateService.new(group_owner, create_group_params).execute
+            result[:group] = ::Groups::CreateService.new(group_owner, create_group_params).execute
 
-            if @group.persisted?
-              success(group: @group)
+            if result[:group].persisted?
+              success(result)
             else
               error(_('Could not create group'))
             end
           end
 
-          def create_project
+          def create_project(result)
             if project_created?
               log_info('Instance administration project already exists')
-              @project = application_settings.instance_administration_project
-              return success(project: project)
+              result[:project] = application_settings.instance_administration_project
+              return success(result)
             end
 
-            @project = ::Projects::CreateService.new(group_owner, create_project_params).execute
+            result[:project] = ::Projects::CreateService.new(group_owner, create_project_params(result[:group])).execute
 
-            if project.persisted?
-              success(project: project)
+            if result[:project].persisted?
+              success(result)
             else
-              log_error("Could not create instance administration project. Errors: %{errors}" % { errors: project.errors.full_messages })
+              log_error("Could not create instance administration project. Errors: %{errors}" % { errors: result[:project].errors.full_messages })
               error(_('Could not create project'))
             end
           end
 
-          def save_project_id
+          def save_project_id(result)
             return success if project_created?
 
-            result = application_settings.update(instance_administration_project_id: @project.id)
+            response = application_settings.update(
+              instance_administration_project_id: result[:project].id
+            )
 
-            if result
-              success
+            if response
+              success(result)
             else
               log_error("Could not save instance administration project ID, errors: %{errors}" % { errors: application_settings.errors.full_messages })
               error(_('Could not save project ID'))
             end
           end
 
-          def add_group_members
-            members = @group.add_users(members_to_add, Gitlab::Access::MAINTAINER)
+          def add_group_members(result)
+            group = result[:group]
+            members = group.add_users(members_to_add(group), Gitlab::Access::MAINTAINER)
             errors = members.flat_map { |member| member.errors.full_messages }
 
             if errors.any?
               log_error('Could not add admins as members to self-monitoring project. Errors: %{errors}' % { errors: errors })
               error(_('Could not add admins as members'))
             else
-              success
+              success(result)
             end
           end
 
-          def add_to_whitelist
-            return success unless prometheus_enabled?
-            return success unless prometheus_listen_address.present?
+          def add_to_whitelist(result)
+            return success(result) unless prometheus_enabled?
+            return success(result) unless prometheus_listen_address.present?
 
             uri = parse_url(internal_prometheus_listen_address_uri)
             return error(_('Prometheus listen_address in config/gitlab.yml is not a valid URI')) unless uri
 
             application_settings.add_to_outbound_local_requests_whitelist([uri.normalized_host])
-            result = application_settings.save
+            response = application_settings.save
 
-            if result
+            if response
               # Expire the Gitlab::CurrentSettings cache after updating the whitelist.
               # This happens automatically in an after_commit hook, but in migrations,
               # the after_commit hook only runs at the end of the migration.
               Gitlab::CurrentSettings.expire_current_application_settings
-              success
+              success(result)
             else
               log_error("Could not add prometheus URL to whitelist, errors: %{errors}" % { errors: application_settings.errors.full_messages })
               error(_('Could not add prometheus URL to whitelist'))
             end
           end
 
-          def add_prometheus_manual_configuration
-            return success unless prometheus_enabled?
-            return success unless prometheus_listen_address.present?
+          def add_prometheus_manual_configuration(result)
+            return success(result) unless prometheus_enabled?
+            return success(result) unless prometheus_listen_address.present?
 
-            service = project.find_or_initialize_service('prometheus')
+            service = result[:project].find_or_initialize_service('prometheus')
 
             unless service.update(prometheus_service_attributes)
               log_error('Could not save prometheus manual configuration for self-monitoring project. Errors: %{errors}' % { errors: service.errors.full_messages })
               return error(_('Could not save prometheus manual configuration'))
             end
 
-            success
+            success(result)
           end
 
           def application_settings
@@ -196,11 +199,11 @@ module Gitlab
             instance_admins.first
           end
 
-          def members_to_add
+          def members_to_add(group)
             # Exclude admins who are already members of group because
-            # `@group.add_users(users)` returns an error if the users parameter contains
+            # `group.add_users(users)` returns an error if the users parameter contains
             # users who are already members of the group.
-            instance_admins - @group.members.collect(&:user)
+            instance_admins - group.members.collect(&:user)
           end
 
           def create_group_params
@@ -217,13 +220,13 @@ module Gitlab
             )
           end
 
-          def create_project_params
+          def create_project_params(group)
             {
               initialize_with_readme: true,
               visibility_level: VISIBILITY_LEVEL,
               name: PROJECT_NAME,
               description: "This project is automatically generated and will be used to help monitor this GitLab instance. [More information](#{docs_path})",
-              namespace_id: @group.id
+              namespace_id: group.id
             }
           end
 

locale/gitlab.pot

@@ -10896,6 +10896,9 @@ msgstr ""
 msgid "No data to display"
 msgstr ""
 
+msgid "No deployment platform available"
+msgstr ""
+
 msgid "No deployments found"
 msgstr ""
 
@@ -10962,6 +10965,9 @@ msgstr ""
 msgid "No parent group"
 msgstr ""
 
+msgid "No pods available"
+msgstr ""
+
 msgid "No preview for this file type"
 msgstr ""
 

spec/models/concerns/stepable_spec.rb

@@ -7,6 +7,8 @@ describe Stepable do
     Class.new do
       include Stepable
 
+      attr_writer :return_non_success
+
       steps :method1, :method2, :method3
 
       def execute
@@ -15,18 +17,18 @@ describe Stepable do
 
       private
 
-      def method1
+      def method1(_result)
         { status: :success }
       end
 
-      def method2
-        return { status: :error } unless @pass
+      def method2(result)
+        return { status: :not_a_success } if @return_non_success
 
-        { status: :success, variable1: 'var1' }
+        result.merge({ status: :success, variable1: 'var1', excluded_variable: 'a' })
       end
 
-      def method3
-        { status: :success, variable2: 'var2' }
+      def method3(result)
+        result.except(:excluded_variable).merge({ status: :success, variable2: 'var2' })
       end
     end
   end
@@ -41,8 +43,8 @@ describe Stepable do
 
       private
 
-      def appended_method1
-        { status: :success }
+      def appended_method1(previous_result)
+        previous_result.merge({ status: :success })
       end
     end
   end
@@ -51,21 +53,19 @@ describe Stepable do
     described_class.prepend(prepended_module)
   end
 
-  it 'stops after the first error' do
+  it 'stops after the first non success status' do
+    subject.return_non_success = true
+
     expect(subject).not_to receive(:method3)
     expect(subject).not_to receive(:appended_method1)
 
     expect(subject.execute).to eq(
-      status: :error,
-      failed_step: :method2
+      status: :not_a_success,
+      last_step: :method2
     )
   end
 
   context 'when all methods return success' do
-    before do
-      subject.instance_variable_set(:@pass, true)
-    end
-
     it 'calls all methods in order' do
       expect(subject).to receive(:method1).and_call_original.ordered
       expect(subject).to receive(:method2).and_call_original.ordered
@@ -82,6 +82,10 @@ describe Stepable do
         variable2: 'var2'
       )
     end
+
+    it 'can modify results of previous steps' do
+      expect(subject.execute).not_to include(excluded_variable: 'a')
+    end
   end
 
   context 'with multiple stepable classes' do