diff options
author | Sean McGivern <sean@mcgivern.me.uk> | 2018-11-07 17:51:26 +0300 |
---|---|---|
committer | Sean McGivern <sean@mcgivern.me.uk> | 2018-11-07 17:51:26 +0300 |
commit | 2120078e6ba5217dc2604235e799ed5d0de13bff (patch) | |
tree | c4e27dcf7f7df136f76d40480c4e25d69150907b | |
parent | 1183183403cadb465cfec52532cb77ad6a9494c9 (diff) | |
parent | 90df732144263d058039a3846d9fe4420816efe1 (diff) |
Merge branch 'fix-environment-status-in-merge-request-widget' into 'master'
Fix environment status in merge request widget
Closes #51120 and #25140
See merge request gitlab-org/gitlab-ce!22799
-rw-r--r-- | app/models/ci/build.rb | 1 | ||||
-rw-r--r-- | app/models/deployment.rb | 4 | ||||
-rw-r--r-- | app/models/environment.rb | 1 | ||||
-rw-r--r-- | app/models/environment_status.rb | 44 | ||||
-rw-r--r-- | changelogs/unreleased/add-action-to-deployment.yml | 5 | ||||
-rw-r--r-- | db/migrate/20181106135939_add_index_to_deployments.rb | 17 | ||||
-rw-r--r-- | db/schema.rb | 1 | ||||
-rw-r--r-- | spec/features/merge_request/user_sees_deployment_widget_spec.rb | 62 | ||||
-rw-r--r-- | spec/features/projects/environments/environment_spec.rb | 41 | ||||
-rw-r--r-- | spec/features/projects/environments/environments_spec.rb | 18 | ||||
-rw-r--r-- | spec/models/environment_spec.rb | 19 | ||||
-rw-r--r-- | spec/models/environment_status_spec.rb | 88 | ||||
-rw-r--r-- | spec/serializers/environment_status_entity_spec.rb | 1 |
13 files changed, 265 insertions, 37 deletions
diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 25596581d0f..5c1168e8ef5 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -211,6 +211,7 @@ module Ci build.deployment&.succeed build.run_after_commit do + BuildSuccessWorker.perform_async(id) PagesWorker.perform_async(:deploy, id) if build.pages_generator? end end diff --git a/app/models/deployment.rb b/app/models/deployment.rb index 54a900a3b85..83434276995 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -10,7 +10,9 @@ class Deployment < ActiveRecord::Base belongs_to :user belongs_to :deployable, polymorphic: true # rubocop:disable Cop/PolymorphicAssociations - has_internal_id :iid, scope: :project, init: ->(s) { s&.project&.deployments&.maximum(:iid) } + has_internal_id :iid, scope: :project, init: ->(s) do + Deployment.where(project: s.project).maximum(:iid) if s&.project + end validates :sha, presence: true validates :ref, presence: true diff --git a/app/models/environment.rb b/app/models/environment.rb index 7d104bb0c25..934828946b9 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -50,6 +50,7 @@ class Environment < ActiveRecord::Base scope :in_review_folder, -> { where(environment_type: "review") } scope :for_name, -> (name) { where(name: name) } scope :for_project, -> (project) { where(project_id: project) } + scope :with_deployment, -> (sha) { where('EXISTS (?)', Deployment.select(1).where('deployments.environment_id = environments.id').where(sha: sha)) } state_machine :state, initial: :available do event :start do diff --git a/app/models/environment_status.rb b/app/models/environment_status.rb index 7efc8da09ad..7078496ff52 100644 --- a/app/models/environment_status.rb +++ b/app/models/environment_status.rb @@ -8,17 +8,16 @@ class EnvironmentStatus delegate :id, to: :environment delegate :name, to: :environment delegate :project, to: :environment - delegate :status, to: :deployment, allow_nil: true delegate :deployed_at, to: :deployment, allow_nil: true def self.for_merge_request(mr, user) - build_environments_status(mr, user, mr.head_pipeline) + build_environments_status(mr, user, mr.diff_head_sha) end def self.after_merge_request(mr, user) return [] unless mr.merged? - build_environments_status(mr, user, mr.merge_pipeline) + build_environments_status(mr, user, mr.merge_commit_sha) end def initialize(environment, merge_request, sha) @@ -29,7 +28,7 @@ class EnvironmentStatus def deployment strong_memoize(:deployment) do - environment.first_deployment_for(sha) + Deployment.where(environment: environment).find_by_sha(sha) end end @@ -44,6 +43,22 @@ class EnvironmentStatus .merge_request_diff_files.where(deleted_file: false) end + ## + # Since frontend has not supported all statuses yet, BE has to + # proxy some status to a supported status. + def status + return unless deployment + + case deployment.status + when 'created' + 'running' + when 'canceled' + 'failed' + else + deployment.status + end + end + private PAGE_EXTENSIONS = /\A\.(s?html?|php|asp|cgi|pl)\z/i.freeze @@ -61,21 +76,14 @@ class EnvironmentStatus } end - def self.build_environments_status(mr, user, pipeline) - return [] unless pipeline.present? + def self.build_environments_status(mr, user, sha) + Environment.where(project_id: [mr.source_project_id, mr.target_project_id]) + .available + .with_deployment(sha).map do |environment| + next unless Ability.allowed?(user, :read_environment, environment) - find_environments(user, pipeline).map do |environment| - EnvironmentStatus.new(environment, mr, pipeline.sha) - end + EnvironmentStatus.new(environment, mr, sha) + end.compact end private_class_method :build_environments_status - - def self.find_environments(user, pipeline) - env_ids = Deployment.where(deployable: pipeline.builds).select(:environment_id) - - Environment.available.where(id: env_ids).select do |environment| - Ability.allowed?(user, :read_environment, environment) - end - end - private_class_method :find_environments end diff --git a/changelogs/unreleased/add-action-to-deployment.yml b/changelogs/unreleased/add-action-to-deployment.yml new file mode 100644 index 00000000000..4629f762ae8 --- /dev/null +++ b/changelogs/unreleased/add-action-to-deployment.yml @@ -0,0 +1,5 @@ +--- +title: Fix environment status in merge request widget +merge_request: 22799 +author: +type: changed diff --git a/db/migrate/20181106135939_add_index_to_deployments.rb b/db/migrate/20181106135939_add_index_to_deployments.rb new file mode 100644 index 00000000000..5f988a4723c --- /dev/null +++ b/db/migrate/20181106135939_add_index_to_deployments.rb @@ -0,0 +1,17 @@ +# frozen_string_literal: true + +class AddIndexToDeployments < ActiveRecord::Migration + include Gitlab::Database::MigrationHelpers + + DOWNTIME = false + + disable_ddl_transaction! + + def up + add_concurrent_index :deployments, [:project_id, :status, :created_at] + end + + def down + remove_concurrent_index :deployments, [:project_id, :status, :created_at] + end +end diff --git a/db/schema.rb b/db/schema.rb index 765cbf0606a..2c8300839d8 100644 --- a/db/schema.rb +++ b/db/schema.rb @@ -837,6 +837,7 @@ ActiveRecord::Schema.define(version: 20181107054254) do add_index "deployments", ["environment_id", "status"], name: "index_deployments_on_environment_id_and_status", using: :btree add_index "deployments", ["id"], name: "partial_index_deployments_for_legacy_successful_deployments", where: "((finished_at IS NULL) AND (status = 2))", using: :btree add_index "deployments", ["project_id", "iid"], name: "index_deployments_on_project_id_and_iid", unique: true, using: :btree + add_index "deployments", ["project_id", "status", "created_at"], name: "index_deployments_on_project_id_and_status_and_created_at", using: :btree add_index "deployments", ["project_id", "status"], name: "index_deployments_on_project_id_and_status", using: :btree create_table "emails", force: :cascade do |t| diff --git a/spec/features/merge_request/user_sees_deployment_widget_spec.rb b/spec/features/merge_request/user_sees_deployment_widget_spec.rb index 0e439c8cb2d..74290c0fff9 100644 --- a/spec/features/merge_request/user_sees_deployment_widget_spec.rb +++ b/spec/features/merge_request/user_sees_deployment_widget_spec.rb @@ -1,7 +1,7 @@ require 'rails_helper' describe 'Merge request > User sees deployment widget', :js do - describe 'when deployed to an environment' do + describe 'when merge request has associated environments' do let(:user) { create(:user) } let(:project) { create(:project, :repository) } let(:merge_request) { create(:merge_request, :merged, source_project: project) } @@ -10,30 +10,74 @@ describe 'Merge request > User sees deployment widget', :js do let(:ref) { merge_request.target_branch } let(:sha) { project.commit(ref).id } let(:pipeline) { create(:ci_pipeline_without_jobs, sha: sha, project: project, ref: ref) } - let(:build) { create(:ci_build, :success, pipeline: pipeline) } - let!(:deployment) { create(:deployment, :succeed, environment: environment, sha: sha, ref: ref, deployable: build) } let!(:manual) { } before do merge_request.update!(merge_commit_sha: sha) project.add_user(user, role) sign_in(user) - visit project_merge_request_path(project, merge_request) - wait_for_requests end - it 'displays that the environment is deployed' do - wait_for_requests + context 'when deployment succeeded' do + let(:build) { create(:ci_build, :success, pipeline: pipeline) } + let!(:deployment) { create(:deployment, :succeed, environment: environment, sha: sha, ref: ref, deployable: build) } - expect(page).to have_content("Deployed to #{environment.name}") - expect(find('.js-deploy-time')['data-original-title']).to eq(deployment.created_at.to_time.in_time_zone.to_s(:medium)) + it 'displays that the environment is deployed' do + visit project_merge_request_path(project, merge_request) + wait_for_requests + + expect(page).to have_content("Deployed to #{environment.name}") + expect(find('.js-deploy-time')['data-original-title']).to eq(deployment.created_at.to_time.in_time_zone.to_s(:medium)) + end + end + + context 'when deployment failed' do + let(:build) { create(:ci_build, :failed, pipeline: pipeline) } + let!(:deployment) { create(:deployment, :failed, environment: environment, sha: sha, ref: ref, deployable: build) } + + it 'displays that the deployment failed' do + visit project_merge_request_path(project, merge_request) + wait_for_requests + + expect(page).to have_content("Failed to deploy to #{environment.name}") + expect(page).not_to have_css('.js-deploy-time') + end + end + + context 'when deployment running' do + let(:build) { create(:ci_build, :running, pipeline: pipeline) } + let!(:deployment) { create(:deployment, :running, environment: environment, sha: sha, ref: ref, deployable: build) } + + it 'displays that the running deployment' do + visit project_merge_request_path(project, merge_request) + wait_for_requests + + expect(page).to have_content("Deploying to #{environment.name}") + expect(page).not_to have_css('.js-deploy-time') + end + end + + context 'when deployment will happen' do + let(:build) { create(:ci_build, :created, pipeline: pipeline) } + let!(:deployment) { create(:deployment, environment: environment, sha: sha, ref: ref, deployable: build) } + + it 'displays that the environment name' do + visit project_merge_request_path(project, merge_request) + wait_for_requests + + expect(page).to have_content("Deploying to #{environment.name}") + expect(page).not_to have_css('.js-deploy-time') + end end context 'with stop action' do + let(:build) { create(:ci_build, :success, pipeline: pipeline) } + let!(:deployment) { create(:deployment, :succeed, environment: environment, sha: sha, ref: ref, deployable: build) } let(:manual) { create(:ci_build, :manual, pipeline: pipeline, name: 'close_app') } before do deployment.update!(on_stop: manual.name) + visit project_merge_request_path(project, merge_request) wait_for_requests end diff --git a/spec/features/projects/environments/environment_spec.rb b/spec/features/projects/environments/environment_spec.rb index 056f4ee2e22..9772a7bacac 100644 --- a/spec/features/projects/environments/environment_spec.rb +++ b/spec/features/projects/environments/environment_spec.rb @@ -25,7 +25,7 @@ describe 'Environment' do end context 'without deployments' do - it 'does show no deployments' do + it 'does not show deployments' do expect(page).to have_content('You don\'t have any deployments right now.') end end @@ -43,6 +43,45 @@ describe 'Environment' do end end + context 'when there is a successful deployment' do + let(:pipeline) { create(:ci_pipeline, project: project) } + let(:build) { create(:ci_build, :success, pipeline: pipeline) } + + let(:deployment) do + create(:deployment, :success, environment: environment, deployable: build) + end + + it 'does show deployments' do + expect(page).to have_link("#{build.name} (##{build.id})") + end + end + + context 'when there is a running deployment' do + let(:pipeline) { create(:ci_pipeline, project: project) } + let(:build) { create(:ci_build, pipeline: pipeline) } + + let(:deployment) do + create(:deployment, :running, environment: environment, deployable: build) + end + + it 'does not show deployments' do + expect(page).to have_content('You don\'t have any deployments right now.') + end + end + + context 'when there is a failed deployment' do + let(:pipeline) { create(:ci_pipeline, project: project) } + let(:build) { create(:ci_build, pipeline: pipeline) } + + let(:deployment) do + create(:deployment, :failed, environment: environment, deployable: build) + end + + it 'does not show deployments' do + expect(page).to have_content('You don\'t have any deployments right now.') + end + end + context 'with related deployable present' do let(:pipeline) { create(:ci_pipeline, project: project) } let(:build) { create(:ci_build, pipeline: pipeline) } diff --git a/spec/features/projects/environments/environments_spec.rb b/spec/features/projects/environments/environments_spec.rb index d0ddf69d574..89954d35f91 100644 --- a/spec/features/projects/environments/environments_spec.rb +++ b/spec/features/projects/environments/environments_spec.rb @@ -128,7 +128,7 @@ describe 'Environments page', :js do end end - context 'when there are deployments' do + context 'when there are successful deployments' do let(:project) { create(:project, :repository) } let!(:deployment) do @@ -328,6 +328,22 @@ describe 'Environments page', :js do end end end + + context 'when there is a failed deployment' do + let(:project) { create(:project, :repository) } + + let!(:deployment) do + create(:deployment, :failed, + environment: environment, + sha: project.commit.id) + end + + it 'does not show deployments' do + visit_environments(project) + + expect(page).to have_content('No deployments yet') + end + end end it 'does have a new environment button' do diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index e121369f6ac..9a3f1f1c5a1 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -53,6 +53,25 @@ describe Environment do end end + describe '.with_deployment' do + subject { described_class.with_deployment(sha) } + + let(:environment) { create(:environment) } + let(:sha) { RepoHelpers.sample_commit.id } + + context 'when deployment has the specified sha' do + let!(:deployment) { create(:deployment, environment: environment, sha: sha) } + + it { is_expected.to eq([environment]) } + end + + context 'when deployment does not have the specified sha' do + let!(:deployment) { create(:deployment, environment: environment, sha: 'abc') } + + it { is_expected.to be_empty } + end + end + describe '#folder_name' do context 'when it is inside a folder' do subject(:environment) do diff --git a/spec/models/environment_status_spec.rb b/spec/models/environment_status_spec.rb index 52b98552184..90f7e4a4590 100644 --- a/spec/models/environment_status_spec.rb +++ b/spec/models/environment_status_spec.rb @@ -1,8 +1,10 @@ require 'spec_helper' describe EnvironmentStatus do + include ProjectForksHelper + let(:deployment) { create(:deployment, :succeed, :review_app) } - let(:environment) { deployment.environment} + let(:environment) { deployment.environment } let(:project) { deployment.project } let(:merge_request) { create(:merge_request, :deployed_review_app, deployment: deployment) } let(:sha) { deployment.sha } @@ -65,9 +67,9 @@ describe EnvironmentStatus do let(:admin) { create(:admin) } let(:pipeline) { create(:ci_pipeline, sha: sha) } - it 'is based on merge_request.head_pipeline' do - expect(merge_request).to receive(:head_pipeline).and_return(pipeline) - expect(merge_request).not_to receive(:merge_pipeline) + it 'is based on merge_request.diff_head_sha' do + expect(merge_request).to receive(:diff_head_sha) + expect(merge_request).not_to receive(:merge_commit_sha) described_class.for_merge_request(merge_request, admin) end @@ -81,11 +83,83 @@ describe EnvironmentStatus do merge_request.mark_as_merged! end - it 'is based on merge_request.merge_pipeline' do - expect(merge_request).to receive(:merge_pipeline).and_return(pipeline) - expect(merge_request).not_to receive(:head_pipeline) + it 'is based on merge_request.merge_commit_sha' do + expect(merge_request).to receive(:merge_commit_sha) + expect(merge_request).not_to receive(:diff_head_sha) described_class.after_merge_request(merge_request, admin) end end + + describe '.build_environments_status' do + subject { described_class.send(:build_environments_status, merge_request, user, sha) } + + let!(:build) { create(:ci_build, :deploy_to_production, pipeline: pipeline) } + let(:environment) { build.deployment.environment } + let(:user) { project.owner } + + before do + build.deployment&.update!(sha: sha) + end + + context 'when environment is created on a forked project' do + let(:project) { create(:project, :repository) } + let(:forked) { fork_project(project, user, repository: true) } + let(:sha) { forked.commit.sha } + let(:pipeline) { create(:ci_pipeline, sha: sha, project: forked) } + + let(:merge_request) do + create(:merge_request, + source_project: forked, + target_project: project, + target_branch: 'master', + head_pipeline: pipeline) + end + + it 'returns environment status' do + expect(subject.count).to eq(1) + expect(subject[0].environment).to eq(environment) + expect(subject[0].merge_request).to eq(merge_request) + expect(subject[0].sha).to eq(sha) + end + end + + context 'when environment is created on a target project' do + let(:project) { create(:project, :repository) } + let(:sha) { project.commit.sha } + let(:pipeline) { create(:ci_pipeline, sha: sha, project: project) } + + let(:merge_request) do + create(:merge_request, + source_project: project, + source_branch: 'feature', + target_project: project, + target_branch: 'master', + head_pipeline: pipeline) + end + + it 'returns environment status' do + expect(subject.count).to eq(1) + expect(subject[0].environment).to eq(environment) + expect(subject[0].merge_request).to eq(merge_request) + expect(subject[0].sha).to eq(sha) + end + + context 'when the build stops an environment' do + let!(:build) { create(:ci_build, :stop_review_app, pipeline: pipeline) } + + it 'does not return environment status' do + expect(subject.count).to eq(0) + end + end + + context 'when user does not have a permission to see the environment' do + let(:user) { create(:user) } + + it 'does not return environment status' do + expect(subject.count).to eq(0) + end + end + end + end end diff --git a/spec/serializers/environment_status_entity_spec.rb b/spec/serializers/environment_status_entity_spec.rb index 962ec919092..52bd40ecb5e 100644 --- a/spec/serializers/environment_status_entity_spec.rb +++ b/spec/serializers/environment_status_entity_spec.rb @@ -15,6 +15,7 @@ describe EnvironmentStatusEntity do subject { entity.as_json } before do + deployment.update(sha: merge_request.diff_head_sha) allow(request).to receive(:current_user).and_return(user) end |