From 89c2e0ce4a5e3e2a904fead1c940c3881845597b Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Thu, 8 Sep 2016 21:33:11 +0200 Subject: [PATCH 01/20] Show the time ago a MR got deployed Per deployment a ref is saved in refs/environments//, so we can use for-each-refs go find the first ID to contain the SHA. Given there is a new ref, the old keep_around call is not needed any more. --- app/models/deployment.rb | 31 ++++++++++--------- app/models/environment.rb | 8 +++-- app/models/merge_request.rb | 13 +++++--- app/models/repository.rb | 19 +++++++++++- app/services/create_deployment_service.rb | 19 +++++++----- .../merge_requests/widget/_heading.html.haml | 4 ++- spec/models/merge_request_spec.rb | 2 +- 7 files changed, 63 insertions(+), 33 deletions(-) diff --git a/app/models/deployment.rb b/app/models/deployment.rb index 1e338889714c..dcfbb451d178 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -11,7 +11,13 @@ class Deployment < ActiveRecord::Base delegate :name, to: :environment, prefix: true - after_save :keep_around_commit + def last? + self == environment.last_deployment + end + + def short_sha + Commit.truncate_sha(sha) + end def commit project.commit(sha) @@ -21,25 +27,22 @@ def commit_title commit.try(:title) end - def short_sha - Commit.truncate_sha(sha) - end - - def last? - self == environment.last_deployment - end - - def keep_around_commit - project.repository.keep_around(self.sha) + def create_refs + project.repository.create_ref(ref_path, sha) + project.repository.create_ref(ref_path_head, sha) end def manual_actions deployable.try(:other_actions) end - def includes_commit?(commit) - return false unless commit + private + + def ref_path + environment.ref_path + id.to_s + end - project.repository.is_ancestor?(commit.id, sha) + def ref_path_head + environment.ref_path + 'HEAD' end end diff --git a/app/models/environment.rb b/app/models/environment.rb index 33c9abf382a1..1915c754427b 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -38,9 +38,11 @@ def set_environment_type end end - def includes_commit?(commit) - return false unless last_deployment + def deployment_for(commit) + project.repository.deployment_ref_for_sha(ref_path, commit.sha) + end - last_deployment.includes_commit?(commit) + def ref_path + "refs/environments/#{name}/" end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 75f48fd4ba5c..12a80d4b745d 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -651,12 +651,15 @@ def mergeable_ci_state? !pipeline || pipeline.success? end - def environments - return [] unless diff_head_commit + def deployments + return nil unless diff_head_commit - target_project.environments.select do |environment| - environment.includes_commit?(diff_head_commit) - end + deployment_ids = + target_project.environments.map do |environment| + environment.deployment_for(diff_head_commit) + end + + deployment_ids.empty? ? nil : Deployment.find(deployment_ids) end def state_human_name diff --git a/app/models/repository.rb b/app/models/repository.rb index 772c62a4124f..b535b3e624e8 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -241,7 +241,7 @@ def keep_around(sha) # This will still fail if the file is corrupted (e.g. 0 bytes) begin - rugged.references.create(keep_around_ref_name(sha), sha, force: true) + create_ref(keep_around_ref_name(sha), sha) rescue Rugged::ReferenceError => ex Rails.logger.error "Unable to create keep-around reference for repository #{path}: #{ex}" rescue Rugged::OSError => ex @@ -258,6 +258,10 @@ def kept_around?(sha) end end + def create_ref(ref_name, sha, force: true) + rugged.references.create(ref_name, sha, force: force) + end + def tag_names cache.fetch(:tag_names) { raw_repository.tag_names } end @@ -717,6 +721,19 @@ def contributors end end + def deployment_ref_for_sha(environment_ref_path, sha) + args = %W(#{Gitlab.config.git.bin_path} for-each-ref --count=1 #{environment_ref_path} --contains #{sha}) + + # Not found -> ["", 0] + # Found -> ["b8d95eb4969eefacb0a58f6a28f6803f8070e7b9 commit\trefs/environments/production/77\n", 0] + ref = Gitlab::Popen.popen(args, path_to_repo).first + + return nil if ref.empty? + + # Parse " commit\trefs/environments//\n" to ID + ref.split('/').last.rstrip! + end + def refs_contains_sha(ref_type, sha) args = %W(#{Gitlab.config.git.bin_path} #{ref_type} --contains #{sha}) names = Gitlab::Popen.popen(args, path_to_repo).first diff --git a/app/services/create_deployment_service.rb b/app/services/create_deployment_service.rb index e6667132e275..fb8b4a50341a 100644 --- a/app/services/create_deployment_service.rb +++ b/app/services/create_deployment_service.rb @@ -4,14 +4,17 @@ class CreateDeploymentService < BaseService def execute(deployable = nil) environment = find_or_create_environment - project.deployments.create( - environment: environment, - ref: params[:ref], - tag: params[:tag], - sha: params[:sha], - user: current_user, - deployable: deployable - ) + deployment = project.deployments.create( + environment: environment, + ref: params[:ref], + tag: params[:tag], + sha: params[:sha], + user: current_user, + deployable: deployable + ) + deployment.create_refs + + deployment end private diff --git a/app/views/projects/merge_requests/widget/_heading.html.haml b/app/views/projects/merge_requests/widget/_heading.html.haml index 494695a03a50..464b7e54dc7c 100644 --- a/app/views/projects/merge_requests/widget/_heading.html.haml +++ b/app/views/projects/merge_requests/widget/_heading.html.haml @@ -43,7 +43,8 @@ = icon("times-circle") Could not connect to the CI server. Please check your settings and try again. -- @merge_request.environments.each do |environment| +- @merge_request.deployments.each do |deployment| + - environment = deployment.environment .mr-widget-heading .ci_widget.ci-success = ci_icon_for_status("success") @@ -51,6 +52,7 @@ Deployed to = succeed '.' do = link_to environment.name, namespace_project_environment_path(@project.namespace, @project, environment), class: 'environment' + = time_ago_with_tooltip(deployment.created_at) - external_url = environment.external_url - if external_url = link_to external_url, target: '_blank' do diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 06feeb1bbba6..76eb03b6acff 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -701,7 +701,7 @@ end end - describe "#environments" do + describe "#deployments" do let(:project) { create(:project) } let(:merge_request) { create(:merge_request, source_project: project) } -- GitLab From 56a98241eb676b7b4f62ea9375abbd8a00879260 Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Fri, 9 Sep 2016 11:57:12 +0200 Subject: [PATCH 02/20] Add and update tests --- CHANGELOG | 1 + app/models/deployment.rb | 6 ++- app/models/environment.rb | 2 +- app/models/merge_request.rb | 4 +- app/models/repository.rb | 6 +-- app/services/create_deployment_service.rb | 14 +++---- spec/models/deployment_spec.rb | 24 ----------- spec/models/environment_spec.rb | 40 ++++++------------- spec/models/merge_request_spec.rb | 23 ----------- .../create_deployment_service_spec.rb | 9 +++-- 10 files changed, 33 insertions(+), 96 deletions(-) diff --git a/CHANGELOG b/CHANGELOG index 51411e1d7f3e..512d3082dff6 100644 --- a/CHANGELOG +++ b/CHANGELOG @@ -25,6 +25,7 @@ v 8.12.0 (unreleased) - Move pushes_since_gc from the database to Redis - Add font color contrast to external label in admin area (ClemMakesApps) - Change logo animation to CSS (ClemMakesApps) + - Show how much time ago a merge request was deployed !6273 - Instructions for enabling Git packfile bitmaps !6104 - Use Search::GlobalService.new in the `GET /projects/search/:query` endpoint - Fix long comments in diffs messing with table width diff --git a/app/models/deployment.rb b/app/models/deployment.rb index dcfbb451d178..a9e494b29a93 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -28,8 +28,10 @@ def commit_title end def create_refs - project.repository.create_ref(ref_path, sha) - project.repository.create_ref(ref_path_head, sha) + path = project.repository.path_to_repo + + project.repository.fetch_ref(path, ref, ref_path) + project.repository.fetch_ref(path, ref, ref_path_head) end def manual_actions diff --git a/app/models/environment.rb b/app/models/environment.rb index 1915c754427b..37787ec9ad40 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -43,6 +43,6 @@ def deployment_for(commit) end def ref_path - "refs/environments/#{name}/" + "refs/environments/#{Shellwords.shellescape(name)}/" end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 12a80d4b745d..330557af5f29 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -657,9 +657,9 @@ def deployments deployment_ids = target_project.environments.map do |environment| environment.deployment_for(diff_head_commit) - end + end.compact - deployment_ids.empty? ? nil : Deployment.find(deployment_ids) + deployment_ids.empty? ? nil : target_project.deployments.find(deployment_ids) end def state_human_name diff --git a/app/models/repository.rb b/app/models/repository.rb index b535b3e624e8..f337e75f51f7 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -241,7 +241,7 @@ def keep_around(sha) # This will still fail if the file is corrupted (e.g. 0 bytes) begin - create_ref(keep_around_ref_name(sha), sha) + rugged.references.create(keep_around_ref_name(sha), sha, force: true) rescue Rugged::ReferenceError => ex Rails.logger.error "Unable to create keep-around reference for repository #{path}: #{ex}" rescue Rugged::OSError => ex @@ -258,10 +258,6 @@ def kept_around?(sha) end end - def create_ref(ref_name, sha, force: true) - rugged.references.create(ref_name, sha, force: force) - end - def tag_names cache.fetch(:tag_names) { raw_repository.tag_names } end diff --git a/app/services/create_deployment_service.rb b/app/services/create_deployment_service.rb index fb8b4a50341a..29278a8bb569 100644 --- a/app/services/create_deployment_service.rb +++ b/app/services/create_deployment_service.rb @@ -5,13 +5,13 @@ def execute(deployable = nil) environment = find_or_create_environment deployment = project.deployments.create( - environment: environment, - ref: params[:ref], - tag: params[:tag], - sha: params[:sha], - user: current_user, - deployable: deployable - ) + environment: environment, + ref: params[:ref], + tag: params[:tag], + sha: params[:sha], + user: current_user, + deployable: deployable + ) deployment.create_refs deployment diff --git a/spec/models/deployment_spec.rb b/spec/models/deployment_spec.rb index bfff639ad78c..7df3df4bb9e6 100644 --- a/spec/models/deployment_spec.rb +++ b/spec/models/deployment_spec.rb @@ -15,28 +15,4 @@ it { is_expected.to validate_presence_of(:ref) } it { is_expected.to validate_presence_of(:sha) } - - describe '#includes_commit?' do - let(:project) { create(:project) } - let(:environment) { create(:environment, project: project) } - let(:deployment) do - create(:deployment, environment: environment, sha: project.commit.id) - end - - context 'when there is no project commit' do - it 'returns false' do - commit = project.commit('feature') - - expect(deployment.includes_commit?(commit)).to be false - end - end - - context 'when they share the same tree branch' do - it 'returns true' do - commit = project.commit - - expect(deployment.includes_commit?(commit)).to be true - end - end - end end diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index 6b1867a44e12..c699d8b85c70 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -31,36 +31,20 @@ end end - describe '#includes_commit?' do - context 'without a last deployment' do - it "returns false" do - expect(environment.includes_commit?('HEAD')).to be false - end + describe '#deployment_for' do + let(:project) { create(:project) } + let!(:environment) { create(:environment, project: project) } + let(:deployment) { create(:deployment, environment: environment) } + let(:deployment1) { create(:deployment, environment: environment, sha: commit.id) } + let(:commit) { project.commit } + + before do + deployment.create_refs + deployment1.create_refs end - context 'with a last deployment' do - let(:project) { create(:project) } - let(:environment) { create(:environment, project: project) } - - let!(:deployment) do - create(:deployment, environment: environment, sha: project.commit('master').id) - end - - context 'in the same branch' do - it 'returns true' do - expect(environment.includes_commit?(RepoHelpers.sample_commit)).to be true - end - end - - context 'not in the same branch' do - before do - deployment.update(sha: project.commit('feature').id) - end - - it 'returns false' do - expect(environment.includes_commit?(RepoHelpers.sample_commit)).to be false - end - end + it 'selects deployed environments' do + expect(environment.deployment_for(commit)).to eq deployment.id.to_s end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 76eb03b6acff..59099c46fcb5 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -701,29 +701,6 @@ end end - describe "#deployments" do - let(:project) { create(:project) } - let(:merge_request) { create(:merge_request, source_project: project) } - - it 'selects deployed environments' do - environments = create_list(:environment, 3, project: project) - create(:deployment, environment: environments.first, sha: project.commit('master').id) - create(:deployment, environment: environments.second, sha: project.commit('feature').id) - - expect(merge_request.environments).to eq [environments.first] - end - - context 'without a diff_head_commit' do - before do - expect(merge_request).to receive(:diff_head_commit).and_return(nil) - end - - it 'returns an empty array' do - expect(merge_request.environments).to be_empty - end - end - end - describe "#reload_diff" do let(:note) { create(:diff_note_on_merge_request, project: subject.project, noteable: subject) } diff --git a/spec/services/create_deployment_service_spec.rb b/spec/services/create_deployment_service_spec.rb index 41b897f36cd1..39a160e1010e 100644 --- a/spec/services/create_deployment_service_spec.rb +++ b/spec/services/create_deployment_service_spec.rb @@ -1,7 +1,7 @@ require 'spec_helper' describe CreateDeploymentService, services: true do - let(:project) { create(:empty_project) } + let(:project) { create(:project) } let(:user) { create(:user) } let(:service) { described_class.new(project, user, params) } @@ -88,7 +88,7 @@ describe 'processing of builds' do let(:environment) { nil } - + shared_examples 'does not create environment and deployment' do it 'does not create a new environment' do expect { subject }.not_to change { Environment.count } @@ -133,12 +133,12 @@ context 'without environment specified' do let(:build) { create(:ci_build, project: project) } - + it_behaves_like 'does not create environment and deployment' do subject { build.success } end end - + context 'when environment is specified' do let(:pipeline) { create(:ci_pipeline, project: project) } let(:build) { create(:ci_build, pipeline: pipeline, environment: 'production', options: options) } @@ -168,5 +168,6 @@ end end end + end end -- GitLab From 5f8d0fe4f4af81706ed275b88f0c06eea86668ca Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Fri, 9 Sep 2016 15:29:03 +0200 Subject: [PATCH 03/20] Incorporate review, groundwork async loading --- .../projects/merge_requests_controller.rb | 13 ++++++++- app/models/deployment.rb | 2 ++ app/models/merge_request.rb | 2 +- app/models/repository.rb | 4 +-- app/services/create_deployment_service.rb | 5 +--- config/routes.rb | 1 + .../merge_requests_controller_spec.rb | 16 ++++++++++ .../create_deployment_service_spec.rb | 29 ------------------- 8 files changed, 34 insertions(+), 38 deletions(-) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index e972376df4c0..79bfe1d28ade 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -10,7 +10,8 @@ class Projects::MergeRequestsController < Projects::ApplicationController before_action :module_enabled before_action :merge_request, only: [ :edit, :update, :show, :diffs, :commits, :conflicts, :builds, :pipelines, :merge, :merge_check, - :ci_status, :toggle_subscription, :cancel_merge_when_build_succeeds, :remove_wip, :resolve_conflicts + :ci_status, :toggle_subscription, :cancel_merge_when_build_succeeds, :remove_wip, :resolve_conflicts, + :deployments ] before_action :validates_merge_request, only: [:show, :diffs, :commits, :builds, :pipelines] before_action :define_show_vars, only: [:show, :diffs, :commits, :conflicts, :builds, :pipelines] @@ -210,6 +211,16 @@ def pipelines end end + def deployments + deployments = @merge_request.deployments + + deployments.map do |deployment| + { environment_name: deployment.environment.name, created_at: deployment.created_at } + end + + render json: deployments.to_json + end + def new apply_diff_view_cookie! diff --git a/app/models/deployment.rb b/app/models/deployment.rb index a9e494b29a93..3ff98e477cb7 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -11,6 +11,8 @@ class Deployment < ActiveRecord::Base delegate :name, to: :environment, prefix: true + after_save :create_refs + def last? self == environment.last_deployment end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 330557af5f29..1415524a98ff 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -659,7 +659,7 @@ def deployments environment.deployment_for(diff_head_commit) end.compact - deployment_ids.empty? ? nil : target_project.deployments.find(deployment_ids) + target_project.deployments.find(deployment_ids) end def state_human_name diff --git a/app/models/repository.rb b/app/models/repository.rb index f337e75f51f7..0f1ee74432ba 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -724,10 +724,8 @@ def deployment_ref_for_sha(environment_ref_path, sha) # Found -> ["b8d95eb4969eefacb0a58f6a28f6803f8070e7b9 commit\trefs/environments/production/77\n", 0] ref = Gitlab::Popen.popen(args, path_to_repo).first - return nil if ref.empty? - # Parse " commit\trefs/environments//\n" to ID - ref.split('/').last.rstrip! + ref.rstrip.split('/').last end def refs_contains_sha(ref_type, sha) diff --git a/app/services/create_deployment_service.rb b/app/services/create_deployment_service.rb index 29278a8bb569..e6667132e275 100644 --- a/app/services/create_deployment_service.rb +++ b/app/services/create_deployment_service.rb @@ -4,7 +4,7 @@ class CreateDeploymentService < BaseService def execute(deployable = nil) environment = find_or_create_environment - deployment = project.deployments.create( + project.deployments.create( environment: environment, ref: params[:ref], tag: params[:tag], @@ -12,9 +12,6 @@ def execute(deployable = nil) user: current_user, deployable: deployable ) - deployment.create_refs - - deployment end private diff --git a/config/routes.rb b/config/routes.rb index 068c92d1400e..769ac702e0b5 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -740,6 +740,7 @@ post :remove_wip get :diff_for_path post :resolve_conflicts + get :deployments end collection do diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 94c9edc91fe2..2f210f9bbd13 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -694,4 +694,20 @@ def resolve_conflicts(sections) end end end + + describe 'GET deployments' do + let(:json_response) { JSON.parse(response.body) } + + before do + get :deployments, namespace_id: project.namespace.to_param, project_id: project.to_param, + id: merge_request.iid + end + + context 'when there are no deployments' do + it 'returns an empty Array' do + expect(json_response).to be_an Array + expect(json_response).to eq [] + end + end + end end diff --git a/spec/services/create_deployment_service_spec.rb b/spec/services/create_deployment_service_spec.rb index 39a160e1010e..34ae2c768f21 100644 --- a/spec/services/create_deployment_service_spec.rb +++ b/spec/services/create_deployment_service_spec.rb @@ -56,34 +56,6 @@ expect(subject).not_to be_persisted end end - - context 'when variables are used' do - let(:params) do - { environment: 'review-apps/$CI_BUILD_REF_NAME', - ref: 'master', - tag: false, - sha: '97de212e80737a608d939f648d959671fb0a0142', - options: { - name: 'review-apps/$CI_BUILD_REF_NAME', - url: 'http://$CI_BUILD_REF_NAME.review-apps.gitlab.com' - }, - variables: [ - { key: 'CI_BUILD_REF_NAME', value: 'feature-review-apps' } - ] - } - end - - it 'does create a new environment' do - expect { subject }.to change { Environment.count }.by(1) - - expect(subject.environment.name).to eq('review-apps/feature-review-apps') - expect(subject.environment.external_url).to eq('http://feature-review-apps.review-apps.gitlab.com') - end - - it 'does create a new deployment' do - expect(subject).to be_persisted - end - end end describe 'processing of builds' do @@ -168,6 +140,5 @@ end end end - end end -- GitLab From 8696d414ae6e9119fa502ce5b7a06d41c0f7a05c Mon Sep 17 00:00:00 2001 From: Luke Bennett Date: Sat, 10 Sep 2016 21:33:21 +0100 Subject: [PATCH 04/20] Merged deployments endpoint to ci_status endpoint --- .../projects/merge_requests_controller.rb | 23 ++++++++----------- config/routes.rb | 1 - .../merge_requests_controller_spec.rb | 16 ------------- 3 files changed, 10 insertions(+), 30 deletions(-) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 79bfe1d28ade..5bf28de7fb1b 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -10,8 +10,7 @@ class Projects::MergeRequestsController < Projects::ApplicationController before_action :module_enabled before_action :merge_request, only: [ :edit, :update, :show, :diffs, :commits, :conflicts, :builds, :pipelines, :merge, :merge_check, - :ci_status, :toggle_subscription, :cancel_merge_when_build_succeeds, :remove_wip, :resolve_conflicts, - :deployments + :ci_status, :toggle_subscription, :cancel_merge_when_build_succeeds, :remove_wip, :resolve_conflicts ] before_action :validates_merge_request, only: [:show, :diffs, :commits, :builds, :pipelines] before_action :define_show_vars, only: [:show, :diffs, :commits, :conflicts, :builds, :pipelines] @@ -211,16 +210,6 @@ def pipelines end end - def deployments - deployments = @merge_request.deployments - - deployments.map do |deployment| - { environment_name: deployment.environment.name, created_at: deployment.created_at } - end - - render json: deployments.to_json - end - def new apply_diff_view_cookie! @@ -387,11 +376,19 @@ def ci_status end end + deployments = @merge_request.deployments + if deployments + deployments.map do |deployment| + { environment_name: deployment.environment.name, created_at: deployment.created_at } + end + end + response = { title: merge_request.title, sha: merge_request.diff_head_commit.short_id, status: status, - coverage: coverage + coverage: coverage, + deployments: deployments } render json: response diff --git a/config/routes.rb b/config/routes.rb index 769ac702e0b5..068c92d1400e 100644 --- a/config/routes.rb +++ b/config/routes.rb @@ -740,7 +740,6 @@ post :remove_wip get :diff_for_path post :resolve_conflicts - get :deployments end collection do diff --git a/spec/controllers/projects/merge_requests_controller_spec.rb b/spec/controllers/projects/merge_requests_controller_spec.rb index 2f210f9bbd13..94c9edc91fe2 100644 --- a/spec/controllers/projects/merge_requests_controller_spec.rb +++ b/spec/controllers/projects/merge_requests_controller_spec.rb @@ -694,20 +694,4 @@ def resolve_conflicts(sections) end end end - - describe 'GET deployments' do - let(:json_response) { JSON.parse(response.body) } - - before do - get :deployments, namespace_id: project.namespace.to_param, project_id: project.to_param, - id: merge_request.iid - end - - context 'when there are no deployments' do - it 'returns an empty Array' do - expect(json_response).to be_an Array - expect(json_response).to eq [] - end - end - end end -- GitLab From 21a5322a2ac53d64d300774d1e91f6128bed5e33 Mon Sep 17 00:00:00 2001 From: Luke Bennett Date: Sat, 10 Sep 2016 21:59:19 +0100 Subject: [PATCH 05/20] Added deployments JS logic --- .../javascripts/merge_request_widget.js | 28 ++++++++++++++++++- .../stylesheets/pages/merge_requests.scss | 4 +++ .../projects/merge_requests_controller.rb | 14 ++++++++-- .../merge_requests/widget/_heading.html.haml | 20 ++++++------- 4 files changed, 52 insertions(+), 14 deletions(-) diff --git a/app/assets/javascripts/merge_request_widget.js b/app/assets/javascripts/merge_request_widget.js index 7bbcdf598388..86e3868de16f 100644 --- a/app/assets/javascripts/merge_request_widget.js +++ b/app/assets/javascripts/merge_request_widget.js @@ -1,6 +1,8 @@ (function() { var indexOf = [].indexOf || function(item) { for (var i = 0, l = this.length; i < l; i++) { if (i in this && this[i] === item) return i; } return -1; }; + var DEPLOYMENT_TEMPLATE; + this.MergeRequestWidget = (function() { function MergeRequestWidget(opts) { // Initialize MergeRequestWidget behavior @@ -19,6 +21,7 @@ clearInterval(this.fetchBuildStatusInterval); this.clearEventListeners(); this.addEventListeners(); + this.initDeploymentStatus(); this.getCIStatus(false); this.pollCIStatus(); notifyPermissions(); @@ -48,6 +51,13 @@ })(this)); }; + MergeRequestWidget.prototype.initDeploymentStatus = function() { + var $deploymentStatus = $('.js-deployment-status'); + DEPLOYMENT_TEMPLATE = $deploymentStatus.html(); + $deploymentStatus.remove(); + this.$widgetBody = $('.mr-widget-body'); + }; + MergeRequestWidget.prototype.mergeInProgress = function(deleteSourceBranch) { if (deleteSourceBranch == null) { deleteSourceBranch = false; @@ -62,7 +72,7 @@ urlSuffix = deleteSourceBranch ? '?deleted_source_branch=true' : ''; return window.location.href = window.location.pathname + urlSuffix; } else if (data.merge_error) { - return $('.mr-widget-body').html("

" + data.merge_error + "

"); + return this.$widgetBody.html("

" + data.merge_error + "

"); } else { callback = function() { return merge_request_widget.mergeInProgress(deleteSourceBranch); @@ -118,6 +128,7 @@ if (data.status === '') { return; } + if (data.deployments && data.deployments.length) _this.renderDeployments(data.deployments); if (_this.firstCICheck || data.status !== _this.opts.ci_status && (data.status != null)) { _this.opts.ci_status = data.status; _this.showCIStatus(data.status); @@ -150,6 +161,21 @@ })(this)); }; + MergeRequestWidget.prototype.renderDeployments = function(deployments) { + for (var i = 0; i < deployments.length; i++) { + var deployment = deployments[i]; + if ($('.mr-state-widget #' + deployment.environment_id).length) return; + var $template = $(DEPLOYMENT_TEMPLATE); + if (!deployment.external_url) $('.js-deployment-link', $template).remove(); + deployment.created_at = $.timeago(deployment.created_at); + var templateString = $template[0].outerHTML + .replace(/{{/g, '<%-') + .replace(/}}/g, '%>'); + var template = _.template(templateString)(deployment) + this.$widgetBody.before(template); + } + }; + MergeRequestWidget.prototype.showCIStatus = function(state) { var allowed_states; if (state == null) { diff --git a/app/assets/stylesheets/pages/merge_requests.scss b/app/assets/stylesheets/pages/merge_requests.scss index 926247e5e87b..10b2d5e10414 100644 --- a/app/assets/stylesheets/pages/merge_requests.scss +++ b/app/assets/stylesheets/pages/merge_requests.scss @@ -120,6 +120,10 @@ color: #5c5d5e; } + .js-deployment-link { + display: inline-block; + } + .mr-widget-body { h4 { font-weight: 600; diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 5bf28de7fb1b..b839d94d46da 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -377,9 +377,19 @@ def ci_status end deployments = @merge_request.deployments + if deployments - deployments.map do |deployment| - { environment_name: deployment.environment.name, created_at: deployment.created_at } + deployments = deployments.map do |deployment| + deployment = { + environment_name: deployment.environment.name, + environment_id: deployment.environment.id, + external_url: deployment.environment.external_url, + created_at: deployment.created_at + } + if deployment[:external_url] + deployment[:external_url_formatted] = deployment[:external_url].gsub(/\A.*?:\/\//, '') + end + deployment end end diff --git a/app/views/projects/merge_requests/widget/_heading.html.haml b/app/views/projects/merge_requests/widget/_heading.html.haml index 464b7e54dc7c..4e7f987ff9dc 100644 --- a/app/views/projects/merge_requests/widget/_heading.html.haml +++ b/app/views/projects/merge_requests/widget/_heading.html.haml @@ -43,17 +43,15 @@ = icon("times-circle") Could not connect to the CI server. Please check your settings and try again. -- @merge_request.deployments.each do |deployment| - - environment = deployment.environment - .mr-widget-heading +-# JavaScript template +.js-deployment-status.hidden + .mr-widget-heading{id: '{{ environment_id }}'} .ci_widget.ci-success - = ci_icon_for_status("success") - %span.hidden-sm + = ci_icon_for_status('success') + %span Deployed to = succeed '.' do - = link_to environment.name, namespace_project_environment_path(@project.namespace, @project, environment), class: 'environment' - = time_ago_with_tooltip(deployment.created_at) - - external_url = environment.external_url - - if external_url - = link_to external_url, target: '_blank' do - = icon('external-link', text: "View on #{external_url.gsub(/\A.*?:\/\//, '')}", right: true) + = link_to '{{ environment_name }}', "#{@project.namespace.path}/#{@project.path}/environments/{{ environment_id }}", class: 'environment', data: { toggle: 'tooltip', placement: 'top', title: '{{ created_at }}' } + .js-deployment-link + = link_to '{{ external_url }}', target: '_blank' do + = icon('external-link', text: 'View on {{ external_url_formatted }}', right: true) -- GitLab From d701c0cef3da45db90a22c4bc9a47e220cce355d Mon Sep 17 00:00:00 2001 From: Luke Bennett Date: Mon, 12 Sep 2016 14:05:50 +0100 Subject: [PATCH 06/20] Added deployments jasmine spec Removed view partial spec and added feature spec --- .../widget_deployments_spec.rb} | 16 ++++++-------- spec/javascripts/merge_request_widget_spec.js | 22 +++++++++++++++++-- 2 files changed, 27 insertions(+), 11 deletions(-) rename spec/{views/projects/merge_requests/_heading.html.haml_spec.rb => features/merge_requests/widget_deployments_spec.rb} (55%) diff --git a/spec/views/projects/merge_requests/_heading.html.haml_spec.rb b/spec/features/merge_requests/widget_deployments_spec.rb similarity index 55% rename from spec/views/projects/merge_requests/_heading.html.haml_spec.rb rename to spec/features/merge_requests/widget_deployments_spec.rb index 733b2dfa7ffe..53c9e2dec1aa 100644 --- a/spec/views/projects/merge_requests/_heading.html.haml_spec.rb +++ b/spec/features/merge_requests/widget_deployments_spec.rb @@ -1,9 +1,9 @@ require 'spec_helper' -describe 'projects/merge_requests/widget/_heading' do - include Devise::TestHelpers +feature 'Widget Deployments Header', feature: true, js: true do + include WaitForAjax - context 'when released to an environment' do + describe 'when deployed to an environment' do let(:project) { merge_request.target_project } let(:merge_request) { create(:merge_request, :merged) } let(:environment) { create(:environment, project: project) } @@ -12,15 +12,13 @@ end before do - assign(:merge_request, merge_request) - assign(:project, project) - - render + login_as :admin + visit namespace_project_merge_request_path(project.namespace, project, merge_request) end it 'displays that the environment is deployed' do - expect(rendered).to match("Deployed to") - expect(rendered).to match("#{environment.name}") + wait_for_ajax + expect(page).to have_content("Deployed to #{environment.name}.") end end end diff --git a/spec/javascripts/merge_request_widget_spec.js b/spec/javascripts/merge_request_widget_spec.js index 17b32914ec39..b8dfb51f4f3a 100644 --- a/spec/javascripts/merge_request_widget_spec.js +++ b/spec/javascripts/merge_request_widget_spec.js @@ -1,5 +1,6 @@ /*= require merge_request_widget */ +/*= require lib/utils/jquery.timeago.js */ (function() { describe('MergeRequestWidget', function() { @@ -30,7 +31,7 @@ }); return describe('getCIStatus', function() { beforeEach(function() { - return spyOn(jQuery, 'getJSON').and.callFake((function(_this) { + spyOn(jQuery, 'getJSON').and.callFake((function(_this) { return function(req, cb) { return cb(_this.ciStatusData); }; @@ -61,13 +62,30 @@ this["class"].getCIStatus(false); return expect(spy).not.toHaveBeenCalled(); }); - return it('should not display a notification on the first check after the widget has been created', function() { + it('should not display a notification on the first check after the widget has been created', function() { var spy; spy = spyOn(window, 'notify'); this["class"] = new MergeRequestWidget(this.opts); this["class"].getCIStatus(true); return expect(spy).not.toHaveBeenCalled(); }); + it('should call renderDeployments when the deployments property is set', function() { + this.ciStatusData.deployments = [{ + created_at: '2016-09-12T13:38:30.636Z', + environment_id: 1, + environment_name: 'env1', + external_url: 'https://test-url.com', + external_url_formatted: 'test-url.com' + }]; + var spy = spyOn(this['class'], 'renderDeployments').and.stub(); + this['class'].getCIStatus(false); + expect(spy).toHaveBeenCalledWith(this.ciStatusData.deployments); + }); + it('should not call renderDeployments when the deployments property is not set', function() { + var spy = spyOn(this['class'], 'renderDeployments').and.stub(); + this['class'].getCIStatus(false); + expect(spy).not.toHaveBeenCalled(); + }); }); }); -- GitLab From 34f367d3c52f011d319e486f651908d9dffa108d Mon Sep 17 00:00:00 2001 From: Luke Bennett Date: Mon, 12 Sep 2016 19:24:04 +0100 Subject: [PATCH 07/20] Moved timeago string inline with main deployment message and added an accurate datetime tooltip --- app/controllers/projects/merge_requests_controller.rb | 3 ++- app/views/projects/merge_requests/widget/_heading.html.haml | 5 +++-- 2 files changed, 5 insertions(+), 3 deletions(-) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index b839d94d46da..366f6624201a 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -384,7 +384,8 @@ def ci_status environment_name: deployment.environment.name, environment_id: deployment.environment.id, external_url: deployment.environment.external_url, - created_at: deployment.created_at + created_at: deployment.created_at, + created_at_formatted: deployment.created_at.to_time.in_time_zone.to_s(:medium) } if deployment[:external_url] deployment[:external_url_formatted] = deployment[:external_url].gsub(/\A.*?:\/\//, '') diff --git a/app/views/projects/merge_requests/widget/_heading.html.haml b/app/views/projects/merge_requests/widget/_heading.html.haml index 4e7f987ff9dc..cf15c86c49ec 100644 --- a/app/views/projects/merge_requests/widget/_heading.html.haml +++ b/app/views/projects/merge_requests/widget/_heading.html.haml @@ -50,8 +50,9 @@ = ci_icon_for_status('success') %span Deployed to - = succeed '.' do - = link_to '{{ environment_name }}', "#{@project.namespace.path}/#{@project.path}/environments/{{ environment_id }}", class: 'environment', data: { toggle: 'tooltip', placement: 'top', title: '{{ created_at }}' } + = link_to '{{ environment_name }}', "#{@project.namespace.path}/#{@project.path}/environments/{{ environment_id }}", class: 'environment' + %span{ data: { toggle: 'tooltip', placement: 'top', title: '{{ created_at_formatted }}' } } + {{ created_at }}. .js-deployment-link = link_to '{{ external_url }}', target: '_blank' do = icon('external-link', text: 'View on {{ external_url_formatted }}', right: true) -- GitLab From 2cb4b3f42d9809dd273d22f7cc2ee8eae7a911bc Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Wed, 14 Sep 2016 12:14:31 +0200 Subject: [PATCH 08/20] Fix test and styling --- app/controllers/projects/merge_requests_controller.rb | 2 ++ app/models/merge_request.rb | 6 +++--- spec/features/merge_requests/widget_deployments_spec.rb | 2 +- 3 files changed, 6 insertions(+), 4 deletions(-) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 366f6624201a..6727daaeacdf 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -387,9 +387,11 @@ def ci_status created_at: deployment.created_at, created_at_formatted: deployment.created_at.to_time.in_time_zone.to_s(:medium) } + if deployment[:external_url] deployment[:external_url_formatted] = deployment[:external_url].gsub(/\A.*?:\/\//, '') end + deployment end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 1415524a98ff..0d561b6d0801 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -655,9 +655,9 @@ def deployments return nil unless diff_head_commit deployment_ids = - target_project.environments.map do |environment| - environment.deployment_for(diff_head_commit) - end.compact + target_project.environments. + map { |environment| environment.deployment_for(diff_head_commit) }. + compact target_project.deployments.find(deployment_ids) end diff --git a/spec/features/merge_requests/widget_deployments_spec.rb b/spec/features/merge_requests/widget_deployments_spec.rb index 53c9e2dec1aa..cafa74fd05d7 100644 --- a/spec/features/merge_requests/widget_deployments_spec.rb +++ b/spec/features/merge_requests/widget_deployments_spec.rb @@ -18,7 +18,7 @@ it 'displays that the environment is deployed' do wait_for_ajax - expect(page).to have_content("Deployed to #{environment.name}.") + expect(page).to have_content("Deployed to #{environment.name}") end end end -- GitLab From 2a41214a76031dcb7176b43063f7ea38a288b32b Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Wed, 14 Sep 2016 22:34:05 +0200 Subject: [PATCH 09/20] Add coverage and incorporate feedback --- app/models/deployment.rb | 15 ++++----------- app/models/environment.rb | 4 ++-- app/models/merge_request.rb | 8 +++++--- app/models/repository.rb | 4 ++-- spec/models/environment_spec.rb | 18 +++++++++--------- spec/models/merge_request_spec.rb | 24 ++++++++++++++++++++++++ spec/models/repository_spec.rb | 22 ++++++++++++++++++++++ 7 files changed, 68 insertions(+), 27 deletions(-) diff --git a/app/models/deployment.rb b/app/models/deployment.rb index 3ff98e477cb7..57386404f0c1 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -11,7 +11,7 @@ class Deployment < ActiveRecord::Base delegate :name, to: :environment, prefix: true - after_save :create_refs + after_save :create_ref def last? self == environment.last_deployment @@ -29,11 +29,8 @@ def commit_title commit.try(:title) end - def create_refs - path = project.repository.path_to_repo - - project.repository.fetch_ref(path, ref, ref_path) - project.repository.fetch_ref(path, ref, ref_path_head) + def create_ref + project.repository.fetch_ref(project.repository.path_to_repo, ref, ref_path) end def manual_actions @@ -43,10 +40,6 @@ def manual_actions private def ref_path - environment.ref_path + id.to_s - end - - def ref_path_head - environment.ref_path + 'HEAD' + "#{environment.ref_path}#{id}" end end diff --git a/app/models/environment.rb b/app/models/environment.rb index 37787ec9ad40..075f544f9293 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -38,8 +38,8 @@ def set_environment_type end end - def deployment_for(commit) - project.repository.deployment_ref_for_sha(ref_path, commit.sha) + def deployment_id_for(commit) + project.repository.ref_name_for_sha(ref_path, commit.sha) end def ref_path diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 0d561b6d0801..1cd44e25f406 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -655,9 +655,11 @@ def deployments return nil unless diff_head_commit deployment_ids = - target_project.environments. - map { |environment| environment.deployment_for(diff_head_commit) }. - compact + target_project.environments.reduce([]) do |array, environment| + id = environment.deployment_id_for(diff_head_commit) + + array.push(id) unless id == 0 + end target_project.deployments.find(deployment_ids) end diff --git a/app/models/repository.rb b/app/models/repository.rb index 0f1ee74432ba..f9a94c62760c 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -717,7 +717,7 @@ def contributors end end - def deployment_ref_for_sha(environment_ref_path, sha) + def ref_name_for_sha(environment_ref_path, sha) args = %W(#{Gitlab.config.git.bin_path} for-each-ref --count=1 #{environment_ref_path} --contains #{sha}) # Not found -> ["", 0] @@ -725,7 +725,7 @@ def deployment_ref_for_sha(environment_ref_path, sha) ref = Gitlab::Popen.popen(args, path_to_repo).first # Parse " commit\trefs/environments//\n" to ID - ref.rstrip.split('/').last + ref.rstrip.split('/').last.to_i end def refs_contains_sha(ref_type, sha) diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index c699d8b85c70..2e4ec500b93b 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -31,20 +31,20 @@ end end - describe '#deployment_for' do + describe '#deployment_id_for' do let(:project) { create(:project) } let!(:environment) { create(:environment, project: project) } - let(:deployment) { create(:deployment, environment: environment) } - let(:deployment1) { create(:deployment, environment: environment, sha: commit.id) } - let(:commit) { project.commit } + let!(:deployment) { create(:deployment, environment: environment, ref: commit.parent.id) } + let!(:deployment1) { create(:deployment, environment: environment, ref: commit.id) } + let(:head_commit) { project.commit } + let(:commit) { project.commit.parent } - before do - deployment.create_refs - deployment1.create_refs + it 'selects deployment id for the environment' do + expect(environment.deployment_id_for(commit)).to eq deployment1.id end - it 'selects deployed environments' do - expect(environment.deployment_for(commit)).to eq deployment.id.to_s + it 'return 0 when no deployment is found' do + expect(environment.deployment_id_for(head_commit)).to eq 0 end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 59099c46fcb5..02a9c1a58e7b 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -701,6 +701,30 @@ end end + describe '#deployments' do + let(:project) { create(:project) } + let!(:environment) { create(:environment, project: project) } + let!(:deployment) { create(:deployment, environment: environment, ref: head_commit.id) } + let(:head_commit) { project.commit('feature') } + + let(:merge_request) { build(:merge_request, :simple, source_project: project) } + + context 'with multiple environments' do + let!(:environment1) { create(:environment, project: project) } + let!(:deployment1) { create(:deployment, environment: environment1, ref: head_commit.id) } + + it 'returns all deployments' do + expect(merge_request.deployments).to eq [deployment, deployment1] + end + end + + it 'when there is no deployment' do + merge_request = build(:merge_request, source_project: project) + + expect(merge_request.deployments).to eq [] + end + end + describe "#reload_diff" do let(:note) { create(:diff_note_on_merge_request, project: subject.project, noteable: subject) } diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index db29f4d353bd..a5082553b164 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -7,10 +7,12 @@ let(:project) { create(:project) } let(:repository) { project.repository } let(:user) { create(:user) } + let(:commit_options) do author = repository.user_to_committer(user) { message: 'Test message', committer: author, author: author } end + let(:merge_commit) do merge_request = create(:merge_request, source_branch: 'feature', target_branch: 'master', source_project: project) merge_commit_id = repository.merge(user, merge_request, commit_options) @@ -90,6 +92,26 @@ end end + describe '#ref_name_for_sha' do + context 'ref found' do + it 'returns the deployment id' do + allow_any_instance_of(Gitlab::Popen).to receive(:popen). + and_return(["b8d95eb4969eefacb0a58f6a28f6803f8070e7b9 commit\trefs/environments/production/77\n", 0]) + + expect(repository.ref_name_for_sha('bla', '0' * 40)).to eq 77 + end + end + + context 'ref not found' do + it 'returns 0' do + allow_any_instance_of(Gitlab::Popen).to receive(:popen). + and_return(["", 0]) + + expect(repository.ref_name_for_sha('bla', '0' * 40)).to eq 0 + end + end + end + describe '#last_commit_for_path' do subject { repository.last_commit_for_path(sample_commit.id, '.gitignore').id } -- GitLab From 07045137f1de63ab93c66c0fa4aae7a9b82a1eca Mon Sep 17 00:00:00 2001 From: Luke Bennett Date: Thu, 15 Sep 2016 16:36:40 +0100 Subject: [PATCH 10/20] Added assertion for created_at_formatted Moved url to JSON response and used npath helper Decode underscore template syntax tokens --- app/assets/javascripts/merge_request_widget.js | 4 +--- .../projects/merge_requests_controller.rb | 3 ++- .../merge_requests/widget/_heading.html.haml | 12 ++++++------ .../merge_requests/widget_deployments_spec.rb | 1 + 4 files changed, 10 insertions(+), 10 deletions(-) diff --git a/app/assets/javascripts/merge_request_widget.js b/app/assets/javascripts/merge_request_widget.js index 86e3868de16f..1a76ff1d6ebb 100644 --- a/app/assets/javascripts/merge_request_widget.js +++ b/app/assets/javascripts/merge_request_widget.js @@ -168,9 +168,7 @@ var $template = $(DEPLOYMENT_TEMPLATE); if (!deployment.external_url) $('.js-deployment-link', $template).remove(); deployment.created_at = $.timeago(deployment.created_at); - var templateString = $template[0].outerHTML - .replace(/{{/g, '<%-') - .replace(/}}/g, '%>'); + var templateString = _.unescape($template[0].outerHTML); var template = _.template(templateString)(deployment) this.$widgetBody.before(template); } diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 6727daaeacdf..7b1b8d7454a0 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -383,6 +383,7 @@ def ci_status deployment = { environment_name: deployment.environment.name, environment_id: deployment.environment.id, + environment_url: namespace_project_environment_path(@project.namespace, @project, deployment.environment), external_url: deployment.environment.external_url, created_at: deployment.created_at, created_at_formatted: deployment.created_at.to_time.in_time_zone.to_s(:medium) @@ -391,7 +392,7 @@ def ci_status if deployment[:external_url] deployment[:external_url_formatted] = deployment[:external_url].gsub(/\A.*?:\/\//, '') end - + deployment end end diff --git a/app/views/projects/merge_requests/widget/_heading.html.haml b/app/views/projects/merge_requests/widget/_heading.html.haml index cf15c86c49ec..212f1120c95e 100644 --- a/app/views/projects/merge_requests/widget/_heading.html.haml +++ b/app/views/projects/merge_requests/widget/_heading.html.haml @@ -45,14 +45,14 @@ -# JavaScript template .js-deployment-status.hidden - .mr-widget-heading{id: '{{ environment_id }}'} + .mr-widget-heading{id: '<%- environment_id %>'} .ci_widget.ci-success = ci_icon_for_status('success') %span Deployed to - = link_to '{{ environment_name }}', "#{@project.namespace.path}/#{@project.path}/environments/{{ environment_id }}", class: 'environment' - %span{ data: { toggle: 'tooltip', placement: 'top', title: '{{ created_at_formatted }}' } } - {{ created_at }}. + = link_to '<%- environment_name %>', '<%- environment_url %>', class: 'environment' + %span{ data: { toggle: 'tooltip', placement: 'top', title: '<%- created_at_formatted %>' } } + <%- created_at %>. .js-deployment-link - = link_to '{{ external_url }}', target: '_blank' do - = icon('external-link', text: 'View on {{ external_url_formatted }}', right: true) + = link_to '<%- external_url %>', target: '_blank' do + = icon('external-link', text: 'View on <%- external_url_formatted %>', right: true) diff --git a/spec/features/merge_requests/widget_deployments_spec.rb b/spec/features/merge_requests/widget_deployments_spec.rb index cafa74fd05d7..cc96293968b8 100644 --- a/spec/features/merge_requests/widget_deployments_spec.rb +++ b/spec/features/merge_requests/widget_deployments_spec.rb @@ -19,6 +19,7 @@ it 'displays that the environment is deployed' do wait_for_ajax expect(page).to have_content("Deployed to #{environment.name}") + expect(find('.ci_widget > span > span')['data-title']).to eq(deployment.created_at.to_time.in_time_zone.to_s(:medium)) end end end -- GitLab From ee6e126f1423f90b23f41e606a938675c2c067d2 Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Tue, 20 Sep 2016 06:39:49 +0300 Subject: [PATCH 11/20] Only reference returned by the repository model --- app/models/environment.rb | 6 ++++- app/models/merge_request.rb | 8 +++---- app/models/repository.rb | 5 +--- .../merge_requests/widget_deployments_spec.rb | 3 ++- spec/models/environment_spec.rb | 6 ++--- spec/models/merge_request_spec.rb | 24 ------------------- spec/models/repository_spec.rb | 8 +++---- 7 files changed, 18 insertions(+), 42 deletions(-) diff --git a/app/models/environment.rb b/app/models/environment.rb index 075f544f9293..f95fb83efdc5 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -39,7 +39,11 @@ def set_environment_type end def deployment_id_for(commit) - project.repository.ref_name_for_sha(ref_path, commit.sha) + ref = project.repository.ref_name_for_sha(ref_path, commit.sha) + + return nil unless ref + + ref.split('/').last.to_i end def ref_path diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 1cd44e25f406..edc9c5c35a89 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -655,11 +655,9 @@ def deployments return nil unless diff_head_commit deployment_ids = - target_project.environments.reduce([]) do |array, environment| - id = environment.deployment_id_for(diff_head_commit) - - array.push(id) unless id == 0 - end + target_project.environments.map do |environment| + environment.deployment_id_for(diff_head_commit) + end.compact target_project.deployments.find(deployment_ids) end diff --git a/app/models/repository.rb b/app/models/repository.rb index f9a94c62760c..caa0324ece7e 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -722,10 +722,7 @@ def ref_name_for_sha(environment_ref_path, sha) # Not found -> ["", 0] # Found -> ["b8d95eb4969eefacb0a58f6a28f6803f8070e7b9 commit\trefs/environments/production/77\n", 0] - ref = Gitlab::Popen.popen(args, path_to_repo).first - - # Parse " commit\trefs/environments//\n" to ID - ref.rstrip.split('/').last.to_i + Gitlab::Popen.popen(args, path_to_repo).first.split.last end def refs_contains_sha(ref_type, sha) diff --git a/spec/features/merge_requests/widget_deployments_spec.rb b/spec/features/merge_requests/widget_deployments_spec.rb index cc96293968b8..ca71cbf69b38 100644 --- a/spec/features/merge_requests/widget_deployments_spec.rb +++ b/spec/features/merge_requests/widget_deployments_spec.rb @@ -18,7 +18,8 @@ it 'displays that the environment is deployed' do wait_for_ajax - expect(page).to have_content("Deployed to #{environment.name}") + + expect(page).to have_content("Deployed to #{environment.name} less than a minute ago") expect(find('.ci_widget > span > span')['data-title']).to eq(deployment.created_at.to_time.in_time_zone.to_s(:medium)) end end diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index 2e4ec500b93b..360a17579a6b 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -39,12 +39,12 @@ let(:head_commit) { project.commit } let(:commit) { project.commit.parent } - it 'selects deployment id for the environment' do + it 'returns deployment id for the environment' do expect(environment.deployment_id_for(commit)).to eq deployment1.id end - it 'return 0 when no deployment is found' do - expect(environment.deployment_id_for(head_commit)).to eq 0 + it 'return nil when no deployment is found' do + expect(environment.deployment_id_for(head_commit)).to eq nil end end diff --git a/spec/models/merge_request_spec.rb b/spec/models/merge_request_spec.rb index 02a9c1a58e7b..59099c46fcb5 100644 --- a/spec/models/merge_request_spec.rb +++ b/spec/models/merge_request_spec.rb @@ -701,30 +701,6 @@ end end - describe '#deployments' do - let(:project) { create(:project) } - let!(:environment) { create(:environment, project: project) } - let!(:deployment) { create(:deployment, environment: environment, ref: head_commit.id) } - let(:head_commit) { project.commit('feature') } - - let(:merge_request) { build(:merge_request, :simple, source_project: project) } - - context 'with multiple environments' do - let!(:environment1) { create(:environment, project: project) } - let!(:deployment1) { create(:deployment, environment: environment1, ref: head_commit.id) } - - it 'returns all deployments' do - expect(merge_request.deployments).to eq [deployment, deployment1] - end - end - - it 'when there is no deployment' do - merge_request = build(:merge_request, source_project: project) - - expect(merge_request.deployments).to eq [] - end - end - describe "#reload_diff" do let(:note) { create(:diff_note_on_merge_request, project: subject.project, noteable: subject) } diff --git a/spec/models/repository_spec.rb b/spec/models/repository_spec.rb index a5082553b164..65648e097231 100644 --- a/spec/models/repository_spec.rb +++ b/spec/models/repository_spec.rb @@ -94,20 +94,20 @@ describe '#ref_name_for_sha' do context 'ref found' do - it 'returns the deployment id' do + it 'returns the ref' do allow_any_instance_of(Gitlab::Popen).to receive(:popen). and_return(["b8d95eb4969eefacb0a58f6a28f6803f8070e7b9 commit\trefs/environments/production/77\n", 0]) - expect(repository.ref_name_for_sha('bla', '0' * 40)).to eq 77 + expect(repository.ref_name_for_sha('bla', '0' * 40)).to eq 'refs/environments/production/77' end end context 'ref not found' do - it 'returns 0' do + it 'returns nil' do allow_any_instance_of(Gitlab::Popen).to receive(:popen). and_return(["", 0]) - expect(repository.ref_name_for_sha('bla', '0' * 40)).to eq 0 + expect(repository.ref_name_for_sha('bla', '0' * 40)).to eq nil end end end -- GitLab From 15df8330b26b8c9f57f715c0424ad2b45af127f7 Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Tue, 20 Sep 2016 10:14:47 +0300 Subject: [PATCH 12/20] Explicitly install git to upgrade --- scripts/prepare_build.sh | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/scripts/prepare_build.sh b/scripts/prepare_build.sh index 76b2178c79c3..a2a4ed9e36d8 100755 --- a/scripts/prepare_build.sh +++ b/scripts/prepare_build.sh @@ -27,9 +27,15 @@ if [ -f /.dockerenv ] || [ -f ./dockerinit ]; then cp "$PHANTOMJS_FILE/bin/phantomjs" "/usr/bin/" popd + # Try to install packages retry 'apt-get update -yqqq; apt-get -o dir::cache::archives="vendor/apt" install -y -qq --force-yes \ - libicu-dev libkrb5-dev cmake nodejs postgresql-client mysql-client unzip' + libicu-dev libkrb5-dev cmake nodejs postgresql-client mysql-client unzip' + + apt-get install -qqy software-properties-common + # Add PPA for updating git, we need software-properties-common + add-apt-repository ppa:git-core/ppa + retry 'apt-get update -yqqq; apt-get -o dif::cache::archives="vendor/apt" install -yqq --force-yes git' cp config/database.yml.mysql config/database.yml sed -i 's/username:.*/username: root/g' config/database.yml -- GitLab From ff630fe81b8a4e49781ca33bdbe7a4ab56a2a9bc Mon Sep 17 00:00:00 2001 From: Luke Bennett Date: Thu, 15 Sep 2016 16:36:40 +0100 Subject: [PATCH 13/20] Added assertion for created_at_formatted Moved url to JSON response and used npath helper Decode underscore template syntax tokens Removed static template from the merge request widget partial and moved full rendering to the clientside, waiting on solution to svg icon troubles --- ..._widget.js => merge_request_widget.js.es6} | 48 +++++++++++++------ .../merge_requests/widget/_heading.html.haml | 15 +----- .../merge_requests/widget_deployments_spec.rb | 3 +- 3 files changed, 37 insertions(+), 29 deletions(-) rename app/assets/javascripts/{merge_request_widget.js => merge_request_widget.js.es6} (83%) diff --git a/app/assets/javascripts/merge_request_widget.js b/app/assets/javascripts/merge_request_widget.js.es6 similarity index 83% rename from app/assets/javascripts/merge_request_widget.js rename to app/assets/javascripts/merge_request_widget.js.es6 index 1a76ff1d6ebb..dd07dc7e5d09 100644 --- a/app/assets/javascripts/merge_request_widget.js +++ b/app/assets/javascripts/merge_request_widget.js.es6 @@ -1,7 +1,26 @@ (function() { var indexOf = [].indexOf || function(item) { for (var i = 0, l = this.length; i < l; i++) { if (i in this && this[i] === item) return i; } return -1; }; - var DEPLOYMENT_TEMPLATE; + const DEPLOYMENT_TEMPLATE = `
+
+ <%= ci_success_icon %> + + Deployed to + + <%- environment_name %> + + + <%- created_at %>. + +
+ + + View on <%- external_url_formatted %> + +
+ +
+
`; this.MergeRequestWidget = (function() { function MergeRequestWidget(opts) { @@ -12,6 +31,7 @@ // ci_status_url - String, URL to use to check CI status // this.opts = opts; + this.$widgetBody = $('.mr-widget-body'); $('#modal_merge_info').modal({ show: false }); @@ -21,7 +41,7 @@ clearInterval(this.fetchBuildStatusInterval); this.clearEventListeners(); this.addEventListeners(); - this.initDeploymentStatus(); + this.retrieveSuccessIcon(); this.getCIStatus(false); this.pollCIStatus(); notifyPermissions(); @@ -51,12 +71,11 @@ })(this)); }; - MergeRequestWidget.prototype.initDeploymentStatus = function() { - var $deploymentStatus = $('.js-deployment-status'); - DEPLOYMENT_TEMPLATE = $deploymentStatus.html(); - $deploymentStatus.remove(); - this.$widgetBody = $('.mr-widget-body'); - }; + MergeRequestWidget.prototype.retrieveSuccessIcon = function() { + const $ciSuccessIcon = $('.js-success-icon'); + this.$ciSuccessIcon = $ciSuccessIcon.html(); + $ciSuccessIcon.remove(); + } MergeRequestWidget.prototype.mergeInProgress = function(deleteSourceBranch) { if (deleteSourceBranch == null) { @@ -162,14 +181,15 @@ }; MergeRequestWidget.prototype.renderDeployments = function(deployments) { - for (var i = 0; i < deployments.length; i++) { - var deployment = deployments[i]; - if ($('.mr-state-widget #' + deployment.environment_id).length) return; - var $template = $(DEPLOYMENT_TEMPLATE); + for (let i = 0; i < deployments.length; i++) { + const deployment = deployments[i]; + if ($(`.mr-state-widget #${ deployment.environment_id }`).length) return; + const $template = $(DEPLOYMENT_TEMPLATE); if (!deployment.external_url) $('.js-deployment-link', $template).remove(); deployment.created_at = $.timeago(deployment.created_at); - var templateString = _.unescape($template[0].outerHTML); - var template = _.template(templateString)(deployment) + deployment.ci_success_icon = this.$ciSuccessIcon; + const templateString = _.unescape($template[0].outerHTML); + const template = _.template(templateString)(deployment) this.$widgetBody.before(template); } }; diff --git a/app/views/projects/merge_requests/widget/_heading.html.haml b/app/views/projects/merge_requests/widget/_heading.html.haml index 212f1120c95e..4e7ece9c269b 100644 --- a/app/views/projects/merge_requests/widget/_heading.html.haml +++ b/app/views/projects/merge_requests/widget/_heading.html.haml @@ -43,16 +43,5 @@ = icon("times-circle") Could not connect to the CI server. Please check your settings and try again. --# JavaScript template -.js-deployment-status.hidden - .mr-widget-heading{id: '<%- environment_id %>'} - .ci_widget.ci-success - = ci_icon_for_status('success') - %span - Deployed to - = link_to '<%- environment_name %>', '<%- environment_url %>', class: 'environment' - %span{ data: { toggle: 'tooltip', placement: 'top', title: '<%- created_at_formatted %>' } } - <%- created_at %>. - .js-deployment-link - = link_to '<%- external_url %>', target: '_blank' do - = icon('external-link', text: 'View on <%- external_url_formatted %>', right: true) +.js-success-icon.hidden + = ci_icon_for_status('success') diff --git a/spec/features/merge_requests/widget_deployments_spec.rb b/spec/features/merge_requests/widget_deployments_spec.rb index ca71cbf69b38..cc96293968b8 100644 --- a/spec/features/merge_requests/widget_deployments_spec.rb +++ b/spec/features/merge_requests/widget_deployments_spec.rb @@ -18,8 +18,7 @@ it 'displays that the environment is deployed' do wait_for_ajax - - expect(page).to have_content("Deployed to #{environment.name} less than a minute ago") + expect(page).to have_content("Deployed to #{environment.name}") expect(find('.ci_widget > span > span')['data-title']).to eq(deployment.created_at.to_time.in_time_zone.to_s(:medium)) end end -- GitLab From be03ff6fbb810d7c1beed183ca5112f02bb57f75 Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Tue, 20 Sep 2016 19:12:45 +0300 Subject: [PATCH 14/20] Print Gt version in build log --- scripts/prepare_build.sh | 2 ++ 1 file changed, 2 insertions(+) diff --git a/scripts/prepare_build.sh b/scripts/prepare_build.sh index a2a4ed9e36d8..4c5f156f49ef 100755 --- a/scripts/prepare_build.sh +++ b/scripts/prepare_build.sh @@ -37,6 +37,8 @@ if [ -f /.dockerenv ] || [ -f ./dockerinit ]; then add-apt-repository ppa:git-core/ppa retry 'apt-get update -yqqq; apt-get -o dif::cache::archives="vendor/apt" install -yqq --force-yes git' + git --version + cp config/database.yml.mysql config/database.yml sed -i 's/username:.*/username: root/g' config/database.yml sed -i 's/password:.*/password:/g' config/database.yml -- GitLab From d0532a220fe70ed5314440acca89ed7675e990a7 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Tue, 20 Sep 2016 19:26:12 +0200 Subject: [PATCH 15/20] Update how we install git package --- scripts/prepare_build.sh | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/scripts/prepare_build.sh b/scripts/prepare_build.sh index 4c5f156f49ef..1d7913666c98 100755 --- a/scripts/prepare_build.sh +++ b/scripts/prepare_build.sh @@ -32,11 +32,11 @@ if [ -f /.dockerenv ] || [ -f ./dockerinit ]; then retry 'apt-get update -yqqq; apt-get -o dir::cache::archives="vendor/apt" install -y -qq --force-yes \ libicu-dev libkrb5-dev cmake nodejs postgresql-client mysql-client unzip' - apt-get install -qqy software-properties-common - # Add PPA for updating git, we need software-properties-common - add-apt-repository ppa:git-core/ppa + # Install latest Git from stretch packages + echo deb http://httpredir.debian.org/debian stretch main > /etc/apt/sources.list retry 'apt-get update -yqqq; apt-get -o dif::cache::archives="vendor/apt" install -yqq --force-yes git' + echo Git version... git --version cp config/database.yml.mysql config/database.yml -- GitLab From 5ab248438665d9474c19c009d343fb6bbdec1a22 Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Wed, 21 Sep 2016 01:13:45 +0300 Subject: [PATCH 16/20] Bring back tests wrongfully removed --- .../create_deployment_service_spec.rb | 28 +++++++++++++++++++ 1 file changed, 28 insertions(+) diff --git a/spec/services/create_deployment_service_spec.rb b/spec/services/create_deployment_service_spec.rb index 34ae2c768f21..5ca8cf20aa03 100644 --- a/spec/services/create_deployment_service_spec.rb +++ b/spec/services/create_deployment_service_spec.rb @@ -56,6 +56,34 @@ expect(subject).not_to be_persisted end end + + context 'when variables are used' do + let(:params) do + { environment: 'review-apps/$CI_BUILD_REF_NAME', + ref: 'master', + tag: false, + sha: '97de212e80737a608d939f648d959671fb0a0142', + options: { + name: 'review-apps/$CI_BUILD_REF_NAME', + url: 'http://$CI_BUILD_REF_NAME.review-apps.gitlab.com' + }, + variables: [ + { key: 'CI_BUILD_REF_NAME', value: 'feature-review-apps' } + ] + } + end + + it 'does create a new environment' do + expect { subject }.to change { Environment.count }.by(1) + + expect(subject.environment.name).to eq('review-apps/feature-review-apps') + expect(subject.environment.external_url).to eq('http://feature-review-apps.review-apps.gitlab.com') + end + + it 'does create a new deployment' do + expect(subject).to be_persisted + end + end end describe 'processing of builds' do -- GitLab From c4b23a5336a6c558326f74dc3643710e9d237e41 Mon Sep 17 00:00:00 2001 From: Luke Bennett Date: Wed, 21 Sep 2016 00:22:50 +0100 Subject: [PATCH 17/20] Fixed failing teaspoon test by adding MergeRequestWidget class to window.gl global --- app/assets/javascripts/merge_request_widget.js.es6 | 6 +++--- app/views/projects/merge_requests/widget/_show.html.haml | 2 +- spec/javascripts/merge_request_widget_spec.js | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/app/assets/javascripts/merge_request_widget.js.es6 b/app/assets/javascripts/merge_request_widget.js.es6 index dd07dc7e5d09..c0457e4b5001 100644 --- a/app/assets/javascripts/merge_request_widget.js.es6 +++ b/app/assets/javascripts/merge_request_widget.js.es6 @@ -1,4 +1,4 @@ -(function() { +((global) => { var indexOf = [].indexOf || function(item) { for (var i = 0, l = this.length; i < l; i++) { if (i in this && this[i] === item) return i; } return -1; }; const DEPLOYMENT_TEMPLATE = `
@@ -22,7 +22,7 @@
`; - this.MergeRequestWidget = (function() { + global.MergeRequestWidget = (function() { function MergeRequestWidget(opts) { // Initialize MergeRequestWidget behavior // @@ -234,4 +234,4 @@ })(); -}).call(this); +})(window.gl || (window.gl = {})); diff --git a/app/views/projects/merge_requests/widget/_show.html.haml b/app/views/projects/merge_requests/widget/_show.html.haml index ea618263a4a8..856ec1e0bee8 100644 --- a/app/views/projects/merge_requests/widget/_show.html.haml +++ b/app/views/projects/merge_requests/widget/_show.html.haml @@ -33,4 +33,4 @@ merge_request_widget.clearEventListeners(); } - merge_request_widget = new MergeRequestWidget(opts); + merge_request_widget = new window.gl.MergeRequestWidget(opts); diff --git a/spec/javascripts/merge_request_widget_spec.js b/spec/javascripts/merge_request_widget_spec.js index b8dfb51f4f3a..9b354d28f853 100644 --- a/spec/javascripts/merge_request_widget_spec.js +++ b/spec/javascripts/merge_request_widget_spec.js @@ -21,7 +21,7 @@ gitlab_icon: "gitlab_logo.png", builds_path: "http://sampledomain.local/sampleBuildsPath" }; - this["class"] = new MergeRequestWidget(this.opts); + this["class"] = new window.gl.MergeRequestWidget(this.opts); return this.ciStatusData = { "title": "Sample MR title", "sha": "12a34bc5", @@ -65,7 +65,7 @@ it('should not display a notification on the first check after the widget has been created', function() { var spy; spy = spyOn(window, 'notify'); - this["class"] = new MergeRequestWidget(this.opts); + this["class"] = new window.gl.MergeRequestWidget(this.opts); this["class"].getCIStatus(true); return expect(spy).not.toHaveBeenCalled(); }); -- GitLab From 303faf35430617e588e35ce0d306088f8a300c5e Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Mon, 26 Sep 2016 10:59:53 +0200 Subject: [PATCH 18/20] Reintroduce tests after removing them In previous commits on this branch I've removed methods and their tests. Now these methods where used by another branch so I needed to reintroduce them. --- .../projects/merge_requests_controller.rb | 4 ++- app/models/deployment.rb | 6 ++++ app/models/environment.rb | 6 ++++ spec/models/deployment_spec.rb | 24 ++++++++++++++ spec/models/environment_spec.rb | 33 +++++++++++++++++++ 5 files changed, 72 insertions(+), 1 deletion(-) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index 276aabc2b422..b7b46f137217 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -378,10 +378,12 @@ def ci_status if deployments deployments = deployments.map do |deployment| + project = deployment.project + deployment = { environment_name: deployment.environment.name, environment_id: deployment.environment.id, - environment_url: namespace_project_environment_path(@project.namespace, @project, deployment.environment), + environment_url: namespace_project_environment_path(project.namespace, project, deployment.environment), external_url: deployment.environment.external_url, created_at: deployment.created_at, created_at_formatted: deployment.created_at.to_time.in_time_zone.to_s(:medium) diff --git a/app/models/deployment.rb b/app/models/deployment.rb index 89cfd7f28c5b..b3d1c94c2823 100644 --- a/app/models/deployment.rb +++ b/app/models/deployment.rb @@ -33,6 +33,12 @@ def create_ref project.repository.fetch_ref(project.repository.path_to_repo, ref, ref_path) end + def includes_commit?(commit) + return false unless commit + + project.repository.is_ancestor?(commit.id, sha) + end + def manual_actions deployable.try(:other_actions) end diff --git a/app/models/environment.rb b/app/models/environment.rb index 22b5d235a3e9..44293e47848f 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -38,6 +38,12 @@ def set_environment_type end end + def includes_commit?(commit) + return false unless last_deployment + + last_deployment.includes_commit?(commit) + end + def deployment_id_for(commit) ref = project.repository.ref_name_for_sha(ref_path, commit.sha) diff --git a/spec/models/deployment_spec.rb b/spec/models/deployment_spec.rb index 7df3df4bb9e6..bfff639ad78c 100644 --- a/spec/models/deployment_spec.rb +++ b/spec/models/deployment_spec.rb @@ -15,4 +15,28 @@ it { is_expected.to validate_presence_of(:ref) } it { is_expected.to validate_presence_of(:sha) } + + describe '#includes_commit?' do + let(:project) { create(:project) } + let(:environment) { create(:environment, project: project) } + let(:deployment) do + create(:deployment, environment: environment, sha: project.commit.id) + end + + context 'when there is no project commit' do + it 'returns false' do + commit = project.commit('feature') + + expect(deployment.includes_commit?(commit)).to be false + end + end + + context 'when they share the same tree branch' do + it 'returns true' do + commit = project.commit + + expect(deployment.includes_commit?(commit)).to be true + end + end + end end diff --git a/spec/models/environment_spec.rb b/spec/models/environment_spec.rb index 360a17579a6b..fb9629ac47ab 100644 --- a/spec/models/environment_spec.rb +++ b/spec/models/environment_spec.rb @@ -31,6 +31,39 @@ end end + describe '#includes_commit?' do + context 'without a last deployment' do + it "returns false" do + expect(environment.includes_commit?('HEAD')).to be false + end + end + + context 'with a last deployment' do + let(:project) { create(:project) } + let(:environment) { create(:environment, project: project) } + + let!(:deployment) do + create(:deployment, environment: environment, sha: project.commit('master').id) + end + + context 'in the same branch' do + it 'returns true' do + expect(environment.includes_commit?(RepoHelpers.sample_commit)).to be true + end + end + + context 'not in the same branch' do + before do + deployment.update(sha: project.commit('feature').id) + end + + it 'returns false' do + expect(environment.includes_commit?(RepoHelpers.sample_commit)).to be false + end + end + end + end + describe '#deployment_id_for' do let(:project) { create(:project) } let!(:environment) { create(:environment, project: project) } -- GitLab From 0021cc2d0a7aad6476efda0c6d562b2015713e81 Mon Sep 17 00:00:00 2001 From: Luke Bennett Date: Mon, 26 Sep 2016 17:56:22 +0100 Subject: [PATCH 19/20] Changed deployments to environments and pull the deployment time if available. Also added perm check. --- .../javascripts/merge_request_widget.js.es6 | 43 ++++++++++--------- .../projects/merge_requests_controller.rb | 35 ++++++++------- spec/javascripts/merge_request_widget_spec.js | 12 +++--- 3 files changed, 49 insertions(+), 41 deletions(-) diff --git a/app/assets/javascripts/merge_request_widget.js.es6 b/app/assets/javascripts/merge_request_widget.js.es6 index c0457e4b5001..ebc4fad1f1d9 100644 --- a/app/assets/javascripts/merge_request_widget.js.es6 +++ b/app/assets/javascripts/merge_request_widget.js.es6 @@ -1,23 +1,21 @@ ((global) => { var indexOf = [].indexOf || function(item) { for (var i = 0, l = this.length; i < l; i++) { if (i in this && this[i] === item) return i; } return -1; }; - const DEPLOYMENT_TEMPLATE = `
+ const DEPLOYMENT_TEMPLATE = `
<%= ci_success_icon %> Deployed to - - <%- environment_name %> + + <%- name %> - - <%- created_at %>. + + <%- deployed_at %> -
- - - View on <%- external_url_formatted %> - -
+ + + View on <%- external_url_formatted %> +
`; @@ -147,7 +145,7 @@ if (data.status === '') { return; } - if (data.deployments && data.deployments.length) _this.renderDeployments(data.deployments); + if (data.environments && data.environments.length) _this.renderEnvironments(data.environments); if (_this.firstCICheck || data.status !== _this.opts.ci_status && (data.status != null)) { _this.opts.ci_status = data.status; _this.showCIStatus(data.status); @@ -180,16 +178,21 @@ })(this)); }; - MergeRequestWidget.prototype.renderDeployments = function(deployments) { - for (let i = 0; i < deployments.length; i++) { - const deployment = deployments[i]; - if ($(`.mr-state-widget #${ deployment.environment_id }`).length) return; + MergeRequestWidget.prototype.renderEnvironments = function(environments) { + for (let i = 0; i < environments.length; i++) { + const environment = environments[i]; + if ($(`.mr-state-widget #${ environment.id }`).length) return; const $template = $(DEPLOYMENT_TEMPLATE); - if (!deployment.external_url) $('.js-deployment-link', $template).remove(); - deployment.created_at = $.timeago(deployment.created_at); - deployment.ci_success_icon = this.$ciSuccessIcon; + if (!environment.external_url) $('.js-environment-link', $template).remove(); + if (environment.deployed_at) { + environment.deployed_at = $.timeago(environment.deployed_at) + '.'; + } else { + $('.js-environment-timeago', $template).remove(); + environment.name += '.'; + } + environment.ci_success_icon = this.$ciSuccessIcon; const templateString = _.unescape($template[0].outerHTML); - const template = _.template(templateString)(deployment) + const template = _.template(templateString)(environment) this.$widgetBody.before(template); } }; diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index b7b46f137217..d0e262acdac2 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -374,26 +374,31 @@ def ci_status end end + environments = @merge_request.environments deployments = @merge_request.deployments - if deployments - deployments = deployments.map do |deployment| - project = deployment.project - - deployment = { - environment_name: deployment.environment.name, - environment_id: deployment.environment.id, - environment_url: namespace_project_environment_path(project.namespace, project, deployment.environment), - external_url: deployment.environment.external_url, - created_at: deployment.created_at, - created_at_formatted: deployment.created_at.to_time.in_time_zone.to_s(:medium) + if environments + environments = environments.select { |e| can?(current_user, :read_environment, e) }.map do |environment| + project = environment.project + deployment = deployments.find { |d| d.environment == environment } + + environment = { + name: environment.name, + id: environment.id, + url: namespace_project_environment_path(project.namespace, project, environment), + external_url: environment.external_url, + deployed_at: deployment ? deployment.created_at : nil } - if deployment[:external_url] - deployment[:external_url_formatted] = deployment[:external_url].gsub(/\A.*?:\/\//, '') + if environment[:external_url] + environment[:external_url_formatted] = environment[:external_url].gsub(/\A.*?:\/\//, '') end - deployment + if environment[:deployed_at] + environment[:deployed_at_formatted] = environment[:deployed_at].to_time.in_time_zone.to_s(:medium) + end + + environment end end @@ -402,7 +407,7 @@ def ci_status sha: merge_request.diff_head_commit.short_id, status: status, coverage: coverage, - deployments: deployments + environments: environments } render json: response diff --git a/spec/javascripts/merge_request_widget_spec.js b/spec/javascripts/merge_request_widget_spec.js index 9b354d28f853..9420a1f9fce8 100644 --- a/spec/javascripts/merge_request_widget_spec.js +++ b/spec/javascripts/merge_request_widget_spec.js @@ -69,20 +69,20 @@ this["class"].getCIStatus(true); return expect(spy).not.toHaveBeenCalled(); }); - it('should call renderDeployments when the deployments property is set', function() { - this.ciStatusData.deployments = [{ + it('should call renderEnvironments when the environments property is set', function() { + this.ciStatusData.environments = [{ created_at: '2016-09-12T13:38:30.636Z', environment_id: 1, environment_name: 'env1', external_url: 'https://test-url.com', external_url_formatted: 'test-url.com' }]; - var spy = spyOn(this['class'], 'renderDeployments').and.stub(); + var spy = spyOn(this['class'], 'renderEnvironments').and.stub(); this['class'].getCIStatus(false); - expect(spy).toHaveBeenCalledWith(this.ciStatusData.deployments); + expect(spy).toHaveBeenCalledWith(this.ciStatusData.environments); }); - it('should not call renderDeployments when the deployments property is not set', function() { - var spy = spyOn(this['class'], 'renderDeployments').and.stub(); + it('should not call renderEnvironments when the environments property is not set', function() { + var spy = spyOn(this['class'], 'renderEnvironments').and.stub(); this['class'].getCIStatus(false); expect(spy).not.toHaveBeenCalled(); }); -- GitLab From f61e03456968e67b338771f5c656afffc0e50e49 Mon Sep 17 00:00:00 2001 From: "Z.J. van de Weg" Date: Tue, 27 Sep 2016 20:15:46 +0200 Subject: [PATCH 20/20] Incorporate feedback -- Deployed Time ago --- app/controllers/projects/merge_requests_controller.rb | 2 +- app/models/merge_request.rb | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/app/controllers/projects/merge_requests_controller.rb b/app/controllers/projects/merge_requests_controller.rb index d0e262acdac2..96275bc7b389 100644 --- a/app/controllers/projects/merge_requests_controller.rb +++ b/app/controllers/projects/merge_requests_controller.rb @@ -377,7 +377,7 @@ def ci_status environments = @merge_request.environments deployments = @merge_request.deployments - if environments + if environments.present? environments = environments.select { |e| can?(current_user, :read_environment, e) }.map do |environment| project = environment.project deployment = deployments.find { |d| d.environment == environment } diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 2ce03a31962d..30aa27af4801 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -673,11 +673,11 @@ def deployments environment.deployment_id_for(diff_head_commit) end.compact - target_project.deployments.find(deployment_ids) + Deployments.find(deployment_ids) end def environments - return [] unless diff_head_sha + return [] unless diff_head_commit environments = source_project.environments_for( source_branch, diff_head_commit) -- GitLab