Commit b4dbc373 authored by Rémy Coutable's avatar Rémy Coutable Committed by Ruben Davila

Merge branch 'limit-number-of-shown-environments' into 'master'

Limit number of shown environments

## What does this MR do?

This MR limits in context of Merge Request a list of shown environments. Previously we would show all environments containing the SHA of the head commit of Merge Request. However, with introducing of dynamically created environments this lead to a cases that we would show multiple review apps, for different branches, because these branches would contain a new questioned commit.

This MR changes what environments we test against presence of the commit, to:
1. We look for environments with deployments to source_branch of source_project: used for deployments to per-branch environments,
2. We look for environments with deployments to target_branch of target_project: used for deployments to staging / production environments,
3. We look for environments with deployments for tags on target_project: used for staging / production environments.

## Why was this MR needed?

To improve a list of returned environments when we introduced ability to create dynamic environments for review apps:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/6323

See merge request !6438
parent 33bb5ddf
...@@ -25,6 +25,7 @@ v 8.12.0 (unreleased) ...@@ -25,6 +25,7 @@ v 8.12.0 (unreleased)
- Cycle analytics (first iteration) !5986 - Cycle analytics (first iteration) !5986
- Remove vendor prefixes for linear-gradient CSS (ClemMakesApps) - Remove vendor prefixes for linear-gradient CSS (ClemMakesApps)
- Move pushes_since_gc from the database to Redis - Move pushes_since_gc from the database to Redis
- Limit number of shown environments on Merge Request: show only environments for target_branch, source_branch and tags
- Add font color contrast to external label in admin area (ClemMakesApps) - Add font color contrast to external label in admin area (ClemMakesApps)
- Change logo animation to CSS (ClemMakesApps) - Change logo animation to CSS (ClemMakesApps)
- Instructions for enabling Git packfile bitmaps !6104 - Instructions for enabling Git packfile bitmaps !6104
......
...@@ -70,6 +70,10 @@ module GitlabRoutingHelper ...@@ -70,6 +70,10 @@ module GitlabRoutingHelper
namespace_project_runner_path(@project.namespace, @project, runner, *args) namespace_project_runner_path(@project.namespace, @project, runner, *args)
end end
def environment_path(environment, *args)
namespace_project_environment_path(environment.project.namespace, environment.project, environment, *args)
end
def issue_path(entity, *args) def issue_path(entity, *args)
namespace_project_issue_path(entity.project.namespace, entity.project, entity, *args) namespace_project_issue_path(entity.project.namespace, entity.project, entity, *args)
end end
......
...@@ -670,9 +670,12 @@ class MergeRequest < ActiveRecord::Base ...@@ -670,9 +670,12 @@ class MergeRequest < ActiveRecord::Base
def environments def environments
return [] unless diff_head_commit return [] unless diff_head_commit
target_project.environments.select do |environment| environments = source_project.environments_for(
environment.includes_commit?(diff_head_commit) source_branch, diff_head_commit)
end environments += target_project.environments_for(
target_branch, diff_head_commit, with_tags: true)
environments.uniq
end end
def state_human_name def state_human_name
......
...@@ -1293,6 +1293,22 @@ class Project < ActiveRecord::Base ...@@ -1293,6 +1293,22 @@ class Project < ActiveRecord::Base
Gitlab::Redis.with { |redis| redis.del(pushes_since_gc_redis_key) } Gitlab::Redis.with { |redis| redis.del(pushes_since_gc_redis_key) }
end end
def environments_for(ref, commit, with_tags: false)
environment_ids = deployments.group(:environment_id).
select(:environment_id)
environment_ids =
if with_tags
environment_ids.where('ref=? OR tag IS TRUE', ref)
else
environment_ids.where(ref: ref)
end
environments.where(id: environment_ids).select do |environment|
environment.includes_commit?(commit)
end
end
private private
def pushes_since_gc_redis_key def pushes_since_gc_redis_key
......
...@@ -43,15 +43,16 @@ ...@@ -43,15 +43,16 @@
= icon("times-circle") = icon("times-circle")
Could not connect to the CI server. Please check your settings and try again. Could not connect to the CI server. Please check your settings and try again.
- @merge_request.environments.each do |environment| - @merge_request.environments.sort_by(&:name).each do |environment|
.mr-widget-heading - if can?(current_user, :read_environment, environment)
.ci_widget.ci-success .mr-widget-heading
= ci_icon_for_status("success") .ci_widget.ci-success
%span.hidden-sm = ci_icon_for_status("success")
Deployed to %span.hidden-sm
= succeed '.' do Deployed to
= link_to environment.name, namespace_project_environment_path(@project.namespace, @project, environment), class: 'environment' = succeed '.' do
- external_url = environment.external_url = link_to environment.name, environment_path(environment), class: 'environment'
- if external_url - external_url = environment.external_url
= link_to external_url, target: '_blank' do - if external_url
= icon('external-link', text: "View on #{external_url.gsub(/\A.*?:\/\//, '')}", right: true) = link_to external_url, target: '_blank' do
= icon('external-link', text: "View on #{external_url.gsub(/\A.*?:\/\//, '')}", right: true)
...@@ -748,16 +748,57 @@ describe MergeRequest, models: true do ...@@ -748,16 +748,57 @@ describe MergeRequest, models: true do
end end
end end
describe "#environments" do describe '#environments' do
let(:project) { create(:project) } let(:project) { create(:project) }
let(:merge_request) { create(:merge_request, source_project: project) } let(:merge_request) { create(:merge_request, source_project: project) }
it 'selects deployed environments' do context 'with multiple environments' do
environments = create_list(:environment, 3, project: project) let(: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] before do
create(:deployment, environment: environments.first, ref: 'master', sha: project.commit('master').id)
create(:deployment, environment: environments.second, ref: 'feature', sha: project.commit('feature').id)
end
it 'selects deployed environments' do
expect(merge_request.environments).to contain_exactly(environments.first)
end
end
context 'with environments on source project' do
let(:source_project) do
create(:project) do |fork_project|
fork_project.create_forked_project_link(forked_to_project_id: fork_project.id, forked_from_project_id: project.id)
end
end
let(:merge_request) do
create(:merge_request,
source_project: source_project, source_branch: 'feature',
target_project: project)
end
let(:source_environment) { create(:environment, project: source_project) }
before do
create(:deployment, environment: source_environment, ref: 'feature', sha: merge_request.diff_head_sha)
end
it 'selects deployed environments' do
expect(merge_request.environments).to contain_exactly(source_environment)
end
context 'with environments on target project' do
let(:target_environment) { create(:environment, project: project) }
before do
create(:deployment, environment: target_environment, tag: true, sha: merge_request.diff_head_sha)
end
it 'selects deployed environments' do
expect(merge_request.environments).to contain_exactly(source_environment, target_environment)
end
end
end end
context 'without a diff_head_commit' do context 'without a diff_head_commit' do
......
...@@ -1647,6 +1647,47 @@ describe Project, models: true do ...@@ -1647,6 +1647,47 @@ describe Project, models: true do
end end
end end
describe '#environments_for' do
let(:project) { create(:project) }
let(:environment) { create(:environment, project: project) }
context 'tagged deployment' do
before do
create(:deployment, environment: environment, ref: '1.0', tag: true, sha: project.commit.id)
end
it 'returns environment when with_tags is set' do
expect(project.environments_for('master', project.commit, with_tags: true)).to contain_exactly(environment)
end
it 'does not return environment when no with_tags is set' do
expect(project.environments_for('master', project.commit)).to be_empty
end
it 'does not return environment when commit is not part of deployment' do
expect(project.environments_for('master', project.commit('feature'))).to be_empty
end
end
context 'branch deployment' do
before do
create(:deployment, environment: environment, ref: 'master', sha: project.commit.id)
end
it 'returns environment when ref is set' do
expect(project.environments_for('master', project.commit)).to contain_exactly(environment)
end
it 'does not environment when ref is different' do
expect(project.environments_for('feature', project.commit)).to be_empty
end
it 'does not return environment when commit is not part of deployment' do
expect(project.environments_for('master', project.commit('feature'))).to be_empty
end
end
end
def enable_lfs def enable_lfs
allow(Gitlab.config.lfs).to receive(:enabled).and_return(true) allow(Gitlab.config.lfs).to receive(:enabled).and_return(true)
end end
......
...@@ -15,6 +15,8 @@ describe 'projects/merge_requests/widget/_heading' do ...@@ -15,6 +15,8 @@ describe 'projects/merge_requests/widget/_heading' do
assign(:merge_request, merge_request) assign(:merge_request, merge_request)
assign(:project, project) assign(:project, project)
allow(view).to receive(:can?).and_return(true)
render render
end end
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment