From d7e204ec849a263cebd5bd2b133445e3d747ae07 Mon Sep 17 00:00:00 2001 From: GitLab Bot Date: Tue, 3 Nov 2020 06:08:58 +0000 Subject: [PATCH] Add latest changes from gitlab-org/gitlab@master --- .../vue_shared/directives/validation.js | 132 ++++++++++++ .../queue-selector-not-experimental.yml | 5 + .../operations/extra_sidekiq_processes.md | 33 ++- lib/gitlab/sidekiq_cluster/cli.rb | 23 ++- locale/gitlab.pot | 9 +- spec/bin/sidekiq_cluster_spec.rb | 4 + .../vue_shared/directives/validation_spec.js | 132 ++++++++++++ spec/lib/gitlab/sidekiq_cluster/cli_spec.rb | 191 ++++++++++-------- 8 files changed, 411 insertions(+), 118 deletions(-) create mode 100644 app/assets/javascripts/vue_shared/directives/validation.js create mode 100644 changelogs/unreleased/queue-selector-not-experimental.yml create mode 100644 spec/frontend/vue_shared/directives/validation_spec.js diff --git a/app/assets/javascripts/vue_shared/directives/validation.js b/app/assets/javascripts/vue_shared/directives/validation.js new file mode 100644 index 00000000000..09bec78edcc --- /dev/null +++ b/app/assets/javascripts/vue_shared/directives/validation.js @@ -0,0 +1,132 @@ +import { merge } from 'lodash'; +import { s__ } from '~/locale'; + +/** + * Validation messages will take priority based on the property order. + * + * For example: + * { valueMissing: {...}, urlTypeMismatch: {...} } + * + * `valueMissing` will be displayed the user has entered a value + * after that, if the input is not a valid URL then `urlTypeMismatch` will show + */ +const defaultFeedbackMap = { + valueMissing: { + isInvalid: el => el.validity?.valueMissing, + message: s__('Please fill out this field.'), + }, + urlTypeMismatch: { + isInvalid: el => el.type === 'url' && el.validity?.typeMismatch, + message: s__('Please enter a valid URL format, ex: http://www.example.com/home'), + }, +}; + +const getFeedbackForElement = (feedbackMap, el) => + Object.values(feedbackMap).find(f => f.isInvalid(el))?.message || el.validationMessage; + +const focusFirstInvalidInput = e => { + const { target: formEl } = e; + const invalidInput = formEl.querySelector('input:invalid'); + + if (invalidInput) { + invalidInput.focus(); + } +}; + +const isEveryFieldValid = form => Object.values(form.fields).every(({ state }) => state === true); + +const createValidator = (context, feedbackMap) => ({ el, reportInvalidInput = false }) => { + const { form } = context; + const { name } = el; + + if (!name) { + if (process.env.NODE_ENV === 'development') { + // eslint-disable-next-line no-console + console.warn( + '[gitlab] the validation directive requires the given input to have "name" attribute', + ); + } + return; + } + + const formField = form.fields[name]; + const isValid = el.checkValidity(); + + // This makes sure we always report valid fields - this can be useful for cases where the consuming + // component's logic depends on certain fields being in a valid state. + // Invalid input, on the other hand, should only be reported once we want to display feedback to the user. + // (eg.: After a field has been touched and moved away from, a submit-button has been clicked, ...) + formField.state = reportInvalidInput ? isValid : isValid || null; + formField.feedback = reportInvalidInput ? getFeedbackForElement(feedbackMap, el) : ''; + + form.state = isEveryFieldValid(form); +}; + +/** + * Takes an object that allows to add or change custom feedback messages. + * + * The passed in object will be merged with the built-in feedback + * so it is possible to override a built-in message. + * + * @example + * validate({ + * tooLong: { + * check: el => el.validity.tooLong === true, + * message: 'Your custom feedback' + * } + * }) + * + * @example + * validate({ + * valueMissing: { + * message: 'Your custom feedback' + * } + * }) + * + * @param {Object} customFeedbackMap + * @returns {{ inserted: function, update: function }} validateDirective + */ +export default function(customFeedbackMap = {}) { + const feedbackMap = merge(defaultFeedbackMap, customFeedbackMap); + const elDataMap = new WeakMap(); + + return { + inserted(el, binding, { context }) { + const { arg: showGlobalValidation } = binding; + const { form: formEl } = el; + + const validate = createValidator(context, feedbackMap); + const elData = { validate, isTouched: false, isBlurred: false }; + + elDataMap.set(el, elData); + + el.addEventListener('input', function markAsTouched() { + elData.isTouched = true; + // once the element has been marked as touched we can stop listening on the 'input' event + el.removeEventListener('input', markAsTouched); + }); + + el.addEventListener('blur', function markAsBlurred({ target }) { + if (elData.isTouched) { + elData.isBlurred = true; + validate({ el: target, reportInvalidInput: true }); + // this event handler can be removed, since the live-feedback in `update` takes over + el.removeEventListener('blur', markAsBlurred); + } + }); + + if (formEl) { + formEl.addEventListener('submit', focusFirstInvalidInput); + } + + validate({ el, reportInvalidInput: showGlobalValidation }); + }, + update(el, binding) { + const { arg: showGlobalValidation } = binding; + const { validate, isTouched, isBlurred } = elDataMap.get(el); + const showValidationFeedback = showGlobalValidation || (isTouched && isBlurred); + + validate({ el, reportInvalidInput: showValidationFeedback }); + }, + }; +} diff --git a/changelogs/unreleased/queue-selector-not-experimental.yml b/changelogs/unreleased/queue-selector-not-experimental.yml new file mode 100644 index 00000000000..bf0a9dd3571 --- /dev/null +++ b/changelogs/unreleased/queue-selector-not-experimental.yml @@ -0,0 +1,5 @@ +--- +title: Mark Sidekiq queue selector as no longer experimental +merge_request: 46562 +author: +type: changed diff --git a/doc/administration/operations/extra_sidekiq_processes.md b/doc/administration/operations/extra_sidekiq_processes.md index 67e0c6bb178..eed92d32ec9 100644 --- a/doc/administration/operations/extra_sidekiq_processes.md +++ b/doc/administration/operations/extra_sidekiq_processes.md @@ -110,29 +110,21 @@ you list: sudo gitlab-ctl reconfigure ``` -## Queue selector (experimental) +## Queue selector > - [Introduced](https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/45) in [GitLab Starter](https://about.gitlab.com/pricing/) 12.8. > - [Sidekiq cluster including queue selector moved](https://gitlab.com/groups/gitlab-com/gl-infra/-/epics/181) to GitLab [Core](https://about.gitlab.com/pricing/#self-managed) in GitLab 12.10. +> - [Marked as supported](https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/147) in GitLab [Core](https://about.gitlab.com/pricing/#self-managed) in GitLab 13.6. Renamed from `experimental_queue_selector` to `queue_selector`. -CAUTION: **Caution:** -As this is marked as **experimental**, it is subject to change at any -time, including **breaking backwards compatibility**. This is so that we -can react to changes we need for our GitLab.com deployment. We have a -tracking issue open to [remove the experimental -designation](https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/147) -from this feature; please comment there if you are interested in using -this in your own deployment. - -In addition to selecting queues by name, as above, the -`experimental_queue_selector` option allows queue groups to be selected -in a more general way using the following components: +In addition to selecting queues by name, as above, the `queue_selector` +option allows queue groups to be selected in a more general way using +the following components: - Attributes that can be selected. - Operators used to construct a query. -When `experimental_queue_selector` is set, all `queue_groups` must be in -the queue selector syntax. +When `queue_selector` is set, all `queue_groups` must be in the queue +selector syntax. ### Available attributes @@ -140,8 +132,7 @@ the queue selector syntax. From the [list of all available attributes](https://gitlab.com/gitlab-org/gitlab/-/blob/master/app/workers/all_queues.yml), -`experimental_queue_selector` allows selecting of queues by the -following attributes: +`queue_selector` allows selecting of queues by the following attributes: - `feature_category` - the [GitLab feature category](https://about.gitlab.com/direction/maturity/#category-maturity) the @@ -173,8 +164,8 @@ neither of those tags. ### Available operators -`experimental_queue_selector` supports the following operators, listed -from highest to lowest precedence: +`queue_selector` supports the following operators, listed from highest +to lowest precedence: - `|` - the logical OR operator. For example, `query_a|query_b` (where `query_a` and `query_b` are queries made up of the other operators here) will include @@ -205,7 +196,7 @@ In `/etc/gitlab/gitlab.rb`: ```ruby sidekiq['enable'] = true -sidekiq['experimental_queue_selector'] = true +sidekiq['queue_selector'] = true sidekiq['queue_groups'] = [ # Run all non-CPU-bound queues that are high urgency 'resource_boundary!=cpu&urgency=high', @@ -234,7 +225,7 @@ All of the aforementioned configuration options for `sidekiq` are available. By default, they will be configured as follows: ```ruby -sidekiq['experimental_queue_selector'] = false +sidekiq['queue_selector'] = false sidekiq['interval'] = nil sidekiq['max_concurrency'] = 50 sidekiq['min_concurrency'] = nil diff --git a/lib/gitlab/sidekiq_cluster/cli.rb b/lib/gitlab/sidekiq_cluster/cli.rb index 1e5d23a8405..e471517c50a 100644 --- a/lib/gitlab/sidekiq_cluster/cli.rb +++ b/lib/gitlab/sidekiq_cluster/cli.rb @@ -47,16 +47,24 @@ module Gitlab option_parser.parse!(argv) + # Remove with https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/646 + if @queue_selector && @experimental_queue_selector + raise CommandError, + 'You cannot specify --queue-selector and --experimental-queue-selector together' + end + all_queues = SidekiqConfig::CliMethods.all_queues(@rails_path) queue_names = SidekiqConfig::CliMethods.worker_queues(@rails_path) queue_groups = argv.map do |queues| next queue_names if queues == '*' - # When using the experimental queue query syntax, we treat - # each queue group as a worker attribute query, and resolve - # the queues for the queue group using this query. - if @experimental_queue_selector + # When using the queue query syntax, we treat each queue group + # as a worker attribute query, and resolve the queues for the + # queue group using this query. + + # Simplify with https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/646 + if @queue_selector || @experimental_queue_selector SidekiqConfig::CliMethods.query_workers(queues, all_queues) else SidekiqConfig::CliMethods.expand_queues(queues.split(','), queue_names) @@ -182,7 +190,12 @@ module Gitlab @rails_path = path end - opt.on('--experimental-queue-selector', 'EXPERIMENTAL: Run workers based on the provided selector') do |experimental_queue_selector| + opt.on('--queue-selector', 'Run workers based on the provided selector') do |queue_selector| + @queue_selector = queue_selector + end + + # Remove with https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/646 + opt.on('--experimental-queue-selector', 'DEPRECATED: use --queue-selector-instead') do |experimental_queue_selector| @experimental_queue_selector = experimental_queue_selector end diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 11367498406..47e0851f3de 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -8352,9 +8352,6 @@ msgstr "" msgid "DastProfiles|Passive" msgstr "" -msgid "DastProfiles|Please enter a valid URL format, ex: http://www.example.com/home" -msgstr "" - msgid "DastProfiles|Please enter a valid timeout value" msgstr "" @@ -19893,6 +19890,9 @@ msgstr "" msgid "Please enter a number greater than %{number} (from the project settings)" msgstr "" +msgid "Please enter a valid URL format, ex: http://www.example.com/home" +msgstr "" + msgid "Please enter a valid number" msgstr "" @@ -19902,6 +19902,9 @@ msgstr "" msgid "Please fill in a descriptive name for your group." msgstr "" +msgid "Please fill out this field." +msgstr "" + msgid "Please follow the %{link_start}Let's Encrypt troubleshooting instructions%{link_end} to re-obtain your Let's Encrypt certificate." msgstr "" diff --git a/spec/bin/sidekiq_cluster_spec.rb b/spec/bin/sidekiq_cluster_spec.rb index fc5e2ae861a..503cc0999c5 100644 --- a/spec/bin/sidekiq_cluster_spec.rb +++ b/spec/bin/sidekiq_cluster_spec.rb @@ -9,6 +9,8 @@ RSpec.describe 'bin/sidekiq-cluster' do context 'when selecting some queues and excluding others' do where(:args, :included, :excluded) do %w[--negate cronjob] | '-qdefault,1' | '-qcronjob,1' + %w[--queue-selector resource_boundary=cpu] | '-qupdate_merge_requests,1' | '-qdefault,1' + # Remove with https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/646 %w[--experimental-queue-selector resource_boundary=cpu] | '-qupdate_merge_requests,1' | '-qdefault,1' end @@ -29,6 +31,8 @@ RSpec.describe 'bin/sidekiq-cluster' do context 'when selecting all queues' do [ %w[*], + %w[--queue-selector *], + # Remove with https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/646 %w[--experimental-queue-selector *] ].each do |args| it "runs successfully with `#{args}`", :aggregate_failures do diff --git a/spec/frontend/vue_shared/directives/validation_spec.js b/spec/frontend/vue_shared/directives/validation_spec.js new file mode 100644 index 00000000000..814d6f43589 --- /dev/null +++ b/spec/frontend/vue_shared/directives/validation_spec.js @@ -0,0 +1,132 @@ +import { shallowMount } from '@vue/test-utils'; +import validation from '~/vue_shared/directives/validation'; + +describe('validation directive', () => { + let wrapper; + + const createComponent = ({ inputAttributes, showValidation } = {}) => { + const defaultInputAttributes = { + type: 'text', + required: true, + }; + + const component = { + directives: { + validation: validation(), + }, + data() { + return { + attributes: inputAttributes || defaultInputAttributes, + showValidation, + form: { + state: null, + fields: { + exampleField: { + state: null, + feedback: '', + }, + }, + }, + }; + }, + template: ` +
+ +
+ `, + }; + + wrapper = shallowMount(component, { attachToDocument: true }); + }; + + afterEach(() => { + wrapper.destroy(); + wrapper = null; + }); + + const getFormData = () => wrapper.vm.form; + const findForm = () => wrapper.find('form'); + const findInput = () => wrapper.find('input'); + + describe.each([true, false])( + 'with fields untouched and "showValidation" set to "%s"', + showValidation => { + beforeEach(() => { + createComponent({ showValidation }); + }); + + it('sets the fields validity correctly', () => { + expect(getFormData().fields.exampleField).toEqual({ + state: showValidation ? false : null, + feedback: showValidation ? expect.any(String) : '', + }); + }); + + it('sets the form validity correctly', () => { + expect(getFormData().state).toBe(false); + }); + }, + ); + + describe.each` + inputAttributes | validValue | invalidValue + ${{ required: true }} | ${'foo'} | ${''} + ${{ type: 'url' }} | ${'http://foo.com'} | ${'foo'} + ${{ type: 'number', min: 1, max: 5 }} | ${3} | ${0} + ${{ type: 'number', min: 1, max: 5 }} | ${3} | ${6} + ${{ pattern: 'foo|bar' }} | ${'bar'} | ${'quz'} + `( + 'with input-attributes set to $inputAttributes', + ({ inputAttributes, validValue, invalidValue }) => { + const setValueAndTriggerValidation = value => { + const input = findInput(); + input.setValue(value); + input.trigger('blur'); + }; + + beforeEach(() => { + createComponent({ inputAttributes }); + }); + + describe('with valid value', () => { + beforeEach(() => { + setValueAndTriggerValidation(validValue); + }); + + it('sets the field to be valid', () => { + expect(getFormData().fields.exampleField).toEqual({ + state: true, + feedback: '', + }); + }); + + it('sets the form to be valid', () => { + expect(getFormData().state).toBe(true); + }); + }); + + describe('with invalid value', () => { + beforeEach(() => { + setValueAndTriggerValidation(invalidValue); + }); + + it('sets the field to be invalid', () => { + expect(getFormData().fields.exampleField).toEqual({ + state: false, + feedback: expect.any(String), + }); + expect(getFormData().fields.exampleField.feedback.length).toBeGreaterThan(0); + }); + + it('sets the form to be invalid', () => { + expect(getFormData().state).toBe(false); + }); + + it('sets focus on the first invalid input when the form is submitted', () => { + findForm().trigger('submit'); + expect(findInput().element).toBe(document.activeElement); + }); + }); + }, + ); +}); diff --git a/spec/lib/gitlab/sidekiq_cluster/cli_spec.rb b/spec/lib/gitlab/sidekiq_cluster/cli_spec.rb index cf165d1770b..74834fb9014 100644 --- a/spec/lib/gitlab/sidekiq_cluster/cli_spec.rb +++ b/spec/lib/gitlab/sidekiq_cluster/cli_spec.rb @@ -108,101 +108,114 @@ RSpec.describe Gitlab::SidekiqCluster::CLI do end end - context 'with --experimental-queue-selector' do - where do - { - 'memory-bound queues' => { - query: 'resource_boundary=memory', - included_queues: %w(project_export), - excluded_queues: %w(merge) - }, - 'memory- or CPU-bound queues' => { - query: 'resource_boundary=memory,cpu', - included_queues: %w(auto_merge:auto_merge_process project_export), - excluded_queues: %w(merge) - }, - 'high urgency CI queues' => { - query: 'feature_category=continuous_integration&urgency=high', - included_queues: %w(pipeline_cache:expire_job_cache pipeline_cache:expire_pipeline_cache), - excluded_queues: %w(merge) - }, - 'CPU-bound high urgency CI queues' => { - query: 'feature_category=continuous_integration&urgency=high&resource_boundary=cpu', - included_queues: %w(pipeline_cache:expire_pipeline_cache), - excluded_queues: %w(pipeline_cache:expire_job_cache merge) - }, - 'CPU-bound high urgency non-CI queues' => { - query: 'feature_category!=continuous_integration&urgency=high&resource_boundary=cpu', - included_queues: %w(new_issue), - excluded_queues: %w(pipeline_cache:expire_pipeline_cache) - }, - 'CI and SCM queues' => { - query: 'feature_category=continuous_integration|feature_category=source_code_management', - included_queues: %w(pipeline_cache:expire_job_cache merge), - excluded_queues: %w(mailers) - } - } - end - - with_them do - it 'expands queues by attributes' do - expect(Gitlab::SidekiqCluster).to receive(:start) do |queues, opts| - expect(opts).to eq(default_options) - expect(queues.first).to include(*included_queues) - expect(queues.first).not_to include(*excluded_queues) - - [] - end - - cli.run(%W(--experimental-queue-selector #{query})) - end - - it 'works when negated' do - expect(Gitlab::SidekiqCluster).to receive(:start) do |queues, opts| - expect(opts).to eq(default_options) - expect(queues.first).not_to include(*included_queues) - expect(queues.first).to include(*excluded_queues) - - [] - end - - cli.run(%W(--negate --experimental-queue-selector #{query})) - end - end - - it 'expands multiple queue groups correctly' do - expect(Gitlab::SidekiqCluster) - .to receive(:start) - .with([['chat_notification'], ['project_export']], default_options) - .and_return([]) - - cli.run(%w(--experimental-queue-selector feature_category=chatops&has_external_dependencies=true resource_boundary=memory&feature_category=importers)) - end - - it 'allows the special * selector' do - worker_queues = %w(foo bar baz) - - expect(Gitlab::SidekiqConfig::CliMethods) - .to receive(:worker_queues).and_return(worker_queues) - - expect(Gitlab::SidekiqCluster) - .to receive(:start).with([worker_queues], default_options) - - cli.run(%w(--experimental-queue-selector *)) - end - - it 'errors when the selector matches no queues' do + # Remove with https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/646 + context 'with --queue-selector and --experimental-queue-selector' do + it 'errors' do expect(Gitlab::SidekiqCluster).not_to receive(:start) - expect { cli.run(%w(--experimental-queue-selector has_external_dependencies=true&has_external_dependencies=false)) } + expect { cli.run(%w(--queue-selector name=foo --experimental-queue-selector name=bar)) } .to raise_error(described_class::CommandError) end + end - it 'errors on an invalid query multiple queue groups correctly' do - expect(Gitlab::SidekiqCluster).not_to receive(:start) + # Simplify with https://gitlab.com/gitlab-com/gl-infra/scalability/-/issues/646 + ['--queue-selector', '--experimental-queue-selector'].each do |flag| + context "with #{flag}" do + where do + { + 'memory-bound queues' => { + query: 'resource_boundary=memory', + included_queues: %w(project_export), + excluded_queues: %w(merge) + }, + 'memory- or CPU-bound queues' => { + query: 'resource_boundary=memory,cpu', + included_queues: %w(auto_merge:auto_merge_process project_export), + excluded_queues: %w(merge) + }, + 'high urgency CI queues' => { + query: 'feature_category=continuous_integration&urgency=high', + included_queues: %w(pipeline_cache:expire_job_cache pipeline_cache:expire_pipeline_cache), + excluded_queues: %w(merge) + }, + 'CPU-bound high urgency CI queues' => { + query: 'feature_category=continuous_integration&urgency=high&resource_boundary=cpu', + included_queues: %w(pipeline_cache:expire_pipeline_cache), + excluded_queues: %w(pipeline_cache:expire_job_cache merge) + }, + 'CPU-bound high urgency non-CI queues' => { + query: 'feature_category!=continuous_integration&urgency=high&resource_boundary=cpu', + included_queues: %w(new_issue), + excluded_queues: %w(pipeline_cache:expire_pipeline_cache) + }, + 'CI and SCM queues' => { + query: 'feature_category=continuous_integration|feature_category=source_code_management', + included_queues: %w(pipeline_cache:expire_job_cache merge), + excluded_queues: %w(mailers) + } + } + end - expect { cli.run(%w(--experimental-queue-selector unknown_field=chatops)) } - .to raise_error(Gitlab::SidekiqConfig::CliMethods::QueryError) + with_them do + it 'expands queues by attributes' do + expect(Gitlab::SidekiqCluster).to receive(:start) do |queues, opts| + expect(opts).to eq(default_options) + expect(queues.first).to include(*included_queues) + expect(queues.first).not_to include(*excluded_queues) + + [] + end + + cli.run(%W(#{flag} #{query})) + end + + it 'works when negated' do + expect(Gitlab::SidekiqCluster).to receive(:start) do |queues, opts| + expect(opts).to eq(default_options) + expect(queues.first).not_to include(*included_queues) + expect(queues.first).to include(*excluded_queues) + + [] + end + + cli.run(%W(--negate #{flag} #{query})) + end + end + + it 'expands multiple queue groups correctly' do + expect(Gitlab::SidekiqCluster) + .to receive(:start) + .with([['chat_notification'], ['project_export']], default_options) + .and_return([]) + + cli.run(%W(#{flag} feature_category=chatops&has_external_dependencies=true resource_boundary=memory&feature_category=importers)) + end + + it 'allows the special * selector' do + worker_queues = %w(foo bar baz) + + expect(Gitlab::SidekiqConfig::CliMethods) + .to receive(:worker_queues).and_return(worker_queues) + + expect(Gitlab::SidekiqCluster) + .to receive(:start).with([worker_queues], default_options) + + cli.run(%W(#{flag} *)) + end + + it 'errors when the selector matches no queues' do + expect(Gitlab::SidekiqCluster).not_to receive(:start) + + expect { cli.run(%W(#{flag} has_external_dependencies=true&has_external_dependencies=false)) } + .to raise_error(described_class::CommandError) + end + + it 'errors on an invalid query multiple queue groups correctly' do + expect(Gitlab::SidekiqCluster).not_to receive(:start) + + expect { cli.run(%W(#{flag} unknown_field=chatops)) } + .to raise_error(Gitlab::SidekiqConfig::CliMethods::QueryError) + end end end end