Commit d9af3752 authored by Mayra Cabrera's avatar Mayra Cabrera

Addresses UX and BE comments:

- Changes help text on clusters form to make it more explicit.
- Removes unnecessary warnings on auto devops form
- Simplifies cluster methods logic
parent 087af654
...@@ -9,30 +9,4 @@ module AutoDevopsHelper ...@@ -9,30 +9,4 @@ module AutoDevopsHelper
!project.repository.gitlab_ci_yml && !project.repository.gitlab_ci_yml &&
!project.ci_service !project.ci_service
end end
def auto_devops_warning_message(project)
if missing_auto_devops_service?(project)
params = {
kubernetes: link_to('Kubernetes cluster', project_clusters_path(project))
}
if missing_auto_devops_domain?(project)
_('Auto Review Apps and Auto Deploy need a domain name and a %{kubernetes} to work correctly.') % params
else
_('Auto Review Apps and Auto Deploy need a %{kubernetes} to work correctly.') % params
end
elsif missing_auto_devops_domain?(project)
_('Auto Review Apps and Auto Deploy need a domain name to work correctly.')
end
end
private
def missing_auto_devops_domain?(project)
!(project.auto_devops || project.build_auto_devops)&.has_domain?
end
def missing_auto_devops_service?(project)
!project.deployment_platform&.active?
end
end end
...@@ -18,6 +18,7 @@ module Clusters ...@@ -18,6 +18,7 @@ module Clusters
Applications::Knative.application_name => Applications::Knative Applications::Knative.application_name => Applications::Knative
}.freeze }.freeze
DEFAULT_ENVIRONMENT = '*'.freeze DEFAULT_ENVIRONMENT = '*'.freeze
KUBE_INGRESS_BASE_DOMAIN = 'KUBE_INGRESS_BASE_DOMAIN'.freeze
belongs_to :user belongs_to :user
...@@ -196,22 +197,39 @@ module Clusters ...@@ -196,22 +197,39 @@ module Clusters
project_type? project_type?
end end
def has_domain? def kube_ingress_domain
domain.present? || instance_domain.present? @kube_ingress_domain ||= domain.presence || instance_domain || legacy_auto_devops_domain
end end
def predefined_variables def predefined_variables
Gitlab::Ci::Variables::Collection.new.tap do |variables| Gitlab::Ci::Variables::Collection.new.tap do |variables|
break variables unless has_domain? break variables unless kube_ingress_domain
variables.append(key: 'KUBE_INGRESS_BASE_DOMAIN', value: domain.presence || instance_domain) variables.append(key: KUBE_INGRESS_BASE_DOMAIN, value: kube_ingress_domain)
end end
end end
private private
def instance_domain def instance_domain
Gitlab::CurrentSettings.auto_devops_domain @instance_domain ||= Gitlab::CurrentSettings.auto_devops_domain
end
# To keep backward compatibility with AUTO_DEVOPS_DOMAIN
# environment variable, we need to ensure KUBE_INGRESS_BASE_DOMAIN
# is set if AUTO_DEVOPS_DOMAIN is set on any of the following options:
# ProjectAutoDevops#Domain, project variables or group variables,
# as the AUTO_DEVOPS_DOMAIN is needed for CI_ENVIRONMENT_URL
#
# This method should be removed on 12.0
def legacy_auto_devops_domain
if project_type?
project&.auto_devops&.domain.presence ||
project.variables.find_by(key: 'AUTO_DEVOPS_DOMAIN')&.value.presence ||
project.group&.variables&.find_by(key: 'AUTO_DEVOPS_DOMAIN')&.value.presence
elsif group_type?
group.variables.find_by(key: 'AUTO_DEVOPS_DOMAIN')&.value.presence
end
end end
def restrict_modification def restrict_modification
......
...@@ -20,7 +20,7 @@ ...@@ -20,7 +20,7 @@
.form-text.text-muted= s_("ClusterIntegration|Choose which of your environments will use this cluster.") .form-text.text-muted= s_("ClusterIntegration|Choose which of your environments will use this cluster.")
- else - else
= text_field_tag :environment_scope, '*', class: 'col-md-6 form-control disabled', placeholder: s_('ClusterIntegration|Environment scope'), disabled: true = text_field_tag :environment_scope, '*', class: 'col-md-6 form-control disabled', placeholder: s_('ClusterIntegration|Environment scope'), disabled: true
- environment_scope_url = help_page_path('user/project/clusters', anchor: 'base-domain') - environment_scope_url = help_page_path('user/project/clusters/index', anchor: 'base-domain')
- environment_scope_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: environment_scope_url } - environment_scope_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: environment_scope_url }
.form-text.text-muted .form-text.text-muted
%code * %code *
...@@ -30,17 +30,16 @@ ...@@ -30,17 +30,16 @@
%h5= s_('ClusterIntegration|Base domain') %h5= s_('ClusterIntegration|Base domain')
= field.text_field :base_domain, class: 'col-md-6 form-control js-select-on-focus' = field.text_field :base_domain, class: 'col-md-6 form-control js-select-on-focus'
.form-text.text-muted .form-text.text-muted
- auto_devops_url = help_page_path('topics/autodevops/index')
- auto_devops_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: auto_devops_url }
= s_('ClusterIntegration|Specifying a domain will allow you to use Auto Review Apps and Auto Deploy stages for %{auto_devops_start}Auto DevOps%{auto_devops_end}. The domain should have a wildcard DNS configured matching the domain.').html_safe % { auto_devops_start: auto_devops_start, auto_devops_end: '</a>'.html_safe }
- if @cluster.application_ingress_external_ip.present? - if @cluster.application_ingress_external_ip.present?
- auto_devops_url = help_page_path('topics/autodevops/')
- auto_devops_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: auto_devops_url }
= s_('ClusterIntegration|Specifying a domain will allow you to use Auto Review Apps and Auto Deploy stages for %{auto_devops_start}Auto DevOps%{auto_devops_end}. The domain should have a wildcard DNS configured to the Ingress IP Address below.').html_safe % { auto_devops_start: auto_devops_start, auto_devops_end: '</a>'.html_safe }
= s_('ClusterIntegration|Alternatively') = s_('ClusterIntegration|Alternatively')
%code #{@cluster.application_ingress_external_ip}.nip.io %code #{@cluster.application_ingress_external_ip}.nip.io
- custom_domain_url = help_page_path('user/project/clusters/', anchor: 'pointing-your-dns-at-the-cluster-ip') = s_('ClusterIntegration| can be used instead of a custom domain.')
- custom_domain_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: custom_domain_url } - custom_domain_url = help_page_path('user/project/clusters/index', anchor: 'pointing-your-dns-at-the-cluster-ip')
= s_('ClusterIntegration| can be used instead of a custom domain. %{custom_domain_start}More information%{custom_domain_end}').html_safe % { custom_domain_start: custom_domain_start, custom_domain_end: '</a>'.html_safe } - custom_domain_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: custom_domain_url }
- else = s_('ClusterIntegration| %{custom_domain_start}More information%{custom_domain_end}.').html_safe % { custom_domain_start: custom_domain_start, custom_domain_end: '</a>'.html_safe }
= s_('ClusterIntegration|Before setting a domain, you must first install Ingress on your cluster below.')
- if can?(current_user, :update_cluster, @cluster) - if can?(current_user, :update_cluster, @cluster)
.form-group .form-group
......
...@@ -4,10 +4,6 @@ ...@@ -4,10 +4,6 @@
= form_errors(@project) = form_errors(@project)
%fieldset.builds-feature.js-auto-devops-settings %fieldset.builds-feature.js-auto-devops-settings
.form-group .form-group
- message = auto_devops_warning_message(@project)
- if message
%p.auto-devops-warning-message.settings-message.text-center
= message.html_safe
= f.fields_for :auto_devops_attributes, @auto_devops do |form| = f.fields_for :auto_devops_attributes, @auto_devops do |form|
.card.auto-devops-card .card.auto-devops-card
.card-body .card-body
...@@ -22,13 +18,11 @@ ...@@ -22,13 +18,11 @@
= link_to _('More information'), help_page_path('topics/autodevops/index.md'), target: '_blank' = link_to _('More information'), help_page_path('topics/autodevops/index.md'), target: '_blank'
.card-footer.js-extra-settings{ class: @project.auto_devops_enabled? || 'hidden' } .card-footer.js-extra-settings{ class: @project.auto_devops_enabled? || 'hidden' }
%p.settings-message.text-center %p.settings-message.text-center
- kubernetes_cluster_link = help_page_path('user/project/clusters/') - kubernetes_cluster_link = help_page_path('user/project/clusters/index')
- kubernetes_cluster_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: kubernetes_cluster_link } - kubernetes_cluster_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: kubernetes_cluster_link }
= s_('CICD|You must add a %{kubernetes_cluster_start}Kubernetes cluster integration%{kubernetes_cluster_end} to this project with a domain in order for your deployment strategy to work correctly.').html_safe % { kubernetes_cluster_start: kubernetes_cluster_start, kubernetes_cluster_end: '</a>'.html_safe } = s_('CICD|You must add a %{kubernetes_cluster_start}Kubernetes cluster integration%{kubernetes_cluster_end} to this project with a domain in order for your deployment strategy to work correctly.').html_safe % { kubernetes_cluster_start: kubernetes_cluster_start, kubernetes_cluster_end: '</a>'.html_safe }
%label.prepend-top-10 %label.prepend-top-10
%strong= s_('CICD|Deployment strategy') %strong= s_('CICD|Deployment strategy')
%p.settings-message.text-center
= s_('CICD|Deployment strategy needs a domain name to work correctly.')
.form-check .form-check
= form.radio_button :deploy_strategy, 'continuous', class: 'form-check-input' = form.radio_button :deploy_strategy, 'continuous', class: 'form-check-input'
= form.label :deploy_strategy_continuous, class: 'form-check-label' do = form.label :deploy_strategy_continuous, class: 'form-check-label' do
......
...@@ -846,15 +846,6 @@ msgstr "" ...@@ -846,15 +846,6 @@ msgstr ""
msgid "Auto DevOps, runners and job artifacts" msgid "Auto DevOps, runners and job artifacts"
msgstr "" msgstr ""
msgid "Auto Review Apps and Auto Deploy need a %{kubernetes} to work correctly."
msgstr ""
msgid "Auto Review Apps and Auto Deploy need a domain name and a %{kubernetes} to work correctly."
msgstr ""
msgid "Auto Review Apps and Auto Deploy need a domain name to work correctly."
msgstr ""
msgid "Auto-cancel redundant, pending pipelines" msgid "Auto-cancel redundant, pending pipelines"
msgstr "" msgstr ""
...@@ -1221,9 +1212,6 @@ msgstr "" ...@@ -1221,9 +1212,6 @@ msgstr ""
msgid "CICD|Deployment strategy" msgid "CICD|Deployment strategy"
msgstr "" msgstr ""
msgid "CICD|Deployment strategy needs a domain name to work correctly."
msgstr ""
msgid "CICD|Jobs" msgid "CICD|Jobs"
msgstr "" msgstr ""
...@@ -1506,7 +1494,10 @@ msgstr "" ...@@ -1506,7 +1494,10 @@ msgstr ""
msgid "Closed (moved)" msgid "Closed (moved)"
msgstr "" msgstr ""
msgid "ClusterIntegration| can be used instead of a custom domain. %{custom_domain_start}More information%{custom_domain_end}" msgid "ClusterIntegration| %{custom_domain_start}More information%{custom_domain_end}."
msgstr ""
msgid "ClusterIntegration| can be used instead of a custom domain."
msgstr "" msgstr ""
msgid "ClusterIntegration| is the default environment scope for this cluster. This means that all jobs, regardless of their environment, will use this cluster. %{environment_scope_start}More information%{environment_scope_end}" msgid "ClusterIntegration| is the default environment scope for this cluster. This means that all jobs, regardless of their environment, will use this cluster. %{environment_scope_start}More information%{environment_scope_end}"
...@@ -1566,9 +1557,6 @@ msgstr "" ...@@ -1566,9 +1557,6 @@ msgstr ""
msgid "ClusterIntegration|Base domain" msgid "ClusterIntegration|Base domain"
msgstr "" msgstr ""
msgid "ClusterIntegration|Before setting a domain, you must first install Ingress on your cluster below."
msgstr ""
msgid "ClusterIntegration|CA Certificate" msgid "ClusterIntegration|CA Certificate"
msgstr "" msgstr ""
...@@ -1893,7 +1881,7 @@ msgstr "" ...@@ -1893,7 +1881,7 @@ msgstr ""
msgid "ClusterIntegration|Something went wrong while installing %{title}" msgid "ClusterIntegration|Something went wrong while installing %{title}"
msgstr "" msgstr ""
msgid "ClusterIntegration|Specifying a domain will allow you to use Auto Review Apps and Auto Deploy stages for %{auto_devops_start}Auto DevOps%{auto_devops_end}. The domain should have a wildcard DNS configured to the Ingress IP Address below." msgid "ClusterIntegration|Specifying a domain will allow you to use Auto Review Apps and Auto Deploy stages for %{auto_devops_start}Auto DevOps%{auto_devops_end}. The domain should have a wildcard DNS configured matching the domain."
msgstr "" msgstr ""
msgid "ClusterIntegration|The IP address is in the process of being assigned. Please check your Kubernetes cluster or Quotas on Google Kubernetes Engine if it takes a long time." msgid "ClusterIntegration|The IP address is in the process of being assigned. Please check your Kubernetes cluster or Quotas on Google Kubernetes Engine if it takes a long time."
......
...@@ -45,7 +45,7 @@ describe 'Clusterable > Show page' do ...@@ -45,7 +45,7 @@ describe 'Clusterable > Show page' do
visit cluster_path visit cluster_path
within '#cluster-integration' do within '#cluster-integration' do
expect(page).to have_content('Before setting a domain, you must first install Ingress on your cluster below.') expect(page).not_to have_content('can be used instead of a custom domain.')
end end
end end
end end
......
...@@ -90,39 +90,4 @@ describe AutoDevopsHelper do ...@@ -90,39 +90,4 @@ describe AutoDevopsHelper do
it { is_expected.to eq(false) } it { is_expected.to eq(false) }
end end
end end
describe '.auto_devops_warning_message' do
subject { helper.auto_devops_warning_message(project) }
context 'when the service is missing' do
before do
allow(helper).to receive(:missing_auto_devops_service?).and_return(true)
end
context 'when the domain is missing' do
before do
allow(helper).to receive(:missing_auto_devops_domain?).and_return(true)
end
it { is_expected.to match(/Auto Review Apps and Auto Deploy need a domain name and a .* to work correctly./) }
end
context 'when the domain is not missing' do
before do
allow(helper).to receive(:missing_auto_devops_domain?).and_return(false)
end
it { is_expected.to match(/Auto Review Apps and Auto Deploy need a .* to work correctly./) }
end
end
context 'when the domain is missing' do
before do
allow(helper).to receive(:missing_auto_devops_service?).and_return(false)
allow(helper).to receive(:missing_auto_devops_domain?).and_return(true)
end
it { is_expected.to eq('Auto Review Apps and Auto Deploy need a domain name to work correctly.') }
end
end
end end
...@@ -516,29 +516,75 @@ describe Clusters::Cluster do ...@@ -516,29 +516,75 @@ describe Clusters::Cluster do
end end
end end
describe '#has_domain?' do describe '#kube_ingress_domain' do
subject { cluster.has_domain? } let(:cluster) { create(:cluster, :provided_by_gcp) }
context 'with domain set at instance level' do
let(:cluster) { create(:cluster, :provided_by_gcp) }
before do
stub_application_setting(auto_devops_domain: 'global_domain.com')
end
it { is_expected.to be_truthy } subject { cluster.kube_ingress_domain }
end
context 'with domain set in cluster' do context 'with domain set in cluster' do
let(:cluster) { create(:cluster, :provided_by_gcp, :with_domain) } let(:cluster) { create(:cluster, :provided_by_gcp, :with_domain) }
it { is_expected.to be_truthy } it { is_expected.to eq(cluster.domain) }
end end
context 'when domain is not set at instance level nor in cluster' do context 'with no domain on cluster' do
let(:cluster) { create(:cluster, :provided_by_gcp) } context 'with a project cluster' do
let(:cluster) { create(:cluster, :project, :provided_by_gcp) }
let(:project) { cluster.project }
context 'with domain set at instance level' do
before do
stub_application_setting(auto_devops_domain: 'global_domain.com')
it { is_expected.to eq('global_domain.com') }
end
end
context 'with domain set on ProjectAutoDevops' do
before do
auto_devops = project.build_auto_devops(domain: 'legacy-ado-domain.com')
auto_devops.save
end
it { is_expected.to eq('legacy-ado-domain.com') }
end
it { is_expected.to be_falsy } context 'with domain set as environment variable on project' do
before do
variable = project.variables.build(key: 'AUTO_DEVOPS_DOMAIN', value: 'project-ado-domain.com')
variable.save
end
it { is_expected.to eq('project-ado-domain.com') }
end
context 'with domain set as environment variable on the group project' do
let(:group) { create(:group) }
before do
project.update(parent_id: group.id)
variable = group.variables.build(key: 'AUTO_DEVOPS_DOMAIN', value: 'group-ado-domain.com')
variable.save
end
it { is_expected.to eq('group-ado-domain.com') }
end
end
context 'with a group cluster' do
let(:cluster) { create(:cluster, :group, :provided_by_gcp) }
context 'with domain set as environment variable for the group' do
let(:group) { cluster.group }
before do
variable = group.variables.build(key: 'AUTO_DEVOPS_DOMAIN', value: 'group-ado-domain.com')
variable.save
end
it { is_expected.to eq('group-ado-domain.com') }
end
end
end end
end end
...@@ -566,7 +612,7 @@ describe Clusters::Cluster do ...@@ -566,7 +612,7 @@ describe Clusters::Cluster do
end end
context 'with no domain' do context 'with no domain' do
let(:cluster) { create(:cluster) } let(:cluster) { create(:cluster, :provided_by_gcp, :project) }
it 'should return an empty array' do it 'should return an empty array' do
expect(subject.to_hash).to be_empty expect(subject.to_hash).to be_empty
......
...@@ -7,56 +7,9 @@ describe 'projects/settings/ci_cd/_autodevops_form' do ...@@ -7,56 +7,9 @@ describe 'projects/settings/ci_cd/_autodevops_form' do
assign :project, project assign :project, project
end end
context 'when kubernetes is not active' do it 'shows a warning message about Kubernetes cluster' do
context 'when auto devops domain is not defined' do render
it 'shows warning message' do
render
expect(rendered).to have_css('.auto-devops-warning-message') expect(rendered).to have_text('You must add a Kubernetes cluster integration to this project with a domain in order for your deployment strategy to work correctly.')
expect(rendered).to have_text('Auto Review Apps and Auto Deploy need a domain name and a')
expect(rendered).to have_link('Kubernetes cluster')
end
end
context 'when auto devops domain is defined' do
before do
project.build_auto_devops(domain: 'example.com')
end
it 'shows warning message' do
render
expect(rendered).to have_css('.auto-devops-warning-message')
expect(rendered).to have_text('Auto Review Apps and Auto Deploy need a')
expect(rendered).to have_link('Kubernetes cluster')
end
end
end
context 'when kubernetes is active' do
before do
create(:kubernetes_service, project: project)
end
context 'when auto devops domain is not defined' do
it 'shows warning message' do
render
expect(rendered).to have_css('.auto-devops-warning-message')
expect(rendered).to have_text('Auto Review Apps and Auto Deploy need a domain name to work correctly.')
end
end
context 'when auto devops domain is defined' do
before do
project.build_auto_devops(domain: 'example.com')
end
it 'does not show warning message' do
render
expect(rendered).not_to have_css('.auto-devops-warning-message')
end
end
end end
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