Merge branch 'fix-external-ci-services' into 'master'

Allow to access statuses for external CI services

Closes #30714, #29369, and #15220

See merge request !11176

24 files changed, 175 insertions, 43 deletions

diff --git a/app/controllers/projects/application_controller.rb b/app/controllers/projects/application_controller.rb
index cb4bd0ad5f5..603a51266da 100644
--- a/app/controllers/projects/application_controller.rb
+++ b/app/controllers/projects/application_controller.rb
@@ -80,10 +80,6 @@ class Projects::ApplicationController < ApplicationController
     cookies.permanent[:diff_view] = params.delete(:view) if params[:view].present?
   end
 
-  def builds_enabled
-    return render_404 unless @project.feature_available?(:builds, current_user)
-  end
-
   def require_pages_enabled!
     not_found unless Gitlab.config.pages.enabled
   end
diff --git a/app/controllers/projects/graphs_controller.rb b/app/controllers/projects/graphs_controller.rb
index 43fc0c39801..df5221fe95f 100644
--- a/app/controllers/projects/graphs_controller.rb
+++ b/app/controllers/projects/graphs_controller.rb
@@ -5,7 +5,6 @@ class Projects::GraphsController < Projects::ApplicationController
   before_action :require_non_empty_project
   before_action :assign_ref_vars
   before_action :authorize_download_code!
-  before_action :builds_enabled, only: :ci
 
   def show
     respond_to do |format|
diff --git a/app/controllers/projects/pipelines_controller.rb b/app/controllers/projects/pipelines_controller.rb
index 6223e7943f8..8effb792689 100644
--- a/app/controllers/projects/pipelines_controller.rb
+++ b/app/controllers/projects/pipelines_controller.rb
@@ -4,7 +4,6 @@ class Projects::PipelinesController < Projects::ApplicationController
   before_action :authorize_read_pipeline!
   before_action :authorize_create_pipeline!, only: [:new, :create]
   before_action :authorize_update_pipeline!, only: [:retry, :cancel]
-  before_action :builds_enabled, only: :charts
 
   wrap_parameters Ci::Pipeline
 
diff --git a/app/helpers/projects_helper.rb b/app/helpers/projects_helper.rb
index 7441b58fddb..c11dd49f4a7 100644
--- a/app/helpers/projects_helper.rb
+++ b/app/helpers/projects_helper.rb
@@ -218,6 +218,10 @@ module ProjectsHelper
       nav_tabs << :container_registry
     end
 
+    if project.builds_enabled? && can?(current_user, :read_pipeline, project)
+      nav_tabs << :pipelines
+    end
+
     tab_ability_map.each do |tab, ability|
       if can?(current_user, ability, project)
         nav_tabs << tab
@@ -231,7 +235,6 @@ module ProjectsHelper
     {
       environments: :read_environment,
       milestones:   :read_milestone,
-      pipelines:    :read_pipeline,
       snippets:     :read_project_snippet,
       settings:     :admin_project,
       builds:       :read_build,
diff --git a/app/models/generic_commit_status.rb b/app/models/generic_commit_status.rb
index 8867ba0d2ff..532b8f4ad69 100644
--- a/app/models/generic_commit_status.rb
+++ b/app/models/generic_commit_status.rb
@@ -11,6 +11,7 @@ class GenericCommitStatus < CommitStatus
   def set_default_values
     self.context ||= 'default'
     self.stage ||= 'external'
+    self.stage_idx ||= 1000000
   end
 
   def tags
diff --git a/app/policies/project_policy.rb b/app/policies/project_policy.rb
index 3959b895f44..47518dddb61 100644
--- a/app/policies/project_policy.rb
+++ b/app/policies/project_policy.rb
@@ -203,7 +203,7 @@ class ProjectPolicy < BasePolicy
 
     unless project.feature_available?(:builds, user) && repository_enabled
       cannot!(*named_abilities(:build))
-      cannot!(*named_abilities(:pipeline))
+      cannot!(*named_abilities(:pipeline) - [:read_pipeline])
       cannot!(*named_abilities(:pipeline_schedule))
       cannot!(*named_abilities(:environment))
       cannot!(*named_abilities(:deployment))
diff --git a/app/serializers/build_details_entity.rb b/app/serializers/build_details_entity.rb
index 0eddbaaaebf..cb6d3dfec89 100644
--- a/app/serializers/build_details_entity.rb
+++ b/app/serializers/build_details_entity.rb
@@ -1,4 +1,4 @@
-class BuildDetailsEntity < BuildEntity
+class BuildDetailsEntity < JobEntity
   expose :coverage, :erased_at, :duration
   expose :tag_list, as: :tags
   expose :user, using: UserEntity
diff --git a/app/serializers/build_serializer.rb b/app/serializers/build_serializer.rb
index 79b67001199..bae9932847f 100644
--- a/app/serializers/build_serializer.rb
+++ b/app/serializers/build_serializer.rb
@@ -1,5 +1,5 @@
 class BuildSerializer < BaseSerializer
-  entity BuildEntity
+  entity JobEntity
 
   def represent_status(resource)
     data = represent(resource, { only: [:status] })
diff --git a/app/serializers/deployment_entity.rb b/app/serializers/deployment_entity.rb
index 8b3de1bed0f..e493c9162fd 100644
--- a/app/serializers/deployment_entity.rb
+++ b/app/serializers/deployment_entity.rb
@@ -24,6 +24,6 @@ class DeploymentEntity < Grape::Entity
 
   expose :user, using: UserEntity
   expose :commit, using: CommitEntity
-  expose :deployable, using: BuildEntity
-  expose :manual_actions, using: BuildEntity
+  expose :deployable, using: JobEntity
+  expose :manual_actions, using: JobEntity
 end
diff --git a/app/serializers/build_entity.rb b/app/serializers/job_entity.rb
index 67001f4547d..d6de43bcbcb 100644
--- a/app/serializers/build_entity.rb
+++ b/app/serializers/job_entity.rb
@@ -1,11 +1,11 @@
-class BuildEntity < Grape::Entity
+class JobEntity < Grape::Entity
   include RequestAwareEntity
 
   expose :id
   expose :name
 
   expose :build_path do |build|
-    path_to(:namespace_project_job, build)
+    build.target_url || path_to(:namespace_project_job, build)
   end
 
   expose :retry_path, if: -> (*) { retryable? } do |build|
diff --git a/app/serializers/job_group_entity.rb b/app/serializers/job_group_entity.rb
index 04487e59009..8554de55517 100644
--- a/app/serializers/job_group_entity.rb
+++ b/app/serializers/job_group_entity.rb
@@ -4,7 +4,7 @@ class JobGroupEntity < Grape::Entity
   expose :name
   expose :size
   expose :detailed_status, as: :status, with: StatusEntity
-  expose :jobs, with: BuildEntity
+  expose :jobs, with: JobEntity
 
   private
 
diff --git a/app/services/git_push_service.rb b/app/services/git_push_service.rb
index f080e6326a1..fb1d4aed58b 100644
--- a/app/services/git_push_service.rb
+++ b/app/services/git_push_service.rb
@@ -101,12 +101,12 @@ class GitPushService < BaseService
     UpdateMergeRequestsWorker
       .perform_async(@project.id, current_user.id, params[:oldrev], params[:newrev], params[:ref])
 
-    SystemHookPushWorker.perform_async(build_push_data.dup, :push_hooks)
-
     EventCreateService.new.push(@project, current_user, build_push_data)
+    Ci::CreatePipelineService.new(@project, current_user, build_push_data).execute(:push)
+    
+    SystemHookPushWorker.perform_async(build_push_data.dup, :push_hooks)
     @project.execute_hooks(build_push_data.dup, :push_hooks)
     @project.execute_services(build_push_data.dup, :push_hooks)
-    Ci::CreatePipelineService.new(@project, current_user, build_push_data).execute(:push)
 
     if push_remove_branch?
       AfterBranchDeleteService
diff --git a/app/services/git_tag_push_service.rb b/app/services/git_tag_push_service.rb
index 7c424fba428..9917a39b795 100644
--- a/app/services/git_tag_push_service.rb
+++ b/app/services/git_tag_push_service.rb
@@ -8,10 +8,12 @@ class GitTagPushService < BaseService
     @push_data = build_push_data
 
     EventCreateService.new.push(project, current_user, @push_data)
+    Ci::CreatePipelineService.new(project, current_user, @push_data).execute(:push)
+
     SystemHooksService.new.execute_hooks(build_system_push_data.dup, :tag_push_hooks)
     project.execute_hooks(@push_data.dup, :tag_push_hooks)
     project.execute_services(@push_data.dup, :tag_push_hooks)
-    Ci::CreatePipelineService.new(project, current_user, @push_data).execute(:push)
+
     ProjectCacheWorker.perform_async(project.id, [], [:commit_count, :repository_size])
 
     true
diff --git a/changelogs/unreleased/fix-support-for-external-ci-services.yml b/changelogs/unreleased/fix-support-for-external-ci-services.yml
new file mode 100644
index 00000000000..eecb4519259
--- /dev/null
+++ b/changelogs/unreleased/fix-support-for-external-ci-services.yml
@@ -0,0 +1,4 @@
+---
+title: Fix support for external CI services
+merge_request: 11176
+author:
diff --git a/lib/gitlab/ci/status/external/common.rb b/lib/gitlab/ci/status/external/common.rb
index 4969a350862..9307545b5b1 100644
--- a/lib/gitlab/ci/status/external/common.rb
+++ b/lib/gitlab/ci/status/external/common.rb
@@ -3,6 +3,10 @@ module Gitlab
     module Status
       module External
         module Common
+          def label
+            subject.description
+          end
+
           def has_details?
             subject.target_url.present? &&
               can?(user, :read_commit_status, subject)
diff --git a/spec/controllers/projects/pipelines_controller_spec.rb b/spec/controllers/projects/pipelines_controller_spec.rb
index 954f89e3854..734532668d3 100644
--- a/spec/controllers/projects/pipelines_controller_spec.rb
+++ b/spec/controllers/projects/pipelines_controller_spec.rb
@@ -5,9 +5,12 @@ describe Projects::PipelinesController do
 
   let(:user) { create(:user) }
   let(:project) { create(:empty_project, :public) }
+  let(:feature) { ProjectFeature::DISABLED }
 
   before do
     project.add_developer(user)
+    project.project_feature.update(
+      builds_access_level: feature)
 
     sign_in(user)
   end
@@ -153,16 +156,26 @@ describe Projects::PipelinesController do
                    format: :json
     end
 
-    it 'retries a pipeline without returning any content' do
-      expect(response).to have_http_status(:no_content)
-      expect(build.reload).to be_retried
+    context 'when builds are enabled' do
+      let(:feature) { ProjectFeature::ENABLED }
+  
+      it 'retries a pipeline without returning any content' do
+        expect(response).to have_http_status(:no_content)
+        expect(build.reload).to be_retried
+      end
+    end
+
+    context 'when builds are disabled' do
+      it 'fails to retry pipeline' do
+        expect(response).to have_http_status(:not_found)
+      end
     end
   end
 
   describe 'POST cancel.json' do
     let!(:pipeline) { create(:ci_pipeline, project: project) }
     let!(:build) { create(:ci_build, :running, pipeline: pipeline) }
-
+  
     before do
       post :cancel, namespace_id: project.namespace,
                     project_id: project,
@@ -170,9 +183,19 @@ describe Projects::PipelinesController do
                     format: :json
     end
 
-    it 'cancels a pipeline without returning any content' do
-      expect(response).to have_http_status(:no_content)
-      expect(pipeline.reload).to be_canceled
+    context 'when builds are enabled' do
+      let(:feature) { ProjectFeature::ENABLED }
+  
+      it 'cancels a pipeline without returning any content' do
+        expect(response).to have_http_status(:no_content)
+        expect(pipeline.reload).to be_canceled
+      end
+    end
+
+    context 'when builds are disabled' do
+      it 'fails to retry pipeline' do
+        expect(response).to have_http_status(:not_found)
+      end
     end
   end
 end
diff --git a/spec/features/projects/features_visibility_spec.rb b/spec/features/projects/features_visibility_spec.rb
index c49648f54bd..d76b5e4ef1b 100644
--- a/spec/features/projects/features_visibility_spec.rb
+++ b/spec/features/projects/features_visibility_spec.rb
@@ -68,9 +68,12 @@ describe 'Edit Project Settings', feature: true do
   end
 
   describe 'project features visibility pages' do
+    let(:pipeline) { create(:ci_empty_pipeline, project: project) }
+    let(:job) { create(:ci_build, pipeline: pipeline) }
+
     let(:tools) do
       {
-        builds: namespace_project_pipelines_path(project.namespace, project),
+        builds: namespace_project_job_path(project.namespace, project, job),
         issues: namespace_project_issues_path(project.namespace, project),
         wiki: namespace_project_wiki_path(project.namespace, project, :home),
         snippets: namespace_project_snippets_path(project.namespace, project),
diff --git a/spec/helpers/projects_helper_spec.rb b/spec/helpers/projects_helper_spec.rb
index a695621b87a..3ed0b0a756b 100644
--- a/spec/helpers/projects_helper_spec.rb
+++ b/spec/helpers/projects_helper_spec.rb
@@ -300,4 +300,37 @@ describe ProjectsHelper do
       expect(helper.send(:visibility_select_options, project, Gitlab::VisibilityLevel::PRIVATE)).to include('Private')
     end
   end
+
+  describe '#get_project_nav_tabs' do
+    let(:project) { create(:empty_project) }
+    let(:user)    { create(:user) }
+
+    before do
+      allow(helper).to receive(:can?) { true }
+    end
+
+    subject do
+      helper.send(:get_project_nav_tabs, project, user)
+    end
+
+    context 'when builds feature is enabled' do
+      before do
+        allow(project).to receive(:builds_enabled?).and_return(true)
+      end
+
+      it "does include pipelines tab" do
+        is_expected.to include(:pipelines)
+      end
+    end
+
+    context 'when builds feature is disabled' do
+      before do
+        allow(project).to receive(:builds_enabled?).and_return(false)
+      end
+
+      it "do not include pipelines tab" do
+        is_expected.not_to include(:pipelines)
+      end
+    end
+  end
 end
diff --git a/spec/lib/gitlab/ci/status/external/common_spec.rb b/spec/lib/gitlab/ci/status/external/common_spec.rb
index 5a97d98b55f..e58f5d8d4df 100644
--- a/spec/lib/gitlab/ci/status/external/common_spec.rb
+++ b/spec/lib/gitlab/ci/status/external/common_spec.rb
@@ -4,9 +4,10 @@ describe Gitlab::Ci::Status::External::Common do
   let(:user) { create(:user) }
   let(:project) { external_status.project }
   let(:external_target_url) { 'http://example.gitlab.com/status' }
+  let(:external_description) { 'my description' }
 
   let(:external_status) do
-    create(:generic_commit_status, target_url: external_target_url)
+    create(:generic_commit_status, target_url: external_target_url, description: external_description)
   end
 
   subject do
@@ -15,6 +16,12 @@ describe Gitlab::Ci::Status::External::Common do
       .extend(described_class)
   end
 
+  describe '#label' do
+    it 'returns description' do
+      expect(subject.label).to eq external_description
+    end
+  end
+
   describe '#has_action?' do
     it { is_expected.not_to have_action }
   end
diff --git a/spec/policies/project_policy_spec.rb b/spec/policies/project_policy_spec.rb
index 0d3af1f4499..848fd547e10 100644
--- a/spec/policies/project_policy_spec.rb
+++ b/spec/policies/project_policy_spec.rb
@@ -139,6 +139,18 @@ describe ProjectPolicy, models: true do
           is_expected.not_to include(:read_build, :read_pipeline)
         end
       end
+
+      context 'when builds are disabled' do
+        before do
+          project.project_feature.update(
+            builds_access_level: ProjectFeature::DISABLED)
+        end
+
+        it do
+          is_expected.not_to include(:read_build)
+          is_expected.to include(:read_pipeline)
+        end
+      end
     end
 
     context 'reporter' do
diff --git a/spec/serializers/build_details_entity_spec.rb b/spec/serializers/build_details_entity_spec.rb
index 396ba96e9b3..b92c1c28ba8 100644
--- a/spec/serializers/build_details_entity_spec.rb
+++ b/spec/serializers/build_details_entity_spec.rb
@@ -3,8 +3,8 @@ require 'spec_helper'
 describe BuildDetailsEntity do
   set(:user) { create(:admin) }
 
-  it 'inherits from BuildEntity' do
-    expect(described_class).to be < BuildEntity
+  it 'inherits from JobEntity' do
+    expect(described_class).to be < JobEntity
   end
 
   describe '#as_json' do
diff --git a/spec/serializers/build_entity_spec.rb b/spec/serializers/job_entity_spec.rb
index e51ff9fc709..5ca7bf2fcaf 100644
--- a/spec/serializers/build_entity_spec.rb
+++ b/spec/serializers/job_entity_spec.rb
@@ -1,9 +1,9 @@
 require 'spec_helper'
 
-describe BuildEntity do
+describe JobEntity do
   let(:user) { create(:user) }
-  let(:build) { create(:ci_build) }
-  let(:project) { build.project }
+  let(:job) { create(:ci_build) }
+  let(:project) { job.project }
   let(:request) { double('request') }
 
   before do
@@ -12,12 +12,12 @@ describe BuildEntity do
   end
 
   let(:entity) do
-    described_class.new(build, request: request)
+    described_class.new(job, request: request)
   end
 
   subject { entity.as_json }
 
-  it 'contains paths to build page action' do
+  it 'contains paths to job page action' do
     expect(subject).to include(:build_path)
   end
 
@@ -27,7 +27,7 @@ describe BuildEntity do
   end
 
   it 'contains whether it is playable' do
-    expect(subject[:playable]).to eq build.playable?
+    expect(subject[:playable]).to eq job.playable?
   end
 
   it 'contains timestamps' do
@@ -39,9 +39,9 @@ describe BuildEntity do
     expect(subject[:status]).to include :icon, :favicon, :text, :label
   end
 
-  context 'when build is retryable' do
+  context 'when job is retryable' do
     before do
-      build.update(status: :failed)
+      job.update(status: :failed)
     end
 
     it 'contains cancel path' do
@@ -49,9 +49,9 @@ describe BuildEntity do
     end
   end
 
-  context 'when build is cancelable' do
+  context 'when job is cancelable' do
     before do
-      build.update(status: :running)
+      job.update(status: :running)
     end
 
     it 'contains cancel path' do
@@ -59,7 +59,7 @@ describe BuildEntity do
     end
   end
 
-  context 'when build is a regular build' do
+  context 'when job is a regular job' do
     it 'does not contain path to play action' do
       expect(subject).not_to include(:play_path)
     end
@@ -69,8 +69,8 @@ describe BuildEntity do
     end
   end
 
-  context 'when build is a manual action' do
-    let(:build) { create(:ci_build, :manual) }
+  context 'when job is a manual action' do
+    let(:job) { create(:ci_build, :manual) }
 
     context 'when user is allowed to trigger action' do
       before do
@@ -99,4 +99,25 @@ describe BuildEntity do
       end
     end
   end
+
+  context 'when job is generic commit status' do
+    let(:job) { create(:generic_commit_status, target_url: 'http://google.com') }
+
+    it 'contains paths to target action' do
+      expect(subject).to include(:build_path)
+    end
+
+    it 'does not contain paths to other action paths' do
+      expect(subject).not_to include(:retry_path, :cancel_path, :play_path)
+    end
+
+    it 'contains timestamps' do
+      expect(subject).to include(:created_at, :updated_at)
+    end
+
+    it 'contains details' do
+      expect(subject).to include :status
+      expect(subject[:status]).to include :icon, :favicon, :text, :label
+    end
+  end
 end
diff --git a/spec/serializers/pipeline_details_entity_spec.rb b/spec/serializers/pipeline_details_entity_spec.rb
index 03cc5ae9b63..5cb9b9945b6 100644
--- a/spec/serializers/pipeline_details_entity_spec.rb
+++ b/spec/serializers/pipeline_details_entity_spec.rb
@@ -91,6 +91,20 @@ describe PipelineDetailsEntity do
       end
     end
 
+    context 'when pipeline has commit statuses' do
+      let(:pipeline) { create(:ci_empty_pipeline) }
+    
+      before do
+        create(:generic_commit_status, pipeline: pipeline)
+      end
+
+      it 'contains stages' do
+        expect(subject).to include(:details)
+        expect(subject[:details]).to include(:stages)
+        expect(subject[:details][:stages].first).to include(name: 'external')
+      end
+    end
+
     context 'when pipeline has YAML errors' do
       let(:pipeline) do
         create(:ci_pipeline, config: { rspec: { invalid: :value } })
diff --git a/spec/serializers/stage_entity_spec.rb b/spec/serializers/stage_entity_spec.rb
index 64b3217b809..40e303f7b89 100644
--- a/spec/serializers/stage_entity_spec.rb
+++ b/spec/serializers/stage_entity_spec.rb
@@ -54,6 +54,17 @@ describe StageEntity do
       it 'exposes the group key' do
         expect(subject).to include :groups
       end
+
+      context 'and contains commit status' do
+        before do
+          create(:generic_commit_status, pipeline: pipeline, stage: 'test')
+        end
+
+        it 'contains commit status' do
+          groups = subject[:groups].map { |group| group[:name] }
+          expect(groups).to include('generic')
+        end
+      end
     end
   end
 end