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

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