diff --git a/app/assets/javascripts/ci/pipeline_new/components/pipeline_new_form.vue b/app/assets/javascripts/ci/pipeline_new/components/pipeline_new_form.vue index fa173e46711b20591d7cd40ac6f9741ec309e8de..ce32584a86198362abafb6f044fd8f9f27c0fa91 100644 --- a/app/assets/javascripts/ci/pipeline_new/components/pipeline_new_form.vue +++ b/app/assets/javascripts/ci/pipeline_new/components/pipeline_new_form.vue @@ -8,7 +8,6 @@ import glFeatureFlagsMixin from '~/vue_shared/mixins/gl_feature_flags_mixin'; import SafeHtml from '~/vue_shared/directives/safe_html'; import PipelineInputsForm from '~/ci/common/pipeline_inputs/pipeline_inputs_form.vue'; import PipelineVariablesPermissionsMixin from '~/ci/mixins/pipeline_variables_permissions_mixin'; -import { IDENTITY_VERIFICATION_REQUIRED_ERROR } from '../constants'; import createPipelineMutation from '../graphql/mutations/create_pipeline.mutation.graphql'; import RefsDropdown from './refs_dropdown.vue'; import PipelineVariablesForm from './pipeline_variables_form.vue'; @@ -100,7 +99,7 @@ export default { }, computed: { identityVerificationRequiredError() { - return this.error === IDENTITY_VERIFICATION_REQUIRED_ERROR; + return this.error === __('Identity verification is required in order to run CI jobs'); }, isPipelineInputsFeatureAvailable() { return this.glFeatures.ciInputsForPipelines; diff --git a/app/assets/javascripts/ci/pipeline_new/components/pipeline_variables_form.vue b/app/assets/javascripts/ci/pipeline_new/components/pipeline_variables_form.vue index c53c64d26cf97ca69c684257b0f04b3c871b108f..49702e3546f97ecb56293e7003c6744a5071cd53 100644 --- a/app/assets/javascripts/ci/pipeline_new/components/pipeline_variables_form.vue +++ b/app/assets/javascripts/ci/pipeline_new/components/pipeline_variables_form.vue @@ -20,25 +20,20 @@ import InputsAdoptionBanner from '~/ci/common/pipeline_inputs/inputs_adoption_ba import { fetchPolicies } from '~/lib/graphql'; import filterVariables from '../utils/filter_variables'; import { - CONFIG_VARIABLES_TIMEOUT, CI_VARIABLE_TYPE_FILE, CI_VARIABLE_TYPE_ENV_VAR, + CI_VARIABLES_POLLING_INTERVAL, + CI_VARIABLES_MAX_POLLING_TIME, } from '../constants'; import ciConfigVariablesQuery from '../graphql/queries/ci_config_variables.graphql'; import VariableValuesListbox from './variable_values_listbox.vue'; -let pollTimeout; -export const POLLING_INTERVAL = 2000; - export default { name: 'PipelineVariablesForm', formElementClasses: 'gl-basis-1/4 gl-shrink-0 gl-flex-grow-0', learnMorePath: helpPagePath('ci/variables/_index', { anchor: 'cicd-variable-precedence', }), - // this height value is used inline on the textarea to match the input field height - // it's used to prevent the overwrite if 'gl-h-7' or '!gl-h-7' were used - textAreaStyle: { height: '32px' }, components: { GlIcon, GlButton, @@ -83,15 +78,13 @@ export default { ciConfigVariables: null, configVariablesWithDescription: {}, form: {}, + maxPollTimeout: null, }; }, apollo: { ciConfigVariables: { fetchPolicy: fetchPolicies.NO_CACHE, query: ciConfigVariablesQuery, - skip() { - return Object.keys(this.form).includes(this.refParam); - }, variables() { return { fullPath: this.projectPath, @@ -99,30 +92,19 @@ export default { }; }, update({ project }) { - return project?.ciConfigVariables || []; + return project?.ciConfigVariables; }, - result() { - // API cache is empty when ciConfigVariables === null, so we need to - // poll while cache values are being populated in the backend. - // After CONFIG_VARIABLES_TIMEOUT ms have passed, we stop polling - // and populate the form regardless. - if (this.isFetchingCiConfigVariables && !pollTimeout) { - pollTimeout = setTimeout(() => { - this.ciConfigVariables = []; - this.clearPolling(); - this.populateForm(); - }, CONFIG_VARIABLES_TIMEOUT); - } - - if (!this.isFetchingCiConfigVariables) { - this.clearPolling(); - this.populateForm(); + result({ data }) { + if (data?.project?.ciConfigVariables) { + this.stopPollingAndPopulateForm(); } }, error(error) { reportToSentry(this.$options.name, error); + this.ciConfigVariables = []; + this.stopPollingAndPopulateForm(); }, - pollInterval: POLLING_INTERVAL, + pollInterval: CI_VARIABLES_POLLING_INTERVAL, }, }, computed: { @@ -133,7 +115,7 @@ export default { return this.ciConfigVariables === null; }, isLoading() { - return this.$apollo.queries.ciConfigVariables.loading || this.isFetchingCiConfigVariables; + return this.isFetchingCiConfigVariables; }, isMobile() { return ['sm', 'xs'].includes(GlBreakpointInstance.getBreakpointSize()); @@ -167,6 +149,18 @@ export default { }, deep: true, }, + refParam() { + this.ciConfigVariables = null; + this.clearTimeouts(); + this.$apollo.queries.ciConfigVariables.startPolling(CI_VARIABLES_POLLING_INTERVAL); + this.startMaxPollTimer(); + }, + }, + mounted() { + this.startMaxPollTimer(); + }, + beforeDestroy() { + this.clearTimeouts(); }, methods: { addEmptyVariable(refValue) { @@ -187,12 +181,14 @@ export default { canRemove(index) { return index < this.variables.length - 1; }, - clearPolling() { - clearTimeout(pollTimeout); - this.$apollo.queries.ciConfigVariables.stopPolling(); + clearTimeouts() { + if (this.maxPollTimeout) { + clearTimeout(this.maxPollTimeout); + this.maxPollTimeout = null; + } }, createListItemsFromVariableOptions(key) { - return this.configVariablesWithDescription.options[key].map((option) => ({ + return (this.configVariablesWithDescription?.options?.[key] || []).map((option) => ({ text: option, value: option, })); @@ -201,6 +197,7 @@ export default { return `${s__('Pipeline|Variable')} ${index + 1}`; }, populateForm() { + if (!this.ciConfigVariables) return; this.configVariablesWithDescription = this.ciConfigVariables.reduce( (accumulator, { description, key, value, valueOptions }) => { if (description) { @@ -222,14 +219,12 @@ export default { }, }; - // Add default variables from yml this.setVariableParams( this.refParam, CI_VARIABLE_TYPE_ENV_VAR, this.configVariablesWithDescription.values, ); - // Add/update variables, e.g. from query string if (this.variableParams) { this.setVariableParams(this.refParam, CI_VARIABLE_TYPE_ENV_VAR, this.variableParams); } @@ -238,7 +233,6 @@ export default { this.setVariableParams(this.refParam, CI_VARIABLE_TYPE_FILE, this.fileParams); } - // Adds empty var at the end of the form this.addEmptyVariable(this.refParam); }, removeVariable(index) { @@ -247,6 +241,7 @@ export default { setVariableAttribute(key, attribute, value) { const { variables } = this.form[this.refParam]; const variable = variables.find((v) => v.key === key); + if (!variable) return; variable[attribute] = value; }, setVariable(refValue, { type, key, value }) { @@ -273,6 +268,19 @@ export default { shouldShowValuesDropdown(key) { return this.configVariablesWithDescription.options[key]?.length > 1; }, + startMaxPollTimer() { + this.maxPollTimeout = setTimeout(() => { + if (this.ciConfigVariables === null) { + this.ciConfigVariables = []; + } + this.stopPollingAndPopulateForm(); + }, CI_VARIABLES_MAX_POLLING_TIME); + }, + stopPollingAndPopulateForm() { + this.clearTimeouts(); + this.$apollo.queries.ciConfigVariables.stopPolling(); + this.populateForm(); + }, }, }; </script> @@ -280,109 +288,109 @@ export default { <template> <div> <h4>{{ s__('Pipeline|Variables') }}</h4> - <inputs-adoption-banner v-if="isPipelineInputsFeatureAvailable" /> <gl-loading-icon v-if="isLoading" class="gl-mb-5" size="md" /> - <gl-form-group v-else class="gl-mb-0"> - <div - v-for="(variable, index) in variables" - :key="variable.uniqueId" - class="gl-mb-4" - data-testid="ci-variable-row-container" - > - <div class="gl-flex gl-flex-col gl-items-stretch gl-gap-4 md:gl-flex-row"> - <gl-collapsible-listbox - :items="variableTypeListboxItems" - :selected="variable.variableType" - block - fluid-width - :aria-label="getPipelineAriaLabel(index)" - :class="$options.formElementClasses" - data-testid="pipeline-form-ci-variable-type" - @select="setVariableAttribute(variable.key, 'variableType', $event)" - /> - <gl-form-input - v-model="variable.key" - :placeholder="s__('CiVariables|Input variable key')" - :aria-label="s__('CiVariables|Input variable key')" - :class="$options.formElementClasses" - data-testid="pipeline-form-ci-variable-key-field" - @change="addEmptyVariable(refParam)" - /> - <variable-values-listbox - v-if="shouldShowValuesDropdown(variable.key)" - :items="createListItemsFromVariableOptions(variable.key)" - :selected="variable.value" - :class="$options.formElementClasses" - class="!gl-mr-0 gl-grow" - data-testid="pipeline-form-ci-variable-value-dropdown" - @select="setVariableAttribute(variable.key, 'value', $event)" - /> - <gl-form-textarea - v-else - v-model="variable.value" - :placeholder="s__('CiVariables|Input variable value')" - :aria-label="s__('CiVariables|Input variable value')" - :style="$options.textAreaStyle" - :no-resize="false" - data-testid="pipeline-form-ci-variable-value-field" - /> - <template v-if="variables.length > 1"> - <gl-button - v-if="canRemove(index)" - size="small" - class="gl-shrink-0" - data-testid="remove-ci-variable-row" - :category="removeButtonCategory" - :aria-label="s__('CiVariables|Remove variable')" - @click="removeVariable(index)" - > - <gl-icon class="!gl-mr-0" name="remove" /> - <span class="md:gl-hidden">{{ s__('CiVariables|Remove variable') }}</span> - </gl-button> - <gl-button + <div v-else> + <inputs-adoption-banner v-if="isPipelineInputsFeatureAvailable" /> + <gl-form-group class="gl-mb-0"> + <div + v-for="(variable, index) in variables" + :key="variable.uniqueId" + class="gl-mb-4" + data-testid="ci-variable-row-container" + > + <div class="gl-flex gl-flex-col gl-items-stretch gl-gap-4 md:gl-flex-row"> + <gl-collapsible-listbox + :items="variableTypeListboxItems" + :selected="variable.variableType" + block + fluid-width + :aria-label="getPipelineAriaLabel(index)" + :class="$options.formElementClasses" + data-testid="pipeline-form-ci-variable-type" + @select="setVariableAttribute(variable.key, 'variableType', $event)" + /> + <gl-form-input + v-model="variable.key" + :placeholder="s__('CiVariables|Input variable key')" + :class="$options.formElementClasses" + data-testid="pipeline-form-ci-variable-key-field" + @change="addEmptyVariable(refParam)" + /> + <variable-values-listbox + v-if="shouldShowValuesDropdown(variable.key)" + :items="createListItemsFromVariableOptions(variable.key)" + :selected="variable.value" + :class="$options.formElementClasses" + class="!gl-mr-0 gl-grow" + data-testid="pipeline-form-ci-variable-value-dropdown" + @select="setVariableAttribute(variable.key, 'value', $event)" + /> + <gl-form-textarea v-else - class="gl-invisible gl-hidden gl-shrink-0 md:gl-block" - icon="remove" - :aria-label="s__('CiVariables|Remove variable')" + v-model="variable.value" + class="!gl-h-7" + :placeholder="s__('CiVariables|Input variable value')" + :no-resize="false" + data-testid="pipeline-form-ci-variable-value-field" /> - </template> - </div> - <div v-if="descriptions[variable.key]" class="gl-text-subtle"> - {{ descriptions[variable.key] }} + <template v-if="variables.length > 1"> + <gl-button + v-if="canRemove(index)" + size="small" + class="gl-shrink-0" + data-testid="remove-ci-variable-row" + :category="removeButtonCategory" + :aria-label="s__('CiVariables|Remove variable')" + @click="removeVariable(index)" + > + <gl-icon class="!gl-mr-0" name="remove" /> + <span class="md:gl-hidden">{{ s__('CiVariables|Remove variable') }}</span> + </gl-button> + <gl-button + v-else + class="gl-invisible gl-hidden gl-shrink-0 md:gl-block" + icon="remove" + :aria-label="s__('CiVariables|Remove variable')" + /> + </template> + </div> + <div v-if="descriptions[variable.key]" class="gl-text-subtle"> + {{ descriptions[variable.key] }} + </div> </div> - </div> - <template #description> - <gl-sprintf - :message=" - s__( - 'Pipeline|Specify variable values to be used in this run. The variables specified in the configuration file as well as %{linkStart}CI/CD settings%{linkEnd} are used by default.', - ) - " - > - <template #link="{ content }"> - <gl-link v-if="isMaintainer" :href="settingsLink" data-testid="ci-cd-settings-link"> - {{ content }} - </gl-link> - <template v-else>{{ content }}</template> - </template> - </gl-sprintf> - <gl-link :href="$options.learnMorePath" target="_blank"> - {{ __('Learn more') }} - </gl-link> - <div class="gl-mt-4 gl-text-subtle"> + <template #description> <gl-sprintf :message=" s__( - 'CiVariables|Variables specified here are %{boldStart}expanded%{boldEnd} and not %{boldStart}masked.%{boldEnd}', + 'Pipeline|Specify variable values to be used in this run. The variables specified in the configuration file as well as %{linkStart}CI/CD settings%{linkEnd} are used by default.', ) " > - <template #bold="{ content }"> - <strong>{{ content }}</strong> + <template #link="{ content }"> + <gl-link v-if="isMaintainer" :href="settingsLink" data-testid="ci-cd-settings-link"> + {{ content }} + </gl-link> + <template v-else>{{ content }}</template> </template> </gl-sprintf> - </div> - </template> - </gl-form-group> + <gl-link :href="$options.learnMorePath" target="_blank"> + {{ __('Learn more') }} + </gl-link> + <div class="gl-mt-4 gl-text-subtle"> + <gl-sprintf + :message=" + s__( + 'CiVariables|Variables specified here are %{boldStart}expanded%{boldEnd} and not %{boldStart}masked.%{boldEnd}', + ) + " + > + <template #bold="{ content }"> + <strong>{{ content }}</strong> + </template> + </gl-sprintf> + </div> + </template> + </gl-form-group> + </div> </div> </template> diff --git a/app/assets/javascripts/ci/pipeline_new/constants.js b/app/assets/javascripts/ci/pipeline_new/constants.js index 9f23f9c93b9701f18dd9440dd56fd9ca1a7ce47e..2a2aae3a8fdc7946d24d03fe8653cff27097fe3c 100644 --- a/app/assets/javascripts/ci/pipeline_new/constants.js +++ b/app/assets/javascripts/ci/pipeline_new/constants.js @@ -1,15 +1,8 @@ -import { __ } from '~/locale'; -import { DEFAULT_DEBOUNCE_AND_THROTTLE_MS } from '~/lib/utils/constants'; - -export const VARIABLE_TYPE = 'env_var'; -export const FILE_TYPE = 'file'; +export const CI_VARIABLES_POLLING_INTERVAL = 2000; +export const CI_VARIABLES_MAX_POLLING_TIME = 10000; export const CI_VARIABLE_TYPE_ENV_VAR = 'ENV_VAR'; export const CI_VARIABLE_TYPE_FILE = 'FILE'; -export const DEBOUNCE_REFS_SEARCH_MS = DEFAULT_DEBOUNCE_AND_THROTTLE_MS; -export const CONFIG_VARIABLES_TIMEOUT = 5000; +export const VARIABLE_TYPE = 'env_var'; +export const FILE_TYPE = 'file'; export const BRANCH_REF_TYPE = 'branch'; export const TAG_REF_TYPE = 'tag'; - -export const IDENTITY_VERIFICATION_REQUIRED_ERROR = __( - 'Identity verification is required in order to run CI jobs', -); diff --git a/spec/frontend/ci/pipeline_new/components/pipeline_variables_form_spec.js b/spec/frontend/ci/pipeline_new/components/pipeline_variables_form_spec.js index beb4eac5880b05d4fe283946e167ac60871c74fd..2c8a474668da1f9f2969ab768ccb2c4adc2f7848 100644 --- a/spec/frontend/ci/pipeline_new/components/pipeline_variables_form_spec.js +++ b/spec/frontend/ci/pipeline_new/components/pipeline_variables_form_spec.js @@ -1,7 +1,7 @@ import { GlFormGroup, GlLoadingIcon } from '@gitlab/ui'; import { GlBreakpointInstance } from '@gitlab/ui/dist/utils'; import VueApollo from 'vue-apollo'; -import Vue from 'vue'; +import Vue, { nextTick } from 'vue'; import { shallowMountExtended } from 'helpers/vue_test_utils_helper'; import createMockApollo from 'helpers/mock_apollo_helper'; import waitForPromises from 'helpers/wait_for_promises'; @@ -98,8 +98,8 @@ describe('PipelineVariablesForm', () => { describe('Feature flag', () => { describe('when the ciInputsForPipelines flag is disabled', () => { - beforeEach(() => { - createComponent(); + beforeEach(async () => { + await createComponent(); }); it('does not display the inputs adoption banner', () => { @@ -108,8 +108,8 @@ describe('PipelineVariablesForm', () => { }); describe('when the ciInputsForPipelines flag is enabled', () => { - beforeEach(() => { - createComponent({ ciInputsForPipelines: true }); + beforeEach(async () => { + await createComponent({ ciInputsForPipelines: true }); }); it('displays the inputs adoption banner', () => { @@ -119,18 +119,35 @@ describe('PipelineVariablesForm', () => { }); describe('loading states', () => { - it('is loading when query is in flight', () => { + it('shows loading when ciConfigVariables is null', () => { createComponent(); expect(findLoadingIcon().exists()).toBe(true); expect(findForm().exists()).toBe(false); }); - it('is not loading after query completes', async () => { + it('hides loading after data is received', async () => { await createComponent(); expect(findLoadingIcon().exists()).toBe(false); expect(findForm().exists()).toBe(true); + expect(wrapper.vm.isFetchingCiConfigVariables).toBe(false); + }); + + it('hides loading when polling reaches max time', async () => { + mockCiConfigVariables = jest.fn().mockResolvedValue({ + data: { project: { ciConfigVariables: null } }, + }); + + await createComponent(); + + // Simulate max polling timeout + jest.advanceTimersByTime(10000); + await nextTick(); + + expect(findLoadingIcon().exists()).toBe(false); + expect(findForm().exists()).toBe(true); + expect(wrapper.vm.ciConfigVariables).toEqual([]); }); }); @@ -195,6 +212,45 @@ describe('PipelineVariablesForm', () => { }); }); + describe('polling behavior', () => { + it('configures Apollo with the correct polling interval', () => { + expect(PipelineVariablesForm.apollo.ciConfigVariables.pollInterval).toBe(2000); + }); + + it('refetches and updates on ref change', async () => { + await createComponent(); + + wrapper.setProps({ refParam: 'new-ref-param' }); + await nextTick(); + + expect(wrapper.vm.ciConfigVariables).toBe(null); + }); + + it('sets ciConfigVariables to empty array on query error', async () => { + await createComponent(); + + const error = new Error('GraphQL error'); + wrapper.vm.$options.apollo.ciConfigVariables.error.call(wrapper.vm, error); + + expect(wrapper.vm.ciConfigVariables).toEqual([]); + expect(reportToSentry).toHaveBeenCalledWith('PipelineVariablesForm', error); + }); + + it('stops polling when data is received', async () => { + await createComponent({ configVariables: configVariablesWithOptions }); + + const stopPollingSpy = jest.spyOn( + wrapper.vm.$apollo.queries.ciConfigVariables, + 'stopPolling', + ); + + const mockData = { data: { project: { ciConfigVariables: configVariablesWithOptions } } }; + wrapper.vm.$options.apollo.ciConfigVariables.result.call(wrapper.vm, mockData); + + expect(stopPollingSpy).toHaveBeenCalled(); + }); + }); + describe('variable rows', () => { it('emits variables-updated event when variables change', async () => { await createComponent();