diff options
author | Mayra Cabrera <mcabrera@gitlab.com> | 2019-02-05 13:11:33 -0600 |
---|---|---|
committer | Mayra Cabrera <mcabrera@gitlab.com> | 2019-02-06 21:51:48 -0600 |
commit | d9af3752fcfa6e97bcec82515b0cbc1ab88285de (patch) | |
tree | 61e1e650e5e7671f5d38b7acc1940c20e3c677e4 | |
parent | 087af654bbae1e4a843029b33e1aab546f4d7d61 (diff) | |
download | gitlab-ce-d9af3752fcfa6e97bcec82515b0cbc1ab88285de.tar.gz |
Addresses UX and BE comments:52363-ui-changes-to-cluster-and-ado-pages
- Changes help text on clusters form to make it more explicit.
- Removes unnecessary warnings on auto devops form
- Simplifies cluster methods logic
-rw-r--r-- | app/helpers/auto_devops_helper.rb | 26 | ||||
-rw-r--r-- | app/models/clusters/cluster.rb | 28 | ||||
-rw-r--r-- | app/views/clusters/clusters/_form.html.haml | 17 | ||||
-rw-r--r-- | app/views/projects/settings/ci_cd/_autodevops_form.html.haml | 8 | ||||
-rw-r--r-- | locale/gitlab.pot | 22 | ||||
-rw-r--r-- | spec/features/clusters/cluster_detail_page_spec.rb | 2 | ||||
-rw-r--r-- | spec/helpers/auto_devops_helper_spec.rb | 35 | ||||
-rw-r--r-- | spec/models/clusters/cluster_spec.rb | 78 | ||||
-rw-r--r-- | spec/views/projects/settings/ci_cd/_autodevops_form.html.haml_spec.rb | 53 |
9 files changed, 103 insertions, 166 deletions
diff --git a/app/helpers/auto_devops_helper.rb b/app/helpers/auto_devops_helper.rb index 8628c90dc51..67e7e475920 100644 --- a/app/helpers/auto_devops_helper.rb +++ b/app/helpers/auto_devops_helper.rb @@ -9,30 +9,4 @@ module AutoDevopsHelper !project.repository.gitlab_ci_yml && !project.ci_service 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 diff --git a/app/models/clusters/cluster.rb b/app/models/clusters/cluster.rb index bf339c935cf..f2f5b89e3bb 100644 --- a/app/models/clusters/cluster.rb +++ b/app/models/clusters/cluster.rb @@ -18,6 +18,7 @@ module Clusters Applications::Knative.application_name => Applications::Knative }.freeze DEFAULT_ENVIRONMENT = '*'.freeze + KUBE_INGRESS_BASE_DOMAIN = 'KUBE_INGRESS_BASE_DOMAIN'.freeze belongs_to :user @@ -196,22 +197,39 @@ module Clusters project_type? end - def has_domain? - domain.present? || instance_domain.present? + def kube_ingress_domain + @kube_ingress_domain ||= domain.presence || instance_domain || legacy_auto_devops_domain end def predefined_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 private 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 def restrict_modification diff --git a/app/views/clusters/clusters/_form.html.haml b/app/views/clusters/clusters/_form.html.haml index e0d3b7e1aec..7acd9ce0562 100644 --- a/app/views/clusters/clusters/_form.html.haml +++ b/app/views/clusters/clusters/_form.html.haml @@ -20,7 +20,7 @@ .form-text.text-muted= s_("ClusterIntegration|Choose which of your environments will use this cluster.") - else = 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 } .form-text.text-muted %code * @@ -30,17 +30,16 @@ %h5= s_('ClusterIntegration|Base domain') = field.text_field :base_domain, class: 'col-md-6 form-control js-select-on-focus' .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? - - 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') %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') - - custom_domain_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: custom_domain_url } - = 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 } - - else - = s_('ClusterIntegration|Before setting a domain, you must first install Ingress on your cluster below.') + = s_('ClusterIntegration| can be used instead of a custom domain.') + - custom_domain_url = help_page_path('user/project/clusters/index', anchor: 'pointing-your-dns-at-the-cluster-ip') + - custom_domain_start = '<a href="%{url}" target="_blank" rel="noopener noreferrer">'.html_safe % { url: custom_domain_url } + = s_('ClusterIntegration| %{custom_domain_start}More information%{custom_domain_end}.').html_safe % { custom_domain_start: custom_domain_start, custom_domain_end: '</a>'.html_safe } - if can?(current_user, :update_cluster, @cluster) .form-group diff --git a/app/views/projects/settings/ci_cd/_autodevops_form.html.haml b/app/views/projects/settings/ci_cd/_autodevops_form.html.haml index d905d015c22..8c4d1c32ebe 100644 --- a/app/views/projects/settings/ci_cd/_autodevops_form.html.haml +++ b/app/views/projects/settings/ci_cd/_autodevops_form.html.haml @@ -4,10 +4,6 @@ = form_errors(@project) %fieldset.builds-feature.js-auto-devops-settings .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| .card.auto-devops-card .card-body @@ -22,13 +18,11 @@ = link_to _('More information'), help_page_path('topics/autodevops/index.md'), target: '_blank' .card-footer.js-extra-settings{ class: @project.auto_devops_enabled? || 'hidden' } %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 } = 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 %strong= s_('CICD|Deployment strategy') - %p.settings-message.text-center - = s_('CICD|Deployment strategy needs a domain name to work correctly.') .form-check = form.radio_button :deploy_strategy, 'continuous', class: 'form-check-input' = form.label :deploy_strategy_continuous, class: 'form-check-label' do diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 8600e6fa394..d4b7f478b3f 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -846,15 +846,6 @@ msgstr "" msgid "Auto DevOps, runners and job artifacts" 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" msgstr "" @@ -1221,9 +1212,6 @@ msgstr "" msgid "CICD|Deployment strategy" msgstr "" -msgid "CICD|Deployment strategy needs a domain name to work correctly." -msgstr "" - msgid "CICD|Jobs" msgstr "" @@ -1506,7 +1494,10 @@ msgstr "" msgid "Closed (moved)" 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 "" 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 "" msgid "ClusterIntegration|Base domain" msgstr "" -msgid "ClusterIntegration|Before setting a domain, you must first install Ingress on your cluster below." -msgstr "" - msgid "ClusterIntegration|CA Certificate" msgstr "" @@ -1893,7 +1881,7 @@ msgstr "" msgid "ClusterIntegration|Something went wrong while installing %{title}" 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 "" 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." diff --git a/spec/features/clusters/cluster_detail_page_spec.rb b/spec/features/clusters/cluster_detail_page_spec.rb index e3c47fa4721..0a9c4bcaf12 100644 --- a/spec/features/clusters/cluster_detail_page_spec.rb +++ b/spec/features/clusters/cluster_detail_page_spec.rb @@ -45,7 +45,7 @@ describe 'Clusterable > Show page' do visit cluster_path 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 diff --git a/spec/helpers/auto_devops_helper_spec.rb b/spec/helpers/auto_devops_helper_spec.rb index 75c30dbfe48..223e562238d 100644 --- a/spec/helpers/auto_devops_helper_spec.rb +++ b/spec/helpers/auto_devops_helper_spec.rb @@ -90,39 +90,4 @@ describe AutoDevopsHelper do it { is_expected.to eq(false) } 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 diff --git a/spec/models/clusters/cluster_spec.rb b/spec/models/clusters/cluster_spec.rb index abef586d258..92ce2b0999a 100644 --- a/spec/models/clusters/cluster_spec.rb +++ b/spec/models/clusters/cluster_spec.rb @@ -516,29 +516,75 @@ describe Clusters::Cluster do end end - describe '#has_domain?' do - subject { cluster.has_domain? } - - 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 + describe '#kube_ingress_domain' do + let(:cluster) { create(:cluster, :provided_by_gcp) } - it { is_expected.to be_truthy } - end + subject { cluster.kube_ingress_domain } context 'with domain set in cluster' do let(:cluster) { create(:cluster, :provided_by_gcp, :with_domain) } - it { is_expected.to be_truthy } + it { is_expected.to eq(cluster.domain) } end - context 'when domain is not set at instance level nor in cluster' do - let(:cluster) { create(:cluster, :provided_by_gcp) } + context 'with no domain on cluster' do + 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 @@ -566,7 +612,7 @@ describe Clusters::Cluster do end context 'with no domain' do - let(:cluster) { create(:cluster) } + let(:cluster) { create(:cluster, :provided_by_gcp, :project) } it 'should return an empty array' do expect(subject.to_hash).to be_empty diff --git a/spec/views/projects/settings/ci_cd/_autodevops_form.html.haml_spec.rb b/spec/views/projects/settings/ci_cd/_autodevops_form.html.haml_spec.rb index cb1b9e6f5fb..2a2539c80b5 100644 --- a/spec/views/projects/settings/ci_cd/_autodevops_form.html.haml_spec.rb +++ b/spec/views/projects/settings/ci_cd/_autodevops_form.html.haml_spec.rb @@ -7,56 +7,9 @@ describe 'projects/settings/ci_cd/_autodevops_form' do assign :project, project end - context 'when kubernetes is not active' do - context 'when auto devops domain is not defined' do - it 'shows warning message' do - render + it 'shows a warning message about Kubernetes cluster' 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 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 + 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.') end end |