summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorGitLab Bot <gitlab-bot@gitlab.com>2021-05-05 09:10:02 +0000
committerGitLab Bot <gitlab-bot@gitlab.com>2021-05-05 09:10:02 +0000
commit1c568d834d0cbe1bbbf558ac9a45940f6dbda37a (patch)
tree0e08971a73035b2ff0b44ef6ff7a2bf48dae7573
parent78bc17257c53bc100a4a1eb07c0fdf032236068f (diff)
downloadgitlab-ce-1c568d834d0cbe1bbbf558ac9a45940f6dbda37a.tar.gz
Add latest changes from gitlab-org/gitlab@master
-rw-r--r--.gitlab/issue_templates/Feature Flag Roll Out.md2
-rw-r--r--.gitlab/merge_request_templates/New End To End Test.md3
-rw-r--r--app/assets/javascripts/runner/components/runner_type_badge.vue45
-rw-r--r--app/assets/javascripts/runner/constants.js11
-rw-r--r--app/assets/javascripts/runner/graphql/get_runner.query.graphql6
-rw-r--r--app/assets/javascripts/runner/runner_details/constants.js3
-rw-r--r--app/assets/javascripts/runner/runner_details/index.js16
-rw-r--r--app/assets/javascripts/runner/runner_details/runner_details_app.vue29
-rw-r--r--app/assets/javascripts/vue_shared/components/sidebar/queries/get_issue_assignees.query.graphql2
-rw-r--r--app/assets/javascripts/vue_shared/components/user_select/user_select.vue46
-rw-r--r--app/models/release.rb7
-rw-r--r--app/views/admin/runners/show.html.haml2
-rw-r--r--app/workers/concerns/limited_capacity/job_tracker.rb42
-rw-r--r--app/workers/concerns/limited_capacity/worker.rb86
-rw-r--r--changelogs/unreleased/cleanup-validate_release_description_length-feature-flag.yml5
-rw-r--r--changelogs/unreleased/kassio-github-importer-add-missing-importes-caches.yml5
-rw-r--r--config/feature_flags/development/validate_release_description_length.yml8
-rw-r--r--doc/development/snowplow/event_dictionary_guide.md1
-rw-r--r--doc/development/usage_ping/dictionary.md24
-rw-r--r--doc/user/compliance/index.md4
-rw-r--r--lib/generators/gitlab/snowplow_event_definition_generator.rb9
-rw-r--r--lib/gitlab/github_import/client.rb2
-rw-r--r--lib/gitlab/github_import/importer/pull_requests_merged_by_importer.rb8
-rw-r--r--lib/gitlab/github_import/importer/pull_requests_reviews_importer.rb19
-rw-r--r--lib/gitlab/usage_data_counters/known_events/epic_events.yml8
-rw-r--r--spec/frontend/runner/components/runner_type_badge_spec.js40
-rw-r--r--spec/frontend/runner/runner_detail/runner_detail_app_spec.js29
-rw-r--r--spec/frontend/runner/runner_detail/runner_details_app_spec.js71
-rw-r--r--spec/frontend/sidebar/mock_data.js77
-rw-r--r--spec/lib/generators/gitlab/snowplow_event_definition_generator_spec.rb24
-rw-r--r--spec/lib/gitlab/github_import/client_spec.rb5
-rw-r--r--spec/lib/gitlab/github_import/importer/pull_requests_merged_by_importer_spec.rb13
-rw-r--r--spec/lib/gitlab/github_import/importer/pull_requests_reviews_importer_spec.rb18
-rw-r--r--spec/models/release_spec.rb12
-rw-r--r--spec/support/helpers/snowplow_helpers.rb8
-rw-r--r--spec/workers/concerns/limited_capacity/job_tracker_spec.rb48
-rw-r--r--spec/workers/concerns/limited_capacity/worker_spec.rb137
37 files changed, 497 insertions, 378 deletions
diff --git a/.gitlab/issue_templates/Feature Flag Roll Out.md b/.gitlab/issue_templates/Feature Flag Roll Out.md
index 6caf6669e16..86083c565fb 100644
--- a/.gitlab/issue_templates/Feature Flag Roll Out.md
+++ b/.gitlab/issue_templates/Feature Flag Roll Out.md
@@ -70,7 +70,7 @@ Are there any other stages or teams involved that need to be kept in the loop?
- [ ] Announce on the issue an estimated time this will be enabled on GitLab.com
-- [ ] Check if the feature flag change needs to be accompagnied with a
+- [ ] Check if the feature flag change needs to be accompanied with a
[change management
issue](https://about.gitlab.com/handbook/engineering/infrastructure/change-management/#feature-flags-and-the-change-management-process). Cross
link the issue here if it does.
diff --git a/.gitlab/merge_request_templates/New End To End Test.md b/.gitlab/merge_request_templates/New End To End Test.md
index 9e6c4049b90..f9664c6315f 100644
--- a/.gitlab/merge_request_templates/New End To End Test.md
+++ b/.gitlab/merge_request_templates/New End To End Test.md
@@ -14,7 +14,8 @@ Please link to the respective test case in the testcases project
- [ ] Ensure that no [transient bugs](https://about.gitlab.com/handbook/engineering/quality/issue-triage/#transient-bugs) are hidden accidentally due to the usage of `waits` and `reloads`.
- [ ] Verify the tags to ensure it runs on the desired test environments.
- [ ] If this MR has a dependency on another MR, such as a GitLab QA MR, specify the order in which the MRs should be merged.
-- [ ] (If applicable) Create a follow-up issue to document [the special setup](https://docs.gitlab.com/ee/development/testing_guide/end_to_end/running_tests_that_require_special_setup.html) necessary to run the test: ISSUE_LINK
+- [ ] (If applicable) Create a follow-up issue to document [the special setup](https://docs.gitlab.com/ee/development/testing_guide/end_to_end/running_tests_that_require_special_setup.html) necessary to run the test: ISSUE_LINK
+- [ ] If the test requires an admin's personal access token, ensure that the test passes on your local with and without the `GITLAB_QA_ADMIN_ACCESS_TOKEN` provided.
<!-- Base labels. -->
/label ~"Quality" ~"QA" ~test
diff --git a/app/assets/javascripts/runner/components/runner_type_badge.vue b/app/assets/javascripts/runner/components/runner_type_badge.vue
new file mode 100644
index 00000000000..dd4fff3a77a
--- /dev/null
+++ b/app/assets/javascripts/runner/components/runner_type_badge.vue
@@ -0,0 +1,45 @@
+<script>
+import { GlBadge } from '@gitlab/ui';
+import { s__ } from '~/locale';
+import { INSTANCE_TYPE, GROUP_TYPE, PROJECT_TYPE } from '../constants';
+
+const badge = {
+ [INSTANCE_TYPE]: {
+ variant: 'success',
+ text: s__('Runners|shared'),
+ },
+ [GROUP_TYPE]: {
+ variant: 'success',
+ text: s__('Runners|group'),
+ },
+ [PROJECT_TYPE]: {
+ variant: 'info',
+ text: s__('Runners|specific'),
+ },
+};
+
+export default {
+ components: {
+ GlBadge,
+ },
+ props: {
+ type: {
+ type: String,
+ required: true,
+ },
+ },
+ computed: {
+ variant() {
+ return badge[this.type]?.variant;
+ },
+ text() {
+ return badge[this.type]?.text;
+ },
+ },
+};
+</script>
+<template>
+ <gl-badge v-if="text" :variant="variant" v-bind="$attrs">
+ {{ text }}
+ </gl-badge>
+</template>
diff --git a/app/assets/javascripts/runner/constants.js b/app/assets/javascripts/runner/constants.js
new file mode 100644
index 00000000000..de3a3fda47e
--- /dev/null
+++ b/app/assets/javascripts/runner/constants.js
@@ -0,0 +1,11 @@
+import { s__ } from '~/locale';
+
+export const I18N_DETAILS_TITLE = s__('Runners|Runner #%{runner_id}');
+
+export const RUNNER_ENTITY_TYPE = 'Ci::Runner';
+
+// CiRunnerType
+
+export const INSTANCE_TYPE = 'INSTANCE_TYPE';
+export const GROUP_TYPE = 'GROUP_TYPE';
+export const PROJECT_TYPE = 'PROJECT_TYPE';
diff --git a/app/assets/javascripts/runner/graphql/get_runner.query.graphql b/app/assets/javascripts/runner/graphql/get_runner.query.graphql
new file mode 100644
index 00000000000..d209313d4df
--- /dev/null
+++ b/app/assets/javascripts/runner/graphql/get_runner.query.graphql
@@ -0,0 +1,6 @@
+query getRunner($id: CiRunnerID!) {
+ runner(id: $id) {
+ id
+ runnerType
+ }
+}
diff --git a/app/assets/javascripts/runner/runner_details/constants.js b/app/assets/javascripts/runner/runner_details/constants.js
deleted file mode 100644
index bb57e85fa8a..00000000000
--- a/app/assets/javascripts/runner/runner_details/constants.js
+++ /dev/null
@@ -1,3 +0,0 @@
-import { s__ } from '~/locale';
-
-export const I18N_TITLE = s__('Runners|Runner #%{runner_id}');
diff --git a/app/assets/javascripts/runner/runner_details/index.js b/app/assets/javascripts/runner/runner_details/index.js
index cbf70640ef7..05e6f86869d 100644
--- a/app/assets/javascripts/runner/runner_details/index.js
+++ b/app/assets/javascripts/runner/runner_details/index.js
@@ -1,7 +1,11 @@
import Vue from 'vue';
+import VueApollo from 'vue-apollo';
+import createDefaultClient from '~/lib/graphql';
import RunnerDetailsApp from './runner_details_app.vue';
-export const initRunnerDetail = (selector = '#js-runner-detail') => {
+Vue.use(VueApollo);
+
+export const initRunnerDetail = (selector = '#js-runner-details') => {
const el = document.querySelector(selector);
if (!el) {
@@ -10,8 +14,18 @@ export const initRunnerDetail = (selector = '#js-runner-detail') => {
const { runnerId } = el.dataset;
+ const apolloProvider = new VueApollo({
+ defaultClient: createDefaultClient(
+ {},
+ {
+ assumeImmutableResults: true,
+ },
+ ),
+ });
+
return new Vue({
el,
+ apolloProvider,
render(h) {
return h(RunnerDetailsApp, {
props: {
diff --git a/app/assets/javascripts/runner/runner_details/runner_details_app.vue b/app/assets/javascripts/runner/runner_details/runner_details_app.vue
index 1b1485bfe72..4736e547cb9 100644
--- a/app/assets/javascripts/runner/runner_details/runner_details_app.vue
+++ b/app/assets/javascripts/runner/runner_details/runner_details_app.vue
@@ -1,9 +1,15 @@
<script>
-import { I18N_TITLE } from './constants';
+import { convertToGraphQLId } from '~/graphql_shared/utils';
+import RunnerTypeBadge from '../components/runner_type_badge.vue';
+import { I18N_DETAILS_TITLE, RUNNER_ENTITY_TYPE } from '../constants';
+import getRunnerQuery from '../graphql/get_runner.query.graphql';
export default {
+ components: {
+ RunnerTypeBadge,
+ },
i18n: {
- I18N_TITLE,
+ I18N_DETAILS_TITLE,
},
props: {
runnerId: {
@@ -11,10 +17,27 @@ export default {
required: true,
},
},
+ data() {
+ return {
+ runner: {},
+ };
+ },
+ apollo: {
+ runner: {
+ query: getRunnerQuery,
+ variables() {
+ return {
+ id: convertToGraphQLId(RUNNER_ENTITY_TYPE, this.runnerId),
+ };
+ },
+ },
+ },
};
</script>
<template>
<h2 class="page-title">
- {{ sprintf($options.i18n.I18N_TITLE, { runner_id: runnerId }) }}
+ {{ sprintf($options.i18n.I18N_DETAILS_TITLE, { runner_id: runnerId }) }}
+
+ <runner-type-badge v-if="runner.runnerType" :type="runner.runnerType" />
</h2>
</template>
diff --git a/app/assets/javascripts/vue_shared/components/sidebar/queries/get_issue_assignees.query.graphql b/app/assets/javascripts/vue_shared/components/sidebar/queries/get_issue_assignees.query.graphql
index e3501e53bdc..93b9833bb7d 100644
--- a/app/assets/javascripts/vue_shared/components/sidebar/queries/get_issue_assignees.query.graphql
+++ b/app/assets/javascripts/vue_shared/components/sidebar/queries/get_issue_assignees.query.graphql
@@ -1,7 +1,7 @@
#import "~/graphql_shared/fragments/user.fragment.graphql"
#import "~/graphql_shared/fragments/user_availability.fragment.graphql"
-query issueParticipants($fullPath: ID!, $iid: String!) {
+query issueAssignees($fullPath: ID!, $iid: String!) {
workspace: project(fullPath: $fullPath) {
__typename
issuable: issue(iid: $iid) {
diff --git a/app/assets/javascripts/vue_shared/components/user_select/user_select.vue b/app/assets/javascripts/vue_shared/components/user_select/user_select.vue
index 22b8d707de8..c570ea09da3 100644
--- a/app/assets/javascripts/vue_shared/components/user_select/user_select.vue
+++ b/app/assets/javascripts/vue_shared/components/user_select/user_select.vue
@@ -88,6 +88,9 @@ export default {
},
},
searchUsers: {
+ // TODO Remove error policy
+ // https://gitlab.com/gitlab-org/gitlab/-/issues/329750
+ errorPolicy: 'all',
query: searchUsers,
variables() {
return {
@@ -97,10 +100,26 @@ export default {
};
},
update(data) {
- return data.workspace?.users?.nodes.map(({ user }) => user) || [];
+ // TODO Remove null filter (BE fix required)
+ // https://gitlab.com/gitlab-org/gitlab/-/issues/329750
+ return data.workspace?.users?.nodes.filter((x) => x).map(({ user }) => user) || [];
},
debounce: ASSIGNEES_DEBOUNCE_DELAY,
- error() {
+ error({ graphQLErrors }) {
+ // TODO This error suppression is temporary (BE fix required)
+ // https://gitlab.com/gitlab-org/gitlab/-/issues/329750
+ if (
+ graphQLErrors.length === 1 &&
+ graphQLErrors[0]?.message === 'Cannot return null for non-nullable field GroupMember.user'
+ ) {
+ // eslint-disable-next-line no-console
+ console.error(
+ "Suppressing the error 'Cannot return null for non-nullable field GroupMember.user'. Please see https://gitlab.com/gitlab-org/gitlab/-/issues/329750",
+ );
+ this.isSearching = false;
+ return;
+ }
+
this.$emit('error');
this.isSearching = false;
},
@@ -117,15 +136,20 @@ export default {
if (!this.participants) {
return [];
}
- const mergedSearchResults = this.participants.reduce((acc, current) => {
- if (
- !acc.some((user) => current.username === user.username) &&
- (current.name.includes(this.search) || current.username.includes(this.search))
- ) {
- acc.push(current);
- }
- return acc;
- }, this.searchUsers);
+
+ const filteredParticipants = this.participants.filter(
+ (user) => user.name.includes(this.search) || user.username.includes(this.search),
+ );
+
+ // TODO this de-duplication is temporary (BE fix required)
+ // https://gitlab.com/gitlab-org/gitlab/-/issues/327822
+ const mergedSearchResults = filteredParticipants
+ .concat(this.searchUsers)
+ .reduce(
+ (acc, current) => (acc.some((user) => current.id === user.id) ? acc : [...acc, current]),
+ [],
+ );
+
return this.moveCurrentUserToStart(mergedSearchResults);
},
isSearchEmpty() {
diff --git a/app/models/release.rb b/app/models/release.rb
index e18cac4a69a..cf90b07c2a5 100644
--- a/app/models/release.rb
+++ b/app/models/release.rb
@@ -24,7 +24,7 @@ class Release < ApplicationRecord
before_create :set_released_at
validates :project, :tag, presence: true
- validates :description, length: { maximum: Gitlab::Database::MAX_TEXT_SIZE_LIMIT }, if: :should_validate_description_length?
+ validates :description, length: { maximum: Gitlab::Database::MAX_TEXT_SIZE_LIMIT }, if: :description_changed?
validates_associated :milestone_releases, message: -> (_, obj) { obj[:value].map(&:errors).map(&:full_messages).join(",") }
validates :links, nested_attributes_duplicates: { scope: :release, child_attributes: %i[name url filepath] }
@@ -102,11 +102,6 @@ class Release < ApplicationRecord
private
- def should_validate_description_length?
- description_changed? &&
- ::Feature.enabled?(:validate_release_description_length, project, default_enabled: :yaml)
- end
-
def actual_sha
sha || actual_tag&.dereferenced_target
end
diff --git a/app/views/admin/runners/show.html.haml b/app/views/admin/runners/show.html.haml
index 3c9ee63c7e3..d911f35d946 100644
--- a/app/views/admin/runners/show.html.haml
+++ b/app/views/admin/runners/show.html.haml
@@ -5,7 +5,7 @@
- add_to_breadcrumbs _('Runners'), admin_runners_path
- if Feature.enabled?(:runner_detailed_view_vue_ui, current_user, default_enabled: :yaml)
- #js-runner-detail{ data: {runner_id: @runner.id} }
+ #js-runner-details{ data: {runner_id: @runner.id} }
- else
%h2.page-title
= s_('Runners|Runner #%{runner_id}' % { runner_id: @runner.id })
diff --git a/app/workers/concerns/limited_capacity/job_tracker.rb b/app/workers/concerns/limited_capacity/job_tracker.rb
index 96b6e1a2024..47b13cd5bf6 100644
--- a/app/workers/concerns/limited_capacity/job_tracker.rb
+++ b/app/workers/concerns/limited_capacity/job_tracker.rb
@@ -3,21 +3,30 @@ module LimitedCapacity
class JobTracker # rubocop:disable Scalability/IdempotentWorker
include Gitlab::Utils::StrongMemoize
+ LUA_REGISTER_SCRIPT = <<~EOS
+ local set_key, element, max_elements = KEYS[1], ARGV[1], ARGV[2]
+
+ if redis.call("scard", set_key) < tonumber(max_elements) then
+ redis.call("sadd", set_key, element)
+ return true
+ end
+
+ return false
+ EOS
+
def initialize(namespace)
@namespace = namespace
end
- def register(jid)
- _added, @count = with_redis_pipeline do |redis|
- register_job_keys(redis, jid)
- get_job_count(redis)
- end
+ def register(jid, max_jids)
+ with_redis do |redis|
+ redis.eval(LUA_REGISTER_SCRIPT, keys: [counter_key], argv: [jid, max_jids])
+ end.present?
end
def remove(jid)
- _removed, @count = with_redis_pipeline do |redis|
+ with_redis do |redis|
remove_job_keys(redis, jid)
- get_job_count(redis)
end
end
@@ -25,14 +34,13 @@ module LimitedCapacity
completed_jids = Gitlab::SidekiqStatus.completed_jids(running_jids)
return unless completed_jids.any?
- _removed, @count = with_redis_pipeline do |redis|
+ with_redis do |redis|
remove_job_keys(redis, completed_jids)
- get_job_count(redis)
end
end
def count
- @count ||= with_redis { |redis| get_job_count(redis) }
+ with_redis { |redis| redis.scard(counter_key) }
end
def running_jids
@@ -49,14 +57,6 @@ module LimitedCapacity
"worker:#{namespace.to_s.underscore}:running"
end
- def get_job_count(redis)
- redis.scard(counter_key)
- end
-
- def register_job_keys(redis, keys)
- redis.sadd(counter_key, keys)
- end
-
def remove_job_keys(redis, keys)
redis.srem(counter_key, keys)
end
@@ -64,11 +64,5 @@ module LimitedCapacity
def with_redis(&block)
Gitlab::Redis::Queues.with(&block) # rubocop: disable CodeReuse/ActiveRecord
end
-
- def with_redis_pipeline(&block)
- with_redis do |redis|
- redis.pipelined(&block)
- end
- end
end
end
diff --git a/app/workers/concerns/limited_capacity/worker.rb b/app/workers/concerns/limited_capacity/worker.rb
index 863f9063a4b..b4cdfda680f 100644
--- a/app/workers/concerns/limited_capacity/worker.rb
+++ b/app/workers/concerns/limited_capacity/worker.rb
@@ -55,26 +55,14 @@ module LimitedCapacity
def perform_with_capacity(*args)
worker = self.new
worker.remove_failed_jobs
- worker.report_prometheus_metrics(*args)
- required_jobs_count = worker.required_jobs_count(*args)
- arguments = Array.new(required_jobs_count) { args }
+ arguments = Array.new(worker.max_running_jobs) { args }
self.bulk_perform_async(arguments) # rubocop:disable Scalability/BulkPerformWithContext
end
end
def perform(*args)
- return unless has_capacity?
-
- job_tracker.register(jid)
- report_running_jobs_metrics
- perform_work(*args)
- rescue StandardError => exception
- raise
- ensure
- job_tracker.remove(jid)
- report_prometheus_metrics(*args)
- re_enqueue(*args) unless exception
+ perform_registered(*args) if job_tracker.register(jid, max_running_jobs)
end
def perform_work(*args)
@@ -89,43 +77,32 @@ module LimitedCapacity
raise NotImplementedError
end
- def has_capacity?
- remaining_capacity > 0
- end
-
- def remaining_capacity
- [
- max_running_jobs - running_jobs_count - self.class.queue_size,
- 0
- ].max
- end
-
- def has_work?(*args)
- remaining_work_count(*args) > 0
- end
-
def remove_failed_jobs
job_tracker.clean_up
end
def report_prometheus_metrics(*args)
report_running_jobs_metrics
- remaining_work_gauge.set(prometheus_labels, remaining_work_count(*args))
- max_running_jobs_gauge.set(prometheus_labels, max_running_jobs)
+ set_metric(:remaining_work_gauge, remaining_work_count(*args))
+ set_metric(:max_running_jobs_gauge, max_running_jobs)
end
- def report_running_jobs_metrics
- running_jobs_gauge.set(prometheus_labels, running_jobs_count)
- end
+ private
- def required_jobs_count(*args)
- [
- remaining_work_count(*args),
- remaining_capacity
- ].min
+ def perform_registered(*args)
+ report_running_jobs_metrics
+ perform_work(*args)
+ rescue StandardError => exception
+ raise
+ ensure
+ job_tracker.remove(jid)
+ report_prometheus_metrics(*args)
+ re_enqueue(*args) unless exception
end
- private
+ def report_running_jobs_metrics
+ set_metric(:running_jobs_gauge, running_jobs_count)
+ end
def running_jobs_count
job_tracker.count
@@ -138,32 +115,21 @@ module LimitedCapacity
end
def re_enqueue(*args)
- return unless has_capacity?
- return unless has_work?(*args)
+ return unless remaining_work_count(*args) > 0
self.class.perform_async(*args)
end
- def running_jobs_gauge
- strong_memoize(:running_jobs_gauge) do
- Gitlab::Metrics.gauge(:limited_capacity_worker_running_jobs, 'Number of running jobs')
- end
- end
-
- def max_running_jobs_gauge
- strong_memoize(:max_running_jobs_gauge) do
- Gitlab::Metrics.gauge(:limited_capacity_worker_max_running_jobs, 'Maximum number of running jobs')
+ def set_metric(name, value)
+ metrics = strong_memoize(:metrics) do
+ {
+ running_jobs_gauge: Gitlab::Metrics.gauge(:limited_capacity_worker_running_jobs, 'Number of running jobs'),
+ max_running_jobs_gauge: Gitlab::Metrics.gauge(:limited_capacity_worker_max_running_jobs, 'Maximum number of running jobs'),
+ remaining_work_gauge: Gitlab::Metrics.gauge(:limited_capacity_worker_remaining_work_count, 'Number of jobs waiting to be enqueued')
+ }
end
- end
-
- def remaining_work_gauge
- strong_memoize(:remaining_work_gauge) do
- Gitlab::Metrics.gauge(:limited_capacity_worker_remaining_work_count, 'Number of jobs waiting to be enqueued')
- end
- end
- def prometheus_labels
- { worker: self.class.name }
+ metrics[name].set({ worker: self.class.name }, value)
end
end
end
diff --git a/changelogs/unreleased/cleanup-validate_release_description_length-feature-flag.yml b/changelogs/unreleased/cleanup-validate_release_description_length-feature-flag.yml
new file mode 100644
index 00000000000..a6e23099777
--- /dev/null
+++ b/changelogs/unreleased/cleanup-validate_release_description_length-feature-flag.yml
@@ -0,0 +1,5 @@
+---
+title: Validate release description length
+merge_request: 60892
+author:
+type: changed
diff --git a/changelogs/unreleased/kassio-github-importer-add-missing-importes-caches.yml b/changelogs/unreleased/kassio-github-importer-add-missing-importes-caches.yml
new file mode 100644
index 00000000000..d949cba7a2d
--- /dev/null
+++ b/changelogs/unreleased/kassio-github-importer-add-missing-importes-caches.yml
@@ -0,0 +1,5 @@
+---
+title: 'Github Importer: Add Cache to Pull Request Reviews importer'
+merge_request: 60668
+author:
+type: changed
diff --git a/config/feature_flags/development/validate_release_description_length.yml b/config/feature_flags/development/validate_release_description_length.yml
deleted file mode 100644
index 6a7b3a937ca..00000000000
--- a/config/feature_flags/development/validate_release_description_length.yml
+++ /dev/null
@@ -1,8 +0,0 @@
----
-name: validate_release_description_length
-introduced_by_url: https://gitlab.com/gitlab-org/gitlab/-/merge_requests/60380
-rollout_issue_url: https://gitlab.com/gitlab-org/gitlab/-/issues/329192
-milestone: '13.12'
-type: development
-group: group::release
-default_enabled: false
diff --git a/doc/development/snowplow/event_dictionary_guide.md b/doc/development/snowplow/event_dictionary_guide.md
index 86975339cd6..5ae81c3426d 100644
--- a/doc/development/snowplow/event_dictionary_guide.md
+++ b/doc/development/snowplow/event_dictionary_guide.md
@@ -86,6 +86,7 @@ The generator takes three options:
- `--ee`: Indicates if the event is for EE.
- `--category=CATEGORY`: Indicates the `category` of the event.
- `--action=ACTION`: Indicates the `action` of the event.
+- `--force`: Overwrites the existing event definition, if one already exists.
```shell
bundle exec rails generate gitlab:snowplow_event_definition --category Groups::EmailCampaignsController --action click
diff --git a/doc/development/usage_ping/dictionary.md b/doc/development/usage_ping/dictionary.md
index 4d91cd0fbed..e4c5e2806ec 100644
--- a/doc/development/usage_ping/dictionary.md
+++ b/doc/development/usage_ping/dictionary.md
@@ -10460,6 +10460,30 @@ Status: `implemented`
Tiers: `premium`, `ultimate`
+### `redis_hll_counters.epics_usage.g_project_management_users_awarding_epic_emoji_monthly`
+
+Counts of MAU awarding emoji on epic
+
+[YAML definition](https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/config/metrics/counts_28d/20210503011217_g_project_management_users_awarding_epic_emoji_monthly.yml)
+
+Group: `group::product planning`
+
+Status: `implemented`
+
+Tiers: `premium`, `ultimate`
+
+### `redis_hll_counters.epics_usage.g_project_management_users_awarding_epic_emoji_weekly`
+
+Counts of WAU awarding emoji on epic
+
+[YAML definition](https://gitlab.com/gitlab-org/gitlab/-/blob/master/ee/config/metrics/counts_7d/20210503011355_g_project_management_users_awarding_epic_emoji_weekly.yml)
+
+Group: `group::product planning`
+
+Status: `implemented`
+
+Tiers: `premium`, `ultimate`
+
### `redis_hll_counters.epics_usage.g_project_management_users_creating_epic_notes_monthly`
Counts of MAU adding epic notes
diff --git a/doc/user/compliance/index.md b/doc/user/compliance/index.md
index d31f877c418..69deefd08a7 100644
--- a/doc/user/compliance/index.md
+++ b/doc/user/compliance/index.md
@@ -15,3 +15,7 @@ following compliance tools are available:
- [License Compliance](license_compliance/index.md): Search your project's dependencies for their
licenses. This lets you determine if the licenses of your project's dependencies are compatible
with your project's license.
+- [Compliance framework labels](../project/settings/index.md#compliance-frameworks): Label your projects that have unique compliance requirements.
+- [Compliance pipelines](../project/settings/index.md#compliance-pipeline-configuration): Ensure that needed compliance jobs are always run for compliance-labeled projects.
+- [Audit Events](../../administration/audit_events.md): Get visibility into individual actions that have taken place in your GitLab instance, group, or project.
+- [Audit Reports](../../administration/audit_reports.md): Create and access reports based on the audit events that have occurred. Use pre-built GitLab reports or the API to build your own.
diff --git a/lib/generators/gitlab/snowplow_event_definition_generator.rb b/lib/generators/gitlab/snowplow_event_definition_generator.rb
index 95637416522..497d0cd512a 100644
--- a/lib/generators/gitlab/snowplow_event_definition_generator.rb
+++ b/lib/generators/gitlab/snowplow_event_definition_generator.rb
@@ -14,11 +14,12 @@ module Gitlab
class_option :ee, type: :boolean, optional: true, default: false, desc: 'Indicates if event is for ee'
class_option :category, type: :string, optional: false, desc: 'Category of the event'
class_option :action, type: :string, optional: false, desc: 'Action of the event'
+ class_option :force, type: :boolean, optional: true, default: false, desc: 'Overwrite existing definition'
def create_event_file
- raise 'Event definition already exists' if definition_exists?
+ raise "Event definition already exists at #{file_path}" if definition_exists? && !force_definition_override?
- template "event_definition.yml", file_path
+ template "event_definition.yml", file_path, force: force_definition_override?
end
def distributions
@@ -41,6 +42,10 @@ module Gitlab
options[:ee]
end
+ def force_definition_override?
+ options[:force]
+ end
+
private
def definition_exists?
diff --git a/lib/gitlab/github_import/client.rb b/lib/gitlab/github_import/client.rb
index 328f1f742c5..138716b1b53 100644
--- a/lib/gitlab/github_import/client.rb
+++ b/lib/gitlab/github_import/client.rb
@@ -70,7 +70,7 @@ module Gitlab
end
def pull_request_reviews(repo_name, iid)
- with_rate_limit { octokit.pull_request_reviews(repo_name, iid) }
+ each_object(:pull_request_reviews, repo_name, iid)
end
# Returns the details of a GitHub repository.
diff --git a/lib/gitlab/github_import/importer/pull_requests_merged_by_importer.rb b/lib/gitlab/github_import/importer/pull_requests_merged_by_importer.rb
index 466288fde4c..94472cd341e 100644
--- a/lib/gitlab/github_import/importer/pull_requests_merged_by_importer.rb
+++ b/lib/gitlab/github_import/importer/pull_requests_merged_by_importer.rb
@@ -22,14 +22,18 @@ module Gitlab
:pull_requests_merged_by
end
- def id_for_already_imported_cache(pr)
- pr.number
+ def id_for_already_imported_cache(merge_request)
+ merge_request.id
end
def each_object_to_import
project.merge_requests.with_state(:merged).find_each do |merge_request|
+ next if already_imported?(merge_request)
+
pull_request = client.pull_request(project.import_source, merge_request.iid)
yield(pull_request)
+
+ mark_as_imported(merge_request)
end
end
end
diff --git a/lib/gitlab/github_import/importer/pull_requests_reviews_importer.rb b/lib/gitlab/github_import/importer/pull_requests_reviews_importer.rb
index 6d1b588f0e0..827027203ff 100644
--- a/lib/gitlab/github_import/importer/pull_requests_reviews_importer.rb
+++ b/lib/gitlab/github_import/importer/pull_requests_reviews_importer.rb
@@ -22,17 +22,22 @@ module Gitlab
:pull_request_reviews
end
- def id_for_already_imported_cache(review)
- review.github_id
+ def id_for_already_imported_cache(merge_request)
+ merge_request.id
end
def each_object_to_import
project.merge_requests.find_each do |merge_request|
- reviews = client.pull_request_reviews(project.import_source, merge_request.iid)
- reviews.each do |review|
- review.merge_request_id = merge_request.id
- yield(review)
- end
+ next if already_imported?(merge_request)
+
+ client
+ .pull_request_reviews(project.import_source, merge_request.iid)
+ .each do |review|
+ review.merge_request_id = merge_request.id
+ yield(review)
+ end
+
+ mark_as_imported(merge_request)
end
end
end
diff --git a/lib/gitlab/usage_data_counters/known_events/epic_events.yml b/lib/gitlab/usage_data_counters/known_events/epic_events.yml
index 6301292a0b6..4d0ea0a5de7 100644
--- a/lib/gitlab/usage_data_counters/known_events/epic_events.yml
+++ b/lib/gitlab/usage_data_counters/known_events/epic_events.yml
@@ -55,6 +55,14 @@
aggregation: daily
feature_flag: track_epics_activity
+# emoji
+
+- name: g_project_management_users_awarding_epic_emoji
+ category: epics_usage
+ redis_slot: project_management
+ aggregation: daily
+ feature_flag: track_epics_activity
+
# start date events
- name: g_project_management_users_setting_epic_start_date_as_fixed
diff --git a/spec/frontend/runner/components/runner_type_badge_spec.js b/spec/frontend/runner/components/runner_type_badge_spec.js
new file mode 100644
index 00000000000..8e52d3398bd
--- /dev/null
+++ b/spec/frontend/runner/components/runner_type_badge_spec.js
@@ -0,0 +1,40 @@
+import { GlBadge } from '@gitlab/ui';
+import { shallowMount } from '@vue/test-utils';
+import RunnerTypeBadge from '~/runner/components/runner_type_badge.vue';
+import { INSTANCE_TYPE, GROUP_TYPE, PROJECT_TYPE } from '~/runner/constants';
+
+describe('RunnerTypeBadge', () => {
+ let wrapper;
+
+ const findBadge = () => wrapper.findComponent(GlBadge);
+
+ const createComponent = ({ props = {} } = {}) => {
+ wrapper = shallowMount(RunnerTypeBadge, {
+ propsData: {
+ ...props,
+ },
+ });
+ };
+
+ afterEach(() => {
+ wrapper.destroy();
+ });
+
+ it.each`
+ type | text | variant
+ ${INSTANCE_TYPE} | ${'shared'} | ${'success'}
+ ${GROUP_TYPE} | ${'group'} | ${'success'}
+ ${PROJECT_TYPE} | ${'specific'} | ${'info'}
+ `('displays $type runner with as "$text" with a $variant variant ', ({ type, text, variant }) => {
+ createComponent({ props: { type } });
+
+ expect(findBadge().text()).toBe(text);
+ expect(findBadge().props('variant')).toBe(variant);
+ });
+
+ it('does not display a badge when type is unknown', () => {
+ createComponent({ props: { type: 'AN_UNKNOWN_VALUE' } });
+
+ expect(findBadge().exists()).toBe(false);
+ });
+});
diff --git a/spec/frontend/runner/runner_detail/runner_detail_app_spec.js b/spec/frontend/runner/runner_detail/runner_detail_app_spec.js
deleted file mode 100644
index 5caa37c8cb3..00000000000
--- a/spec/frontend/runner/runner_detail/runner_detail_app_spec.js
+++ /dev/null
@@ -1,29 +0,0 @@
-import { shallowMount } from '@vue/test-utils';
-import RunnerDetailsApp from '~/runner/runner_details/runner_details_app.vue';
-
-const mockRunnerId = '55';
-
-describe('RunnerDetailsApp', () => {
- let wrapper;
-
- const createComponent = (props) => {
- wrapper = shallowMount(RunnerDetailsApp, {
- propsData: {
- runnerId: mockRunnerId,
- ...props,
- },
- });
- };
-
- beforeEach(() => {
- createComponent();
- });
-
- afterEach(() => {
- wrapper.destroy();
- });
-
- it('displays the runner id', () => {
- expect(wrapper.text()).toContain('Runner #55');
- });
-});
diff --git a/spec/frontend/runner/runner_detail/runner_details_app_spec.js b/spec/frontend/runner/runner_detail/runner_details_app_spec.js
new file mode 100644
index 00000000000..c61cb647ae6
--- /dev/null
+++ b/spec/frontend/runner/runner_detail/runner_details_app_spec.js
@@ -0,0 +1,71 @@
+import { createLocalVue, mount, shallowMount } from '@vue/test-utils';
+import VueApollo from 'vue-apollo';
+import createMockApollo from 'helpers/mock_apollo_helper';
+import waitForPromises from 'helpers/wait_for_promises';
+
+import RunnerTypeBadge from '~/runner/components/runner_type_badge.vue';
+import { INSTANCE_TYPE } from '~/runner/constants';
+import getRunnerQuery from '~/runner/graphql/get_runner.query.graphql';
+import RunnerDetailsApp from '~/runner/runner_details/runner_details_app.vue';
+
+const mockRunnerId = '55';
+
+const localVue = createLocalVue();
+localVue.use(VueApollo);
+
+describe('RunnerDetailsApp', () => {
+ let wrapper;
+ let mockRunnerQuery;
+
+ const findRunnerTypeBadge = () => wrapper.findComponent(RunnerTypeBadge);
+
+ const createComponentWithApollo = ({ props = {}, mountFn = shallowMount } = {}) => {
+ const handlers = [[getRunnerQuery, mockRunnerQuery]];
+
+ wrapper = mountFn(RunnerDetailsApp, {
+ localVue,
+ apolloProvider: createMockApollo(handlers),
+ propsData: {
+ runnerId: mockRunnerId,
+ ...props,
+ },
+ });
+
+ return waitForPromises();
+ };
+
+ beforeEach(async () => {
+ mockRunnerQuery = jest.fn().mockResolvedValue({
+ data: {
+ runner: {
+ id: `gid://gitlab/Ci::Runner/${mockRunnerId}`,
+ runnerType: INSTANCE_TYPE,
+ __typename: 'CiRunner',
+ },
+ },
+ });
+ });
+
+ afterEach(() => {
+ mockRunnerQuery.mockReset();
+ wrapper.destroy();
+ });
+
+ it('expect GraphQL ID to be requested', async () => {
+ await createComponentWithApollo();
+
+ expect(mockRunnerQuery).toHaveBeenCalledWith({ id: `gid://gitlab/Ci::Runner/${mockRunnerId}` });
+ });
+
+ it('displays the runner id', async () => {
+ await createComponentWithApollo();
+
+ expect(wrapper.text()).toContain('Runner #55');
+ });
+
+ it('displays the runner type', async () => {
+ await createComponentWithApollo({ mountFn: mount });
+
+ expect(findRunnerTypeBadge().text()).toBe('shared');
+ });
+});
diff --git a/spec/frontend/sidebar/mock_data.js b/spec/frontend/sidebar/mock_data.js
index f51d2f3d459..8a969d64467 100644
--- a/spec/frontend/sidebar/mock_data.js
+++ b/spec/frontend/sidebar/mock_data.js
@@ -380,6 +380,25 @@ export const subscriptionNullResponse = {
},
};
+const mockUser1 = {
+ id: 'gid://gitlab/User/1',
+ avatarUrl:
+ 'https://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80\u0026d=identicon',
+ name: 'Administrator',
+ username: 'root',
+ webUrl: '/root',
+ status: null,
+};
+
+const mockUser2 = {
+ id: 'gid://gitlab/User/4',
+ avatarUrl: '/avatar2',
+ name: 'rookie',
+ username: 'rookie',
+ webUrl: 'rookie',
+ status: null,
+};
+
export const searchResponse = {
data: {
workspace: {
@@ -387,24 +406,10 @@ export const searchResponse = {
users: {
nodes: [
{
- user: {
- id: '1',
- avatarUrl: '/avatar',
- name: 'root',
- username: 'root',
- webUrl: 'root',
- status: null,
- },
+ user: mockUser1,
},
{
- user: {
- id: '2',
- avatarUrl: '/avatar2',
- name: 'rookie',
- username: 'rookie',
- webUrl: 'rookie',
- status: null,
- },
+ user: mockUser2,
},
],
},
@@ -418,27 +423,13 @@ export const projectMembersResponse = {
__typename: 'Project',
users: {
nodes: [
- {
- user: {
- id: 'gid://gitlab/User/1',
- avatarUrl:
- 'https://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80\u0026d=identicon',
- name: 'Administrator',
- username: 'root',
- webUrl: '/root',
- status: null,
- },
- },
- {
- user: {
- id: '2',
- avatarUrl: '/avatar2',
- name: 'rookie',
- username: 'rookie',
- webUrl: 'rookie',
- status: null,
- },
- },
+ // Remove nulls https://gitlab.com/gitlab-org/gitlab/-/issues/329750
+ null,
+ null,
+ // Remove duplicated entry https://gitlab.com/gitlab-org/gitlab/-/issues/327822
+ mockUser1,
+ mockUser1,
+ mockUser2,
{
user: {
id: 'gid://gitlab/User/2',
@@ -468,15 +459,9 @@ export const participantsQueryResponse = {
iid: '1',
participants: {
nodes: [
- {
- id: 'gid://gitlab/User/1',
- avatarUrl:
- 'https://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80\u0026d=identicon',
- name: 'Administrator',
- username: 'root',
- webUrl: '/root',
- status: null,
- },
+ // Remove duplicated entry https://gitlab.com/gitlab-org/gitlab/-/issues/327822
+ mockUser1,
+ mockUser1,
{
id: 'gid://gitlab/User/2',
avatarUrl:
diff --git a/spec/lib/generators/gitlab/snowplow_event_definition_generator_spec.rb b/spec/lib/generators/gitlab/snowplow_event_definition_generator_spec.rb
index 4f7c44e5d4e..25c4001a192 100644
--- a/spec/lib/generators/gitlab/snowplow_event_definition_generator_spec.rb
+++ b/spec/lib/generators/gitlab/snowplow_event_definition_generator_spec.rb
@@ -32,6 +32,30 @@ RSpec.describe Gitlab::SnowplowEventDefinitionGenerator do
expect(::Gitlab::Config::Loader::Yaml.new(File.read(event_definition_path)).load_raw!).to eq(sample_event)
end
+ context 'event definition already exists' do
+ before do
+ stub_const('Gitlab::VERSION', '12.11.0-pre')
+ described_class.new([], generator_options).invoke_all
+ end
+
+ it 'overwrites event definition --force flag set to true' do
+ sample_event = ::Gitlab::Config::Loader::Yaml.new(fixture_file(File.join(sample_event_dir, 'sample_event.yml'))).load_raw!
+
+ stub_const('Gitlab::VERSION', '13.11.0-pre')
+ described_class.new([], generator_options.merge('force' => true)).invoke_all
+
+ event_definition_path = File.join(ce_temp_dir, 'groups__email_campaigns_controller_click.yml')
+ event_data = ::Gitlab::Config::Loader::Yaml.new(File.read(event_definition_path)).load_raw!
+
+ expect(event_data).to eq(sample_event)
+ end
+
+ it 'raises error when --force flag set to false' do
+ expect { described_class.new([], generator_options.merge('force' => false)).invoke_all }
+ .to raise_error(StandardError, /Event definition already exists at/)
+ end
+ end
+
it 'creates EE event definition file using the template' do
sample_event = ::Gitlab::Config::Loader::Yaml.new(fixture_file(File.join(sample_event_dir, 'sample_event_ee.yml'))).load_raw!
diff --git a/spec/lib/gitlab/github_import/client_spec.rb b/spec/lib/gitlab/github_import/client_spec.rb
index 4000e0b2611..194dfb228ee 100644
--- a/spec/lib/gitlab/github_import/client_spec.rb
+++ b/spec/lib/gitlab/github_import/client_spec.rb
@@ -32,8 +32,9 @@ RSpec.describe Gitlab::GithubImport::Client do
it 'returns the pull request reviews' do
client = described_class.new('foo')
- expect(client.octokit).to receive(:pull_request_reviews).with('foo/bar', 999)
- expect(client).to receive(:with_rate_limit).and_yield
+ expect(client)
+ .to receive(:each_object)
+ .with(:pull_request_reviews, 'foo/bar', 999)
client.pull_request_reviews('foo/bar', 999)
end
diff --git a/spec/lib/gitlab/github_import/importer/pull_requests_merged_by_importer_spec.rb b/spec/lib/gitlab/github_import/importer/pull_requests_merged_by_importer_spec.rb
index b859cc727a6..4a47d103cde 100644
--- a/spec/lib/gitlab/github_import/importer/pull_requests_merged_by_importer_spec.rb
+++ b/spec/lib/gitlab/github_import/importer/pull_requests_merged_by_importer_spec.rb
@@ -23,12 +23,11 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestsMergedByImporter do
end
describe '#id_for_already_imported_cache' do
- it { expect(subject.id_for_already_imported_cache(double(number: 1))).to eq(1) }
+ it { expect(subject.id_for_already_imported_cache(double(id: 1))).to eq(1) }
end
- describe '#each_object_to_import' do
+ describe '#each_object_to_import', :clean_gitlab_redis_cache do
it 'fetchs the merged pull requests data' do
- pull_request = double
create(
:merged_merge_request,
iid: 999,
@@ -36,12 +35,18 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestsMergedByImporter do
target_project: project
)
+ pull_request = double
+
allow(client)
.to receive(:pull_request)
+ .exactly(:once) # ensure to be cached on the second call
.with('http://somegithub.com', 999)
.and_return(pull_request)
- expect { |b| subject.each_object_to_import(&b) }.to yield_with_args(pull_request)
+ expect { |b| subject.each_object_to_import(&b) }
+ .to yield_with_args(pull_request)
+
+ subject.each_object_to_import {}
end
end
end
diff --git a/spec/lib/gitlab/github_import/importer/pull_requests_reviews_importer_spec.rb b/spec/lib/gitlab/github_import/importer/pull_requests_reviews_importer_spec.rb
index 5e2302f9662..f18064f10aa 100644
--- a/spec/lib/gitlab/github_import/importer/pull_requests_reviews_importer_spec.rb
+++ b/spec/lib/gitlab/github_import/importer/pull_requests_reviews_importer_spec.rb
@@ -23,12 +23,18 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestsReviewsImporter do
end
describe '#id_for_already_imported_cache' do
- it { expect(subject.id_for_already_imported_cache(double(github_id: 1))).to eq(1) }
+ it { expect(subject.id_for_already_imported_cache(double(id: 1))).to eq(1) }
end
- describe '#each_object_to_import' do
+ describe '#each_object_to_import', :clean_gitlab_redis_cache do
it 'fetchs the merged pull requests data' do
- merge_request = create(:merge_request, source_project: project)
+ merge_request = create(
+ :merged_merge_request,
+ iid: 999,
+ source_project: project,
+ target_project: project
+ )
+
review = double
expect(review)
@@ -37,10 +43,14 @@ RSpec.describe Gitlab::GithubImport::Importer::PullRequestsReviewsImporter do
allow(client)
.to receive(:pull_request_reviews)
+ .exactly(:once) # ensure to be cached on the second call
.with('github/repo', merge_request.iid)
.and_return([review])
- expect { |b| subject.each_object_to_import(&b) }.to yield_with_args(review)
+ expect { |b| subject.each_object_to_import(&b) }
+ .to yield_with_args(review)
+
+ subject.each_object_to_import {}
end
end
end
diff --git a/spec/models/release_spec.rb b/spec/models/release_spec.rb
index 836ffadd7f7..b88813b3328 100644
--- a/spec/models/release_spec.rb
+++ b/spec/models/release_spec.rb
@@ -48,18 +48,6 @@ RSpec.describe Release do
expect(release.errors.full_messages)
.to include("Description is too long (maximum is #{Gitlab::Database::MAX_TEXT_SIZE_LIMIT} characters)")
end
-
- context 'when validate_release_description_length feature flag is disabled' do
- before do
- stub_feature_flags(validate_release_description_length: false)
- end
-
- it 'does not create a validation error' do
- release.validate
-
- expect(release.errors.full_messages).to be_empty
- end
- end
end
context 'when a release is tied to a milestone for another project' do
diff --git a/spec/support/helpers/snowplow_helpers.rb b/spec/support/helpers/snowplow_helpers.rb
index 70a4eadd8de..daff0bc8e14 100644
--- a/spec/support/helpers/snowplow_helpers.rb
+++ b/spec/support/helpers/snowplow_helpers.rb
@@ -71,7 +71,11 @@ module SnowplowHelpers
# expect_no_snowplow_event
# end
# end
- def expect_no_snowplow_event
- expect(Gitlab::Tracking).not_to have_received(:event) # rubocop:disable RSpec/ExpectGitlabTracking
+ def expect_no_snowplow_event(category: nil, action: nil, **kwargs)
+ if category && action
+ expect(Gitlab::Tracking).not_to have_received(:event).with(category, action, **kwargs) # rubocop:disable RSpec/ExpectGitlabTracking
+ else
+ expect(Gitlab::Tracking).not_to have_received(:event) # rubocop:disable RSpec/ExpectGitlabTracking
+ end
end
end
diff --git a/spec/workers/concerns/limited_capacity/job_tracker_spec.rb b/spec/workers/concerns/limited_capacity/job_tracker_spec.rb
index 2c79f347903..f141a1ad7ad 100644
--- a/spec/workers/concerns/limited_capacity/job_tracker_spec.rb
+++ b/spec/workers/concerns/limited_capacity/job_tracker_spec.rb
@@ -7,30 +7,30 @@ RSpec.describe LimitedCapacity::JobTracker, :clean_gitlab_redis_queues do
described_class.new('namespace')
end
+ let(:max_jids) { 10 }
+
describe '#register' do
it 'adds jid to the set' do
- job_tracker.register('a-job-id')
-
+ expect(job_tracker.register('a-job-id', max_jids)). to be true
expect(job_tracker.running_jids).to contain_exactly('a-job-id')
end
- it 'updates the counter' do
- expect { job_tracker.register('a-job-id') }
- .to change { job_tracker.count }
- .from(0)
- .to(1)
- end
-
- it 'does it in only one Redis call' do
- expect(job_tracker).to receive(:with_redis).once.and_call_original
+ it 'returns false if the jid was not added' do
+ max_jids = 2
+ %w[jid1 jid2].each do |jid|
+ expect(job_tracker.register(jid, max_jids)).to be true
+ end
- job_tracker.register('a-job-id')
+ expect(job_tracker.register('jid3', max_jids)).to be false
+ expect(job_tracker.running_jids).to contain_exactly(*%w[jid1 jid2])
end
end
describe '#remove' do
before do
- job_tracker.register(%w[a-job-id other-job-id])
+ %w[a-job-id other-job-id].each do |jid|
+ job_tracker.register(jid, max_jids)
+ end
end
it 'removes jid from the set' do
@@ -38,24 +38,11 @@ RSpec.describe LimitedCapacity::JobTracker, :clean_gitlab_redis_queues do
expect(job_tracker.running_jids).to contain_exactly('a-job-id')
end
-
- it 'updates the counter' do
- expect { job_tracker.remove('other-job-id') }
- .to change { job_tracker.count }
- .from(2)
- .to(1)
- end
-
- it 'does it in only one Redis call' do
- expect(job_tracker).to receive(:with_redis).once.and_call_original
-
- job_tracker.remove('other-job-id')
- end
end
describe '#clean_up' do
before do
- job_tracker.register('a-job-id')
+ job_tracker.register('a-job-id', max_jids)
end
context 'with running jobs' do
@@ -83,13 +70,6 @@ RSpec.describe LimitedCapacity::JobTracker, :clean_gitlab_redis_queues do
.to change { job_tracker.running_jids.include?('a-job-id') }
end
- it 'updates the counter' do
- expect { job_tracker.clean_up }
- .to change { job_tracker.count }
- .from(1)
- .to(0)
- end
-
it 'gets the job ids, removes them, and updates the counter with only two Redis calls' do
expect(job_tracker).to receive(:with_redis).twice.and_call_original
diff --git a/spec/workers/concerns/limited_capacity/worker_spec.rb b/spec/workers/concerns/limited_capacity/worker_spec.rb
index 2c33c8666ec..790b5c3544d 100644
--- a/spec/workers/concerns/limited_capacity/worker_spec.rb
+++ b/spec/workers/concerns/limited_capacity/worker_spec.rb
@@ -44,40 +44,22 @@ RSpec.describe LimitedCapacity::Worker, :clean_gitlab_redis_queues, :aggregate_f
describe '.perform_with_capacity' do
subject(:perform_with_capacity) { worker_class.perform_with_capacity(:arg) }
+ let(:max_running_jobs) { 3 }
+
before do
expect_next_instance_of(worker_class) do |instance|
expect(instance).to receive(:remove_failed_jobs)
- expect(instance).to receive(:report_prometheus_metrics)
-
- allow(instance).to receive(:remaining_work_count).and_return(remaining_work_count)
- allow(instance).to receive(:remaining_capacity).and_return(remaining_capacity)
- end
- end
-
- context 'when capacity is larger than work' do
- let(:remaining_work_count) { 2 }
- let(:remaining_capacity) { 3 }
- it 'enqueues jobs for remaining work' do
- expect(worker_class)
- .to receive(:bulk_perform_async)
- .with([[:arg], [:arg]])
-
- perform_with_capacity
+ allow(instance).to receive(:max_running_jobs).and_return(max_running_jobs)
end
end
- context 'when capacity is lower than work' do
- let(:remaining_work_count) { 5 }
- let(:remaining_capacity) { 3 }
-
- it 'enqueues jobs for remaining work' do
- expect(worker_class)
- .to receive(:bulk_perform_async)
- .with([[:arg], [:arg], [:arg]])
+ it 'enqueues jobs' do
+ expect(worker_class)
+ .to receive(:bulk_perform_async)
+ .with([[:arg], [:arg], [:arg]])
- perform_with_capacity
- end
+ perform_with_capacity
end
end
@@ -104,34 +86,27 @@ RSpec.describe LimitedCapacity::Worker, :clean_gitlab_redis_queues, :aggregate_f
perform
end
- it 'registers itself in the running set' do
+ it 'reports prometheus metrics' do
allow(worker).to receive(:perform_work)
- expect(job_tracker).to receive(:register).with('my-jid')
+ expect(worker).to receive(:report_prometheus_metrics).once.and_call_original
+ expect(worker).to receive(:report_running_jobs_metrics).twice.and_call_original
perform
end
- it 'removes itself from the running set' do
- expect(job_tracker).to receive(:remove).with('my-jid')
-
+ it 'updates the running set' do
+ expect(job_tracker.running_jids).to be_empty
allow(worker).to receive(:perform_work)
perform
- end
- it 'reports prometheus metrics' do
- allow(worker).to receive(:perform_work)
- expect(worker).to receive(:report_prometheus_metrics).once.and_call_original
- expect(worker).to receive(:report_running_jobs_metrics).twice.and_call_original
-
- perform
+ expect(job_tracker.running_jids).to be_empty
end
end
context 'with capacity and without work' do
before do
allow(worker).to receive(:max_running_jobs).and_return(10)
- allow(worker).to receive(:running_jobs_count).and_return(0)
allow(worker).to receive(:remaining_work_count).and_return(0)
allow(worker).to receive(:perform_work)
end
@@ -146,7 +121,7 @@ RSpec.describe LimitedCapacity::Worker, :clean_gitlab_redis_queues, :aggregate_f
context 'without capacity' do
before do
allow(worker).to receive(:max_running_jobs).and_return(10)
- allow(worker).to receive(:running_jobs_count).and_return(15)
+ allow(job_tracker).to receive(:register).and_return(false)
allow(worker).to receive(:remaining_work_count).and_return(10)
end
@@ -161,27 +136,14 @@ RSpec.describe LimitedCapacity::Worker, :clean_gitlab_redis_queues, :aggregate_f
perform
end
-
- it 'does not register in the running set' do
- expect(job_tracker).not_to receive(:register)
-
- perform
- end
-
- it 'removes itself from the running set' do
- expect(job_tracker).to receive(:remove).with('my-jid')
-
- perform
- end
-
- it 'reports prometheus metrics' do
- expect(worker).to receive(:report_prometheus_metrics)
-
- perform
- end
end
context 'when perform_work fails' do
+ before do
+ allow(worker).to receive(:max_running_jobs).and_return(10)
+ allow(job_tracker).to receive(:register).and_return(true)
+ end
+
it 'does not re-enqueue itself' do
expect(worker).not_to receive(:re_enqueue)
@@ -189,7 +151,7 @@ RSpec.describe LimitedCapacity::Worker, :clean_gitlab_redis_queues, :aggregate_f
end
it 'removes itself from the running set' do
- expect(job_tracker).to receive(:remove)
+ expect(job_tracker).to receive(:remove).with('my-jid')
expect { perform }.to raise_error(NotImplementedError)
end
@@ -202,65 +164,14 @@ RSpec.describe LimitedCapacity::Worker, :clean_gitlab_redis_queues, :aggregate_f
end
end
- describe '#remaining_capacity' do
- subject(:remaining_capacity) { worker.remaining_capacity }
-
- before do
- expect(worker).to receive(:max_running_jobs).and_return(max_capacity)
- end
-
- context 'when changing the capacity to a lower value' do
- let(:max_capacity) { -1 }
-
- it { expect(remaining_capacity).to eq(0) }
- end
-
- context 'when registering new jobs' do
- let(:max_capacity) { 2 }
-
- before do
- job_tracker.register('a-job-id')
- end
-
- it { expect(remaining_capacity).to eq(1) }
- end
-
- context 'with jobs in the queue' do
- let(:max_capacity) { 2 }
-
- before do
- expect(worker_class).to receive(:queue_size).and_return(1)
- end
-
- it { expect(remaining_capacity).to eq(1) }
- end
-
- context 'with both running jobs and queued jobs' do
- let(:max_capacity) { 10 }
-
- before do
- expect(worker_class).to receive(:queue_size).and_return(5)
- expect(worker).to receive(:running_jobs_count).and_return(3)
- end
-
- it { expect(remaining_capacity).to eq(2) }
- end
- end
-
describe '#remove_failed_jobs' do
subject(:remove_failed_jobs) { worker.remove_failed_jobs }
- before do
- job_tracker.register('a-job-id')
- allow(worker).to receive(:max_running_jobs).and_return(2)
+ it 'removes failed jobs' do
+ job_tracker.register('a-job-id', 10)
expect(job_tracker).to receive(:clean_up).and_call_original
- end
-
- context 'with failed jobs' do
- it 'update the available capacity' do
- expect { remove_failed_jobs }.to change { worker.remaining_capacity }.by(1)
- end
+ expect { remove_failed_jobs }.to change { job_tracker.running_jids.size }.by(-1)
end
end