Automatic merge of gitlab-org/gitlab master

app/assets/javascripts/pipeline_new/constants.js

+import { __ } from '~/locale';
 import { DEFAULT_DEBOUNCE_AND_THROTTLE_MS } from '~/lib/utils/constants';
 
 export const VARIABLE_TYPE = 'env_var';
@@ -7,5 +8,7 @@ export const CONFIG_VARIABLES_TIMEOUT = 5000;
 export const BRANCH_REF_TYPE = 'branch';
 export const TAG_REF_TYPE = 'tag';
 
-export const CC_VALIDATION_REQUIRED_ERROR =
-  'Credit card required to be on file in order to create a pipeline';
+// must match pipeline/chain/validate/after_config.rb
+export const CC_VALIDATION_REQUIRED_ERROR = __(
+  'Credit card required to be on file in order to create a pipeline',
+);

app/assets/javascripts/projects/settings/components/shared_runners_toggle.vue

@@ -2,6 +2,7 @@
 import { GlAlert, GlToggle, GlTooltip } from '@gitlab/ui';
 import axios from '~/lib/utils/axios_utils';
 import { __, s__ } from '~/locale';
+import { CC_VALIDATION_REQUIRED_ERROR } from '../constants';
 
 const DEFAULT_ERROR_MESSAGE = __('An error occurred while updating the configuration.');
 const REQUIRES_VALIDATION_TEXT = s__(
@@ -47,11 +48,13 @@ export default {
     };
   },
   computed: {
-    showCreditCardValidation() {
+    ccRequiredError() {
+      return this.errorMessage === CC_VALIDATION_REQUIRED_ERROR && !this.ccAlertDismissed;
+    },
+    genericError() {
       return (
-        this.isCreditCardValidationRequired &&
-        !this.isSharedRunnerEnabled &&
-        !this.successfulValidation &&
+        this.errorMessage &&
+        this.errorMessage !== CC_VALIDATION_REQUIRED_ERROR &&
         !this.ccAlertDismissed
       );
     },
@@ -62,6 +65,7 @@ export default {
     },
     toggleSharedRunners() {
       this.isLoading = true;
+      this.ccAlertDismissed = false;
       this.errorMessage = null;
 
       axios
@@ -82,20 +86,19 @@ export default {
 <template>
   <div>
     <section class="gl-mt-5">
-      <gl-alert v-if="errorMessage" class="gl-mb-3" variant="danger" :dismissible="false">
-        {{ errorMessage }}
-      </gl-alert>
-
       <cc-validation-required-alert
-        v-if="showCreditCardValidation"
+        v-if="ccRequiredError"
         class="gl-pb-5"
         :custom-message="$options.i18n.REQUIRES_VALIDATION_TEXT"
         @verifiedCreditCard="creditCardValidated"
         @dismiss="ccAlertDismissed = true"
       />
 
+      <gl-alert v-if="genericError" class="gl-mb-3" variant="danger" :dismissible="false">
+        {{ errorMessage }}
+      </gl-alert>
+
       <gl-toggle
-        v-else
         ref="sharedRunnersToggle"
         :disabled="isDisabledAndUnoverridable"
         :is-loading="isLoading"

app/assets/javascripts/projects/settings/constants.js

+import { __ } from '~/locale';
+
 export const LEVEL_TYPES = {
   ROLE: 'role',
   USER: 'user',
@@ -18,3 +20,8 @@ export const ACCESS_LEVELS = {
 };
 
 export const ACCESS_LEVEL_NONE = 0;
+
+// must match shared_runners_setting in update_service.rb
+export const CC_VALIDATION_REQUIRED_ERROR = __(
+  'Shared runners enabled cannot be enabled until a valid credit card is on file',
+);

app/models/ci/pipeline.rb

@@ -1286,12 +1286,6 @@ module Ci
       end
     end
 
-    def use_variables_builder_definitions?
-      strong_memoize(:use_variables_builder_definitions) do
-        ::Feature.enabled?(:ci_use_variables_builder_definitions, project, default_enabled: :yaml)
-      end
-    end
-
     private
 
     def add_message(severity, content)

app/models/concerns/ci/contextable.rb

@@ -11,26 +11,9 @@ module Ci
     #
     def scoped_variables(environment: expanded_environment_name, dependencies: true)
       track_duration do
-        variables = pipeline.variables_builder.scoped_variables(self, environment: environment, dependencies: dependencies)
-
-        next variables if pipeline.use_variables_builder_definitions?
-
-        variables.concat(project.predefined_variables)
-        variables.concat(pipeline.predefined_variables)
-        variables.concat(runner.predefined_variables) if runnable? && runner
-        variables.concat(kubernetes_variables)
-        variables.concat(deployment_variables(environment: environment))
-        variables.concat(yaml_variables)
-        variables.concat(user_variables)
-        variables.concat(dependency_variables) if dependencies
-        variables.concat(secret_instance_variables)
-        variables.concat(secret_group_variables(environment: environment))
-        variables.concat(secret_project_variables(environment: environment))
-        variables.concat(trigger_request.user_variables) if trigger_request
-        variables.concat(pipeline.variables)
-        variables.concat(pipeline.pipeline_schedule.job_variables) if pipeline.pipeline_schedule
-
-        variables
+        pipeline
+          .variables_builder
+          .scoped_variables(self, environment: environment, dependencies: dependencies)
       end
     end
 
@@ -60,29 +43,5 @@ module Ci
         scoped_variables(environment: nil, dependencies: false)
       end
     end
-
-    def user_variables
-      pipeline.variables_builder.user_variables(user)
-    end
-
-    def kubernetes_variables
-      pipeline.variables_builder.kubernetes_variables(self)
-    end
-
-    def deployment_variables(environment:)
-      pipeline.variables_builder.deployment_variables(job: self, environment: environment)
-    end
-
-    def secret_instance_variables
-      pipeline.variables_builder.secret_instance_variables(ref: git_ref)
-    end
-
-    def secret_group_variables(environment: expanded_environment_name)
-      pipeline.variables_builder.secret_group_variables(environment: environment, ref: git_ref)
-    end
-
-    def secret_project_variables(environment: expanded_environment_name)
-      pipeline.variables_builder.secret_project_variables(environment: environment, ref: git_ref)
-    end
   end
 end

config/feature_flags/development/ci_use_variables_builder_definitions.yml

----
-name: ci_use_variables_builder_definitions
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/75254
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/349049
-milestone: '14.7'
-type: development
-group: group::pipeline execution
-default_enabled: false

ee/spec/frontend/projects/settings/components/shared_runners_toggle_spec.js

-import { GlToggle } from '@gitlab/ui';
+import { GlToggle, GlAlert } from '@gitlab/ui';
 import { shallowMount } from '@vue/test-utils';
 import MockAxiosAdapter from 'axios-mock-adapter';
 import { nextTick } from 'vue';
@@ -7,6 +7,7 @@ import { TEST_HOST } from 'helpers/test_constants';
 import waitForPromises from 'helpers/wait_for_promises';
 import axios from '~/lib/utils/axios_utils';
 import SharedRunnersToggleComponent from '~/projects/settings/components/shared_runners_toggle.vue';
+import { CC_VALIDATION_REQUIRED_ERROR } from '~/projects/settings/constants';
 
 const TEST_UPDATE_PATH = '/test/update_shared_runners';
 
@@ -29,6 +30,7 @@ describe('projects/settings/components/shared_runners', () => {
 
   const findSharedRunnersToggle = () => wrapper.findComponent(GlToggle);
   const findCcValidationRequiredAlert = () => wrapper.findComponent(CcValidationRequiredAlert);
+  const findGenericAlert = () => wrapper.findComponent(GlAlert);
   const getToggleValue = () => findSharedRunnersToggle().props('value');
   const isToggleDisabled = () => findSharedRunnersToggle().props('disabled');
 
@@ -54,36 +56,39 @@ describe('projects/settings/components/shared_runners', () => {
       });
     });
 
-    it('toggle should not be visible', () => {
-      expect(findSharedRunnersToggle().exists()).toBe(false);
+    it('should show the toggle button', () => {
+      expect(findSharedRunnersToggle().exists()).toBe(true);
+      expect(getToggleValue()).toBe(false);
+      expect(isToggleDisabled()).toBe(false);
     });
 
-    it('credit card validation component should exist', () => {
-      expect(findCcValidationRequiredAlert().exists()).toBe(true);
-      expect(findCcValidationRequiredAlert().text()).toBe(
-        SharedRunnersToggleComponent.i18n.REQUIRES_VALIDATION_TEXT,
-      );
-    });
+    describe('when credit card is unvalidated', () => {
+      beforeEach(() => {
+        mockAxios.onPost(TEST_UPDATE_PATH).reply(401, { error: CC_VALIDATION_REQUIRED_ERROR });
+      });
 
-    it('credit card alert should be hidden after dismiss', async () => {
-      findCcValidationRequiredAlert().vm.$emit('dismiss');
+      it('should show credit card validation error on toggle', async () => {
+        findSharedRunnersToggle().vm.$emit('change', true);
+        await waitForPromises();
 
-      await nextTick();
+        expect(findCcValidationRequiredAlert().exists()).toBe(true);
+        expect(findCcValidationRequiredAlert().text()).toBe(
+          SharedRunnersToggleComponent.i18n.REQUIRES_VALIDATION_TEXT,
+        );
+      });
 
-      expect(findCcValidationRequiredAlert().exists()).toBe(false);
-    });
+      it('should hide credit card alert on dismiss', async () => {
+        findSharedRunnersToggle().vm.$emit('change', true);
+        await waitForPromises();
 
-    describe('when credit card is validated', () => {
-      beforeEach(() => {
-        findCcValidationRequiredAlert().vm.$emit('verifiedCreditCard');
-      });
+        findCcValidationRequiredAlert().vm.$emit('dismiss');
+        await nextTick();
 
-      it('should show the toggle button', () => {
-        expect(findSharedRunnersToggle().exists()).toBe(true);
-        expect(getToggleValue()).toBe(false);
-        expect(isToggleDisabled()).toBe(false);
+        expect(findCcValidationRequiredAlert().exists()).toBe(false);
       });
+    });
 
+    describe('when credit card is validated', () => {
       it('should not show credit card alert after toggling on and off', async () => {
         findSharedRunnersToggle().vm.$emit('change', true);
         await waitForPromises();
@@ -100,5 +105,22 @@ describe('projects/settings/components/shared_runners', () => {
         expect(findCcValidationRequiredAlert().exists()).toBe(false);
       });
     });
+
+    describe('when toggling fails for some other reason', () => {
+      beforeEach(() => {
+        mockAxios.onPost(TEST_UPDATE_PATH).reply(500);
+      });
+
+      it('should show a generic alert instead', async () => {
+        findSharedRunnersToggle().vm.$emit('change', true);
+        await waitForPromises();
+
+        expect(findCcValidationRequiredAlert().exists()).toBe(false);
+        expect(findGenericAlert().exists()).toBe(true);
+        expect(findGenericAlert().text()).toBe(
+          'An error occurred while updating the configuration.',
+        );
+      });
+    });
   });
 });

lib/gitlab/ci/variables/builder.rb

@@ -13,9 +13,6 @@ module Gitlab
         def scoped_variables(job, environment:, dependencies:)
           Gitlab::Ci::Variables::Collection.new.tap do |variables|
             variables.concat(predefined_variables(job))
-
-            next variables unless pipeline.use_variables_builder_definitions?
-
             variables.concat(project.predefined_variables)
             variables.concat(pipeline.predefined_variables)
             variables.concat(job.runner.predefined_variables) if job.runnable? && job.runner

locale/gitlab.pot

@@ -10470,6 +10470,9 @@ msgstr ""
 msgid "CredentialsInventory|SSH Keys"
 msgstr ""
 
+msgid "Credit card required to be on file in order to create a pipeline"
+msgstr ""
+
 msgid "Credit card:"
 msgstr ""
 
@@ -33086,6 +33089,9 @@ msgstr ""
 msgid "Shared runners details"
 msgstr ""
 
+msgid "Shared runners enabled cannot be enabled until a valid credit card is on file"
+msgstr ""
+
 msgid "Shared runners help link"
 msgstr ""
 

spec/lib/gitlab/ci/variables/builder_spec.rb

@@ -3,9 +3,10 @@
 require 'spec_helper'
 
 RSpec.describe Gitlab::Ci::Variables::Builder do
-  let_it_be(:project) { create(:project, :repository) }
-  let_it_be(:pipeline) { create(:ci_pipeline, project: project) }
-  let_it_be(:user) { project.first_owner }
+  let_it_be(:group) { create(:group) }
+  let_it_be(:project) { create(:project, :repository, namespace: group) }
+  let_it_be_with_reload(:pipeline) { create(:ci_pipeline, project: project) }
+  let_it_be(:user) { create(:user) }
   let_it_be(:job) do
     create(:ci_build,
       pipeline: pipeline,
@@ -153,7 +154,7 @@ RSpec.describe Gitlab::Ci::Variables::Builder do
 
       before do
         allow(builder).to receive(:predefined_variables) { [var('A', 1), var('B', 1)] }
-        allow(project).to receive(:predefined_variables) { [var('B', 2), var('C', 2)] }
+        allow(pipeline.project).to receive(:predefined_variables) { [var('B', 2), var('C', 2)] }
         allow(pipeline).to receive(:predefined_variables) { [var('C', 3), var('D', 3)] }
         allow(job).to receive(:runner) { double(predefined_variables: [var('D', 4), var('E', 4)]) }
         allow(builder).to receive(:kubernetes_variables) { [var('E', 5), var('F', 5)] }
@@ -201,4 +202,146 @@ RSpec.describe Gitlab::Ci::Variables::Builder do
       end
     end
   end
+
+  describe '#user_variables' do
+    context 'with user' do
+      subject { builder.user_variables(user).to_hash }
+
+      let(:expected_variables) do
+        {
+          'GITLAB_USER_EMAIL' => user.email,
+          'GITLAB_USER_ID' => user.id.to_s,
+          'GITLAB_USER_LOGIN' => user.username,
+          'GITLAB_USER_NAME' => user.name
+        }
+      end
+
+      it { is_expected.to eq(expected_variables) }
+    end
+
+    context 'without user' do
+      subject { builder.user_variables(nil).to_hash }
+
+      it { is_expected.to be_empty }
+    end
+  end
+
+  describe '#kubernetes_variables' do
+    let(:service) { double(execute: template) }
+    let(:template) { double(to_yaml: 'example-kubeconfig', valid?: template_valid) }
+    let(:template_valid) { true }
+
+    subject { builder.kubernetes_variables(job) }
+
+    before do
+      allow(Ci::GenerateKubeconfigService).to receive(:new).with(job).and_return(service)
+    end
+
+    it { is_expected.to include(key: 'KUBECONFIG', value: 'example-kubeconfig', public: false, file: true) }
+
+    context 'generated config is invalid' do
+      let(:template_valid) { false }
+
+      it { is_expected.not_to include(key: 'KUBECONFIG', value: 'example-kubeconfig', public: false, file: true) }
+    end
+  end
+
+  describe '#deployment_variables' do
+    let(:environment) { 'production' }
+    let(:kubernetes_namespace) { 'namespace' }
+    let(:project_variables) { double }
+
+    subject { builder.deployment_variables(environment: environment, job: job) }
+
+    before do
+      allow(job).to receive(:expanded_kubernetes_namespace)
+        .and_return(kubernetes_namespace)
+
+      allow(project).to receive(:deployment_variables)
+        .with(environment: environment, kubernetes_namespace: kubernetes_namespace)
+        .and_return(project_variables)
+    end
+
+    context 'environment is nil' do
+      let(:environment) { nil }
+
+      it { is_expected.to be_empty }
+    end
+  end
+
+  shared_examples "secret CI variables" do
+    context 'when ref is branch' do
+      context 'when ref is protected' do
+        before do
+          create(:protected_branch, :developers_can_merge, name: job.ref, project: project)
+        end
+
+        it { is_expected.to include(variable) }
+      end
+
+      context 'when ref is not protected' do
+        it { is_expected.not_to include(variable) }
+      end
+    end
+
+    context 'when ref is tag' do
+      let_it_be(:job) { create(:ci_build, ref: 'v1.1.0', tag: true, pipeline: pipeline) }
+
+      context 'when ref is protected' do
+        before do
+          create(:protected_tag, project: project, name: 'v*')
+        end
+
+        it { is_expected.to include(variable) }
+      end
+
+      context 'when ref is not protected' do
+        it { is_expected.not_to include(variable) }
+      end
+    end
+
+    context 'when ref is merge request' do
+      let_it_be(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline, source_project: project) }
+      let_it_be(:pipeline) { merge_request.pipelines_for_merge_request.first }
+      let_it_be(:job) { create(:ci_build, ref: merge_request.source_branch, tag: false, pipeline: pipeline) }
+
+      context 'when ref is protected' do
+        before do
+          create(:protected_branch, :developers_can_merge, name: merge_request.source_branch, project: project)
+        end
+
+        it 'does not return protected variables as it is not supported for merge request pipelines' do
+          is_expected.not_to include(variable)
+        end
+      end
+
+      context 'when ref is not protected' do
+        it { is_expected.not_to include(variable) }
+      end
+    end
+  end
+
+  describe '#secret_instance_variables' do
+    subject { builder.secret_instance_variables(ref: job.git_ref) }
+
+    let_it_be(:variable) { create(:ci_instance_variable, protected: true) }
+
+    include_examples "secret CI variables"
+  end
+
+  describe '#secret_group_variables' do
+    subject { builder.secret_group_variables(ref: job.git_ref, environment: job.expanded_environment_name) }
+
+    let_it_be(:variable) { create(:ci_group_variable, protected: true, group: group) }
+
+    include_examples "secret CI variables"
+  end
+
+  describe '#secret_project_variables' do
+    subject { builder.secret_project_variables(ref: job.git_ref, environment: job.expanded_environment_name) }
+
+    let_it_be(:variable) { create(:ci_variable, protected: true, project: project) }
+
+    include_examples "secret CI variables"
+  end
 end

spec/models/ci/build_spec.rb

@@ -3618,20 +3618,6 @@ RSpec.describe Ci::Build do
 
       build.scoped_variables
     end
-
-    context 'when variables builder is used' do
-      it 'returns the same variables' do
-        build.user = create(:user)
-
-        allow(build.pipeline).to receive(:use_variables_builder_definitions?).and_return(false)
-        legacy_variables = build.scoped_variables.to_hash
-
-        allow(build.pipeline).to receive(:use_variables_builder_definitions?).and_return(true)
-        new_variables = build.scoped_variables.to_hash
-
-        expect(new_variables).to eq(legacy_variables)
-      end
-    end
   end
 
   describe '#simple_variables_without_dependencies' do
@@ -3642,160 +3628,6 @@ RSpec.describe Ci::Build do
     end
   end
 
-  shared_examples "secret CI variables" do
-    context 'when ref is branch' do
-      let(:pipeline) { create(:ci_pipeline, project: project) }
-      let(:build) { create(:ci_build, ref: 'master', tag: false, pipeline: pipeline, project: project) }
-
-      context 'when ref is protected' do
-        before do
-          create(:protected_branch, :developers_can_merge, name: 'master', project: project)
-        end
-
-        it { is_expected.to include(variable) }
-      end
-
-      context 'when ref is not protected' do
-        it { is_expected.not_to include(variable) }
-      end
-    end
-
-    context 'when ref is tag' do
-      let(:pipeline) { create(:ci_pipeline, project: project) }
-      let(:build) { create(:ci_build, ref: 'v1.1.0', tag: true, pipeline: pipeline, project: project) }
-
-      context 'when ref is protected' do
-        before do
-          create(:protected_tag, project: project, name: 'v*')
-        end
-
-        it { is_expected.to include(variable) }
-      end
-
-      context 'when ref is not protected' do
-        it { is_expected.not_to include(variable) }
-      end
-    end
-
-    context 'when ref is merge request' do
-      let(:merge_request) { create(:merge_request, :with_detached_merge_request_pipeline) }
-      let(:pipeline) { merge_request.pipelines_for_merge_request.first }
-      let(:build) { create(:ci_build, ref: merge_request.source_branch, tag: false, pipeline: pipeline, project: project) }
-
-      context 'when ref is protected' do
-        before do
-          create(:protected_branch, :developers_can_merge, name: merge_request.source_branch, project: project)
-        end
-
-        it 'does not return protected variables as it is not supported for merge request pipelines' do
-          is_expected.not_to include(variable)
-        end
-      end
-
-      context 'when ref is not protected' do
-        it { is_expected.not_to include(variable) }
-      end
-    end
-  end
-
-  describe '#secret_instance_variables' do
-    subject { build.secret_instance_variables }
-
-    let_it_be(:variable) { create(:ci_instance_variable, protected: true) }
-
-    include_examples "secret CI variables"
-  end
-
-  describe '#secret_group_variables' do
-    subject { build.secret_group_variables }
-
-    let_it_be(:variable) { create(:ci_group_variable, protected: true, group: group) }
-
-    include_examples "secret CI variables"
-  end
-
-  describe '#secret_project_variables' do
-    subject { build.secret_project_variables }
-
-    let_it_be(:variable) { create(:ci_variable, protected: true, project: project) }
-
-    include_examples "secret CI variables"
-  end
-
-  describe '#kubernetes_variables' do
-    let(:build) { create(:ci_build) }
-    let(:service) { double(execute: template) }
-    let(:template) { double(to_yaml: 'example-kubeconfig', valid?: template_valid) }
-    let(:template_valid) { true }
-
-    subject { build.kubernetes_variables }
-
-    before do
-      allow(Ci::GenerateKubeconfigService).to receive(:new).with(build).and_return(service)
-    end
-
-    it { is_expected.to include(key: 'KUBECONFIG', value: 'example-kubeconfig', public: false, file: true) }
-
-    context 'generated config is invalid' do
-      let(:template_valid) { false }
-
-      it { is_expected.not_to include(key: 'KUBECONFIG', value: 'example-kubeconfig', public: false, file: true) }
-    end
-  end
-
-  describe '#deployment_variables' do
-    let(:build) { create(:ci_build, environment: environment) }
-    let(:environment) { 'production' }
-    let(:kubernetes_namespace) { 'namespace' }
-    let(:project_variables) { double }
-
-    subject { build.deployment_variables(environment: environment) }
-
-    before do
-      allow(build).to receive(:expanded_kubernetes_namespace)
-        .and_return(kubernetes_namespace)
-
-      allow(build.project).to receive(:deployment_variables)
-        .with(environment: environment, kubernetes_namespace: kubernetes_namespace)
-        .and_return(project_variables)
-    end
-
-    context 'environment is nil' do
-      let(:environment) { nil }
-
-      it { is_expected.to be_empty }
-    end
-  end
-
-  describe '#user_variables' do
-    subject { build.user_variables.to_hash }
-
-    context 'with user' do
-      let(:expected_variables) do
-        {
-          'GITLAB_USER_EMAIL' => user.email,
-          'GITLAB_USER_ID' => user.id.to_s,
-          'GITLAB_USER_LOGIN' => user.username,
-          'GITLAB_USER_NAME' => user.name
-        }
-      end
-
-      before do
-        build.user = user
-      end
-
-      it { is_expected.to eq(expected_variables) }
-    end
-
-    context 'without user' do
-      before do
-        expect(build).to receive(:user).and_return(nil)
-      end
-
-      it { is_expected.to be_empty }
-    end
-  end
-
   describe '#any_unmet_prerequisites?' do
     let(:build) { create(:ci_build, :created) }