diff options
author | GitLab Bot <gitlab-bot@gitlab.com> | 2023-12-27 18:07:02 +0300 |
---|---|---|
committer | GitLab Bot <gitlab-bot@gitlab.com> | 2023-12-27 18:07:02 +0300 |
commit | d9953dadb4f0170a32fbb3204564f2f96396656e (patch) | |
tree | 5106d7c970874f3afcf39cca0d8adfc76abc09c3 | |
parent | f8740a1ade9d4614dde927d8983eeb288e783ccf (diff) |
Add latest changes from gitlab-org/gitlab@master
38 files changed, 749 insertions, 425 deletions
diff --git a/.rubocop_todo/layout/empty_line_after_magic_comment.yml b/.rubocop_todo/layout/empty_line_after_magic_comment.yml index 409acc4b5c1..ba08bc8ae08 100644 --- a/.rubocop_todo/layout/empty_line_after_magic_comment.yml +++ b/.rubocop_todo/layout/empty_line_after_magic_comment.yml @@ -312,9 +312,9 @@ Layout/EmptyLineAfterMagicComment: - 'ee/spec/models/ci/minutes/quota_spec.rb' - 'ee/spec/models/ci/minutes/usage_spec.rb' - 'ee/spec/models/deployments/approval_summary_spec.rb' - - 'ee/spec/models/group_member_spec.rb' + - 'ee/spec/models/ee/group_member_spec.rb' - 'ee/spec/models/packages/package_file_spec.rb' - - 'ee/spec/models/project_member_spec.rb' + - 'ee/spec/models/ee/project_member_spec.rb' - 'ee/spec/models/protected_environment_spec.rb' - 'ee/spec/models/protected_environments/approval_rule_spec.rb' - 'ee/spec/models/protected_environments/deploy_access_level_spec.rb' diff --git a/.rubocop_todo/layout/line_length.yml b/.rubocop_todo/layout/line_length.yml index 8cab670cc83..b8b267d6d86 100644 --- a/.rubocop_todo/layout/line_length.yml +++ b/.rubocop_todo/layout/line_length.yml @@ -1794,7 +1794,7 @@ Layout/LineLength: - 'ee/spec/models/geo_node_status_spec.rb' - 'ee/spec/models/gitlab_subscription_spec.rb' - 'ee/spec/models/gitlab_subscriptions/features_spec.rb' - - 'ee/spec/models/group_member_spec.rb' + - 'ee/spec/models/ee/group_member_spec.rb' - 'ee/spec/models/historical_data_spec.rb' - 'ee/spec/models/incident_management/escalation_policy_spec.rb' - 'ee/spec/models/incident_management/escalation_rule_spec.rb' @@ -1807,7 +1807,7 @@ Layout/LineLength: - 'ee/spec/models/issue_spec.rb' - 'ee/spec/models/iteration_spec.rb' - 'ee/spec/models/license_spec.rb' - - 'ee/spec/models/member_spec.rb' + - 'ee/spec/models/ee/member_spec.rb' - 'ee/spec/models/merge_request_spec.rb' - 'ee/spec/models/merge_requests/compliance_violation_spec.rb' - 'ee/spec/models/merge_requests/external_status_check_spec.rb' @@ -1816,7 +1816,7 @@ Layout/LineLength: - 'ee/spec/models/packages/package_file_spec.rb' - 'ee/spec/models/project_import_data_spec.rb' - 'ee/spec/models/project_import_state_spec.rb' - - 'ee/spec/models/project_member_spec.rb' + - 'ee/spec/models/ee/project_member_spec.rb' - 'ee/spec/models/project_security_setting_spec.rb' - 'ee/spec/models/protected_environment_spec.rb' - 'ee/spec/models/protected_environments/approval_rule_spec.rb' diff --git a/.rubocop_todo/layout/space_inside_parens.yml b/.rubocop_todo/layout/space_inside_parens.yml index 34f13f780fb..d044e48d7aa 100644 --- a/.rubocop_todo/layout/space_inside_parens.yml +++ b/.rubocop_todo/layout/space_inside_parens.yml @@ -35,7 +35,7 @@ Layout/SpaceInsideParens: - 'ee/spec/models/iteration_spec.rb' - 'ee/spec/models/ldap_group_link_spec.rb' - 'ee/spec/models/license_spec.rb' - - 'ee/spec/models/member_spec.rb' + - 'ee/spec/models/ee/member_spec.rb' - 'ee/spec/models/release_highlight_spec.rb' - 'ee/spec/models/security/orchestration_policy_configuration_spec.rb' - 'ee/spec/models/vulnerabilities/feedback_spec.rb' diff --git a/.rubocop_todo/lint/unused_block_argument.yml b/.rubocop_todo/lint/unused_block_argument.yml index 1df273d408d..f14c02c4ecd 100644 --- a/.rubocop_todo/lint/unused_block_argument.yml +++ b/.rubocop_todo/lint/unused_block_argument.yml @@ -131,7 +131,7 @@ Lint/UnusedBlockArgument: - 'ee/spec/lib/gitlab/graphql/aggregations/epics/lazy_epic_aggregate_spec.rb' - 'ee/spec/lib/gitlab/insights/project_insights_config_spec.rb' - 'ee/spec/lib/gitlab/usage_data_metrics_spec.rb' - - 'ee/spec/models/member_spec.rb' + - 'ee/spec/models/ee/member_spec.rb' - 'ee/spec/requests/api/graphql/project/pipeline/security_report_summary_spec.rb' - 'ee/spec/requests/api/graphql/vulnerabilities/sort_spec.rb' - 'ee/spec/requests/api/related_epic_links_spec.rb' diff --git a/.rubocop_todo/performance/map_compact.yml b/.rubocop_todo/performance/map_compact.yml index 8f831bf8f59..22cefbab067 100644 --- a/.rubocop_todo/performance/map_compact.yml +++ b/.rubocop_todo/performance/map_compact.yml @@ -75,7 +75,7 @@ Performance/MapCompact: - 'ee/lib/gitlab/ci/reports/metrics/reports_comparer.rb' - 'ee/lib/gitlab/search/aggregation_parser.rb' - 'ee/spec/models/analytics/issues_analytics_spec.rb' - - 'ee/spec/models/member_spec.rb' + - 'ee/spec/models/ee/member_spec.rb' - 'ee/spec/requests/api/audit_events_spec.rb' - 'ee/spec/requests/api/search_spec.rb' - 'haml_lint/linter/no_plain_nodes.rb' diff --git a/.rubocop_todo/rspec/before_all_role_assignment.yml b/.rubocop_todo/rspec/before_all_role_assignment.yml index fcdaf78ffe8..3436cffc2ff 100644 --- a/.rubocop_todo/rspec/before_all_role_assignment.yml +++ b/.rubocop_todo/rspec/before_all_role_assignment.yml @@ -321,7 +321,7 @@ RSpec/BeforeAllRoleAssignment: - 'ee/spec/models/ee/vulnerability_spec.rb' - 'ee/spec/models/epic_issue_spec.rb' - 'ee/spec/models/epic_spec.rb' - - 'ee/spec/models/group_member_spec.rb' + - 'ee/spec/models/ee/group_member_spec.rb' - 'ee/spec/models/instance_security_dashboard_spec.rb' - 'ee/spec/models/issue_spec.rb' - 'ee/spec/models/merge_request_spec.rb' diff --git a/.rubocop_todo/rspec/context_wording.yml b/.rubocop_todo/rspec/context_wording.yml index 6375841e697..0a50fc4485d 100644 --- a/.rubocop_todo/rspec/context_wording.yml +++ b/.rubocop_todo/rspec/context_wording.yml @@ -453,7 +453,7 @@ RSpec/ContextWording: - 'ee/spec/models/geo_node_spec.rb' - 'ee/spec/models/geo_node_status_spec.rb' - 'ee/spec/models/gitlab_subscription_spec.rb' - - 'ee/spec/models/group_member_spec.rb' + - 'ee/spec/models/ee/group_member_spec.rb' - 'ee/spec/models/group_wiki_repository_spec.rb' - 'ee/spec/models/incident_management/escalation_rule_spec.rb' - 'ee/spec/models/incident_management/oncall_rotation_spec.rb' @@ -467,7 +467,7 @@ RSpec/ContextWording: - 'ee/spec/models/issue_spec.rb' - 'ee/spec/models/iteration_spec.rb' - 'ee/spec/models/license_spec.rb' - - 'ee/spec/models/member_spec.rb' + - 'ee/spec/models/ee/member_spec.rb' - 'ee/spec/models/merge_request/blocking_spec.rb' - 'ee/spec/models/merge_request_spec.rb' - 'ee/spec/models/namespace_setting_spec.rb' @@ -476,7 +476,7 @@ RSpec/ContextWording: - 'ee/spec/models/path_lock_spec.rb' - 'ee/spec/models/project_import_data_spec.rb' - 'ee/spec/models/project_import_state_spec.rb' - - 'ee/spec/models/project_member_spec.rb' + - 'ee/spec/models/ee/project_member_spec.rb' - 'ee/spec/models/protected_environment_spec.rb' - 'ee/spec/models/push_rule_spec.rb' - 'ee/spec/models/release_highlight_spec.rb' diff --git a/.rubocop_todo/rspec/expect_change.yml b/.rubocop_todo/rspec/expect_change.yml index f358600f4fe..c5e3556defd 100644 --- a/.rubocop_todo/rspec/expect_change.yml +++ b/.rubocop_todo/rspec/expect_change.yml @@ -36,7 +36,7 @@ RSpec/ExpectChange: - 'ee/spec/models/gitlab_subscription_spec.rb' - 'ee/spec/models/group_wiki_spec.rb' - 'ee/spec/models/incident_management/issuable_escalation_status_spec.rb' - - 'ee/spec/models/member_spec.rb' + - 'ee/spec/models/ee/member_spec.rb' - 'ee/spec/models/project_import_state_spec.rb' - 'ee/spec/models/push_rule_spec.rb' - 'ee/spec/models/security/orchestration_policy_configuration_spec.rb' diff --git a/.rubocop_todo/rspec/feature_category.yml b/.rubocop_todo/rspec/feature_category.yml index ab4170d5a2f..163eeb207ad 100644 --- a/.rubocop_todo/rspec/feature_category.yml +++ b/.rubocop_todo/rspec/feature_category.yml @@ -1063,7 +1063,7 @@ RSpec/FeatureCategory: - 'ee/spec/models/gitlab_subscriptions/features_spec.rb' - 'ee/spec/models/gitlab_subscriptions/upcoming_reconciliation_spec.rb' - 'ee/spec/models/group_deletion_schedule_spec.rb' - - 'ee/spec/models/group_member_spec.rb' + - 'ee/spec/models/ee/group_member_spec.rb' - 'ee/spec/models/group_merge_request_approval_setting_spec.rb' - 'ee/spec/models/group_wiki_repository_spec.rb' - 'ee/spec/models/historical_data_spec.rb' @@ -1108,7 +1108,7 @@ RSpec/FeatureCategory: - 'ee/spec/models/project_ci_cd_setting_spec.rb' - 'ee/spec/models/project_feature_spec.rb' - 'ee/spec/models/project_import_data_spec.rb' - - 'ee/spec/models/project_member_spec.rb' + - 'ee/spec/models/ee/project_member_spec.rb' - 'ee/spec/models/project_repository_state_spec.rb' - 'ee/spec/models/project_security_setting_spec.rb' - 'ee/spec/models/protected_branch/required_code_owners_section_spec.rb' diff --git a/app/graphql/mutations/ml/models/base.rb b/app/graphql/mutations/ml/models/base.rb new file mode 100644 index 00000000000..e3c5a7a13a8 --- /dev/null +++ b/app/graphql/mutations/ml/models/base.rb @@ -0,0 +1,20 @@ +# frozen_string_literal: true + +module Mutations + module Ml + module Models + class Base < BaseMutation + authorize :write_model_registry + + argument :project_path, GraphQL::Types::ID, + required: true, + description: "Project the model to mutate is in." + + field :model, + Types::Ml::ModelType, + null: true, + description: 'Model after mutation.' + end + end + end +end diff --git a/app/graphql/mutations/ml/models/create.rb b/app/graphql/mutations/ml/models/create.rb new file mode 100644 index 00000000000..21570fc34b8 --- /dev/null +++ b/app/graphql/mutations/ml/models/create.rb @@ -0,0 +1,32 @@ +# frozen_string_literal: true + +module Mutations + module Ml + module Models + class Create < Base + graphql_name 'MlModelCreate' + + include FindsProject + + argument :name, GraphQL::Types::String, + required: true, + description: 'Name of the model.' + + argument :description, GraphQL::Types::String, + required: false, + description: 'Description of the model.' + + def resolve(**args) + project = authorized_find!(args[:project_path]) + + model = ::Ml::CreateModelService.new(project, args[:name], current_user, args[:description]).execute + + { + model: model.persisted? ? model : nil, + errors: errors_on_object(model) + } + end + end + end + end +end diff --git a/app/graphql/types/ml/model_type.rb b/app/graphql/types/ml/model_type.rb index e677b37547a..a26d50cbdc4 100644 --- a/app/graphql/types/ml/model_type.rb +++ b/app/graphql/types/ml/model_type.rb @@ -9,12 +9,16 @@ module Types connection_type_class Types::LimitedCountableConnectionType + present_using ::Ml::ModelPresenter + field :id, ::Types::GlobalIDType[::Ml::Model], null: false, description: 'ID of the model.' field :name, ::GraphQL::Types::String, null: false, description: 'Name of the model.' field :created_at, Types::TimeType, null: false, description: 'Date of creation.' + field :description, ::GraphQL::Types::String, null: false, description: 'Description of the model.' + field :latest_version, ::Types::Ml::ModelVersionType, null: true, description: 'Latest version of the model.' field :version_count, ::GraphQL::Types::Int, null: true, description: 'Count of versions in the model.' diff --git a/app/graphql/types/mutation_type.rb b/app/graphql/types/mutation_type.rb index 4c987c657ef..0a725c2e0a7 100644 --- a/app/graphql/types/mutation_type.rb +++ b/app/graphql/types/mutation_type.rb @@ -203,6 +203,7 @@ module Types mount_mutation Mutations::Users::SetNamespaceCommitEmail mount_mutation Mutations::WorkItems::Subscribe, alpha: { milestone: '16.3' } mount_mutation Mutations::Admin::AbuseReportLabels::Create, alpha: { milestone: '16.4' } + mount_mutation Mutations::Ml::Models::Create, alpha: { milestone: '16.8' } end end diff --git a/app/helpers/ci/status_helper.rb b/app/helpers/ci/status_helper.rb index 21d982d42bc..15413cb05ac 100644 --- a/app/helpers/ci/status_helper.rb +++ b/app/helpers/ci/status_helper.rb @@ -56,10 +56,6 @@ module Ci end # rubocop:enable Metrics/CyclomaticComplexity - def pipeline_status_cache_key(pipeline_status) - "pipeline-status/#{pipeline_status.sha}-#{pipeline_status.status}" - end - def render_commit_status(commit, status, ref: nil, tooltip_placement: 'left') project = commit.project path = pipelines_project_commit_path(project, commit, ref: ref) diff --git a/app/models/member.rb b/app/models/member.rb index feb0b66f63c..2db2df91cbe 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -526,6 +526,14 @@ class Member < ApplicationRecord end def post_update_hook + if saved_change_to_access_level? + run_after_commit { notification_service.updated_member_access_level(self) } + end + + if saved_change_to_expires_at? + run_after_commit { notification_service.updated_member_expiration(self) } + end + system_hook_service.execute_hooks_for(self, :update) end diff --git a/app/models/members/group_member.rb b/app/models/members/group_member.rb index 0e70f9973d6..3f1fcfe8339 100644 --- a/app/models/members/group_member.rb +++ b/app/models/members/group_member.rb @@ -94,18 +94,6 @@ class GroupMember < Member super end - def post_update_hook - if saved_change_to_access_level? - run_after_commit { notification_service.update_group_member(self) } - end - - if saved_change_to_expires_at? - run_after_commit { notification_service.updated_group_member_expiration(self) } - end - - super - end - def after_accept_invite run_after_commit_or_now do notification_service.accept_group_invite(self) diff --git a/app/models/members/project_member.rb b/app/models/members/project_member.rb index e442fc969d0..3540e7a043a 100644 --- a/app/models/members/project_member.rb +++ b/app/models/members/project_member.rb @@ -128,14 +128,6 @@ class ProjectMember < Member super end - def post_update_hook - if saved_change_to_access_level? - run_after_commit { notification_service.update_project_member(self) } - end - - super - end - def post_destroy_hook if expired? event_service.expired_leave_project(self.project, self.user) diff --git a/app/services/click_house/sync_strategies/base_sync_strategy.rb b/app/services/click_house/sync_strategies/base_sync_strategy.rb new file mode 100644 index 00000000000..58c2161b83c --- /dev/null +++ b/app/services/click_house/sync_strategies/base_sync_strategy.rb @@ -0,0 +1,124 @@ +# frozen_string_literal: true + +module ClickHouse + module SyncStrategies + class BaseSyncStrategy + include Gitlab::ExclusiveLeaseHelpers + include Gitlab::Utils::StrongMemoize + + # the job is scheduled every 3 minutes and we will allow maximum 2.5 minutes runtime + MAX_TTL = 2.5.minutes.to_i + MAX_RUNTIME = 120.seconds + BATCH_SIZE = 500 + INSERT_BATCH_SIZE = 5000 + + def execute + return { status: :disabled } unless enabled? + + metadata = { status: :processed } + + begin + # Prevent parallel jobs + in_lock(self.class.to_s, ttl: MAX_TTL, retries: 0) do + loop { break unless next_batch } + + metadata.merge!(records_inserted: context.total_record_count, + reached_end_of_table: context.no_more_records?) + + if context.last_processed_id + ClickHouse::SyncCursor.update_cursor_for(model_class.table_name, + context.last_processed_id) + end + end + rescue Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError + # Skip retrying, just let the next worker to start after a few minutes + metadata = { status: :skipped } + end + + metadata + end + + private + + def enabled? + ClickHouse::Client.database_configured?(:main) + end + + def context + @context ||= ClickHouse::RecordSyncContext.new( + last_record_id: ClickHouse::SyncCursor.cursor_for(model_class.table_name), + max_records_per_batch: INSERT_BATCH_SIZE, + runtime_limiter: Analytics::CycleAnalytics::RuntimeLimiter.new(MAX_RUNTIME) + ) + end + + def last_id_in_postgresql + model_class.maximum(:id) + end + + strong_memoize_attr :last_id_in_postgresql + + def next_batch + context.new_batch! + + CsvBuilder::Gzip.new(process_batch(context), csv_mapping).render do |tempfile, rows_written| + unless rows_written == 0 + ClickHouse::Client.insert_csv(insert_query, File.open(tempfile.path), + :main) + end + end + + !(context.over_time? || context.no_more_records?) + end + + def process_batch(context) + Enumerator.new do |yielder| + has_more_data = false + batching_scope.each_batch(of: BATCH_SIZE) do |relation| + records = relation.select(projections).to_a + has_more_data = records.size == BATCH_SIZE + records.each do |row| + yielder << transform_row(row) + context.last_processed_id = row.id + + break if context.record_limit_reached? + end + + break if context.over_time? || context.record_limit_reached? || !has_more_data + end + + context.no_more_records! unless has_more_data + end + end + + def transform_row(row) + row + end + + # rubocop: disable CodeReuse/ActiveRecord -- because model here is dynamic and is passed by child class + def batching_scope + return model_class.none unless last_id_in_postgresql + + table = model_class.arel_table + + model_class + .where(table[:id].gt(context.last_record_id)) + .where(table[:id].lteq(last_id_in_postgresql)) + end + + # rubocop: enable CodeReuse/ActiveRecord + + def projections + raise NotImplementedError, "Subclasses must implement `projections`" + end + + def csv_mapping + raise NotImplementedError, "Subclasses must implement `csv_mapping`" + end + + def insert_query + raise NotImplementedError, "Subclasses must implement `insert_query`" + end + end + end +end diff --git a/app/services/click_house/sync_strategies/event_sync_strategy.rb b/app/services/click_house/sync_strategies/event_sync_strategy.rb new file mode 100644 index 00000000000..3e86e8c52bc --- /dev/null +++ b/app/services/click_house/sync_strategies/event_sync_strategy.rb @@ -0,0 +1,62 @@ +# frozen_string_literal: true + +module ClickHouse + module SyncStrategies + class EventSyncStrategy < BaseSyncStrategy + # transforms the traversal_ids to a String: + # Example: group_id/subgroup_id/group_or_projectnamespace_id/ + PATH_COLUMN = <<~SQL + ( + CASE + WHEN project_id IS NOT NULL THEN (SELECT array_to_string(traversal_ids, '/') || '/' FROM namespaces WHERE id = (SELECT project_namespace_id FROM projects WHERE id = events.project_id LIMIT 1) LIMIT 1) + WHEN group_id IS NOT NULL THEN (SELECT array_to_string(traversal_ids, '/') || '/' FROM namespaces WHERE id = events.group_id LIMIT 1) + ELSE '' + END + ) AS path + SQL + + private + + def csv_mapping + { + id: :id, + path: :path, + author_id: :author_id, + target_id: :target_id, + target_type: :target_type, + action: :raw_action, + created_at: :casted_created_at, + updated_at: :casted_updated_at + } + end + + def projections + [ + :id, + PATH_COLUMN, + :author_id, + :target_id, + :target_type, + 'action AS raw_action', + 'EXTRACT(epoch FROM created_at) AS casted_created_at', + 'EXTRACT(epoch FROM updated_at) AS casted_updated_at' + ] + end + + def insert_query + <<~SQL.squish + INSERT INTO events (#{csv_mapping.keys.join(', ')}) + SETTINGS async_insert=1, wait_for_async_insert=1 FORMAT CSV + SQL + end + + def model_class + ::Event + end + + def enabled? + super && Feature.enabled?(:event_sync_worker_for_click_house) + end + end + end +end diff --git a/app/services/ml/create_model_service.rb b/app/services/ml/create_model_service.rb index b87b13dd379..7ac9c2a2737 100644 --- a/app/services/ml/create_model_service.rb +++ b/app/services/ml/create_model_service.rb @@ -12,21 +12,25 @@ module Ml def execute ApplicationRecord.transaction do - model = Ml::Model.create!( + model = Ml::Model.new( project: @project, name: @name, - user: (@user.is_a?(User) ? @user : nil), + user: @user, description: @description, default_experiment: default_experiment ) - add_metadata(model, @metadata) + model.save - Gitlab::InternalEvents.track_event( - 'model_registry_ml_model_created', - project: @project, - user: @user - ) + if model.persisted? + add_metadata(model, @metadata) + + Gitlab::InternalEvents.track_event( + 'model_registry_ml_model_created', + project: @project, + user: @user + ) + end model end diff --git a/app/services/notification_service.rb b/app/services/notification_service.rb index 95d291b9bd6..ff712d78c8b 100644 --- a/app/services/notification_service.rb +++ b/app/services/notification_service.rb @@ -532,14 +532,21 @@ class NotificationService mailer.member_invite_accepted_email(project_member.real_source_type, project_member.id).deliver_later end - def new_project_member(project_member) - return true unless project_member.notifiable?(:mention, skip_read_ability: true) + def updated_member_access_level(member) + return true unless member.notifiable?(:mention) - mailer.member_access_granted_email(project_member.real_source_type, project_member.id).deliver_later + mailer.member_access_granted_email(member.real_source_type, member.id).deliver_later + end + + def updated_member_expiration(member) + return true unless member.source.is_a?(Group) + return true unless member.notifiable?(:mention) + + mailer.member_expiration_date_updated_email(member.real_source_type, member.id).deliver_later end - def update_project_member(project_member) - return true unless project_member.notifiable?(:mention) + def new_project_member(project_member) + return true unless project_member.notifiable?(:mention, skip_read_ability: true) mailer.member_access_granted_email(project_member.real_source_type, project_member.id).deliver_later end @@ -564,18 +571,6 @@ class NotificationService mailer.member_access_granted_email(group_member.real_source_type, group_member.id).deliver_later end - def update_group_member(group_member) - return true unless group_member.notifiable?(:mention) - - mailer.member_access_granted_email(group_member.real_source_type, group_member.id).deliver_later - end - - def updated_group_member_expiration(group_member) - return true unless group_member.notifiable?(:mention) - - mailer.member_expiration_date_updated_email(group_member.real_source_type, group_member.id).deliver_later - end - def project_was_moved(project, old_path_with_namespace) recipients = project_moved_recipients(project) recipients = notifiable_users(recipients, :custom, custom_action: :moved_project, project: project) diff --git a/app/workers/click_house/events_sync_worker.rb b/app/workers/click_house/events_sync_worker.rb index 21c10566a67..3cfd3f91a29 100644 --- a/app/workers/click_house/events_sync_worker.rb +++ b/app/workers/click_house/events_sync_worker.rb @@ -4,8 +4,6 @@ module ClickHouse class EventsSyncWorker include ApplicationWorker include ClickHouseWorker - include Gitlab::ExclusiveLeaseHelpers - include Gitlab::Utils::StrongMemoize idempotent! queue_namespace :cronjob @@ -13,138 +11,9 @@ module ClickHouse worker_has_external_dependencies! # the worker interacts with a ClickHouse database feature_category :value_stream_management - # the job is scheduled every 3 minutes and we will allow maximum 2.5 minutes runtime - MAX_TTL = 2.5.minutes.to_i - MAX_RUNTIME = 120.seconds - BATCH_SIZE = 500 - INSERT_BATCH_SIZE = 5000 - CSV_MAPPING = { - id: :id, - path: :path, - author_id: :author_id, - target_id: :target_id, - target_type: :target_type, - action: :raw_action, - created_at: :casted_created_at, - updated_at: :casted_updated_at - }.freeze - - # transforms the traversal_ids to a String: - # Example: group_id/subgroup_id/group_or_projectnamespace_id/ - PATH_COLUMN = <<~SQL - ( - CASE - WHEN project_id IS NOT NULL THEN (SELECT array_to_string(traversal_ids, '/') || '/' FROM namespaces WHERE id = (SELECT project_namespace_id FROM projects WHERE id = events.project_id LIMIT 1) LIMIT 1) - WHEN group_id IS NOT NULL THEN (SELECT array_to_string(traversal_ids, '/') || '/' FROM namespaces WHERE id = events.group_id LIMIT 1) - ELSE '' - END - ) AS path - SQL - - EVENT_PROJECTIONS = [ - :id, - PATH_COLUMN, - :author_id, - :target_id, - :target_type, - 'action AS raw_action', - 'EXTRACT(epoch FROM created_at) AS casted_created_at', - 'EXTRACT(epoch FROM updated_at) AS casted_updated_at' - ].freeze - - INSERT_EVENTS_QUERY = <<~SQL.squish - INSERT INTO events (#{CSV_MAPPING.keys.join(', ')}) - SETTINGS async_insert=1, wait_for_async_insert=1 FORMAT CSV - SQL - def perform - unless enabled? - log_extra_metadata_on_done(:result, { status: :disabled }) - - return - end - - metadata = { status: :processed } - - begin - # Prevent parallel jobs - in_lock(self.class.to_s, ttl: MAX_TTL, retries: 0) do - loop { break unless next_batch } - - metadata.merge!(records_inserted: context.total_record_count, reached_end_of_table: context.no_more_records?) - - ClickHouse::SyncCursor.update_cursor_for(:events, context.last_processed_id) if context.last_processed_id - end - rescue Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError - # Skip retrying, just let the next worker to start after a few minutes - metadata = { status: :skipped } - end - - log_extra_metadata_on_done(:result, metadata) - end - - private - - def context - @context ||= ClickHouse::RecordSyncContext.new( - last_record_id: ClickHouse::SyncCursor.cursor_for(:events), - max_records_per_batch: INSERT_BATCH_SIZE, - runtime_limiter: Analytics::CycleAnalytics::RuntimeLimiter.new(MAX_RUNTIME) - ) - end - - def last_event_id_in_postgresql - Event.maximum(:id) - end - strong_memoize_attr :last_event_id_in_postgresql - - def enabled? - ClickHouse::Client.database_configured?(:main) && Feature.enabled?(:event_sync_worker_for_click_house) - end - - def next_batch - context.new_batch! - - CsvBuilder::Gzip.new(process_batch(context), CSV_MAPPING).render do |tempfile, rows_written| - unless rows_written == 0 - ClickHouse::Client.insert_csv(INSERT_EVENTS_QUERY, File.open(tempfile.path), - :main) - end - end - - !(context.over_time? || context.no_more_records?) - end - - def process_batch(context) - Enumerator.new do |yielder| - has_more_data = false - batching_scope.each_batch(of: BATCH_SIZE) do |relation| - records = relation.select(*EVENT_PROJECTIONS).to_a - has_more_data = records.size == BATCH_SIZE - records.each do |row| - yielder << row - context.last_processed_id = row.id - - break if context.record_limit_reached? - end - - break if context.over_time? || context.record_limit_reached? || !has_more_data - end - - context.no_more_records! unless has_more_data - end - end - - # rubocop: disable CodeReuse/ActiveRecord - def batching_scope - return Event.none unless last_event_id_in_postgresql - - table = Event.arel_table - - Event - .where(table[:id].gt(context.last_record_id)) - .where(table[:id].lteq(last_event_id_in_postgresql)) + result = ::ClickHouse::SyncStrategies::EventSyncStrategy.new.execute + log_extra_metadata_on_done(:result, result) end - # rubocop: enable CodeReuse/ActiveRecord end end diff --git a/doc/api/graphql/reference/index.md b/doc/api/graphql/reference/index.md index 621ac030783..ee1d49be690 100644 --- a/doc/api/graphql/reference/index.md +++ b/doc/api/graphql/reference/index.md @@ -5719,6 +5719,31 @@ Input type: `MergeRequestUpdateApprovalRuleInput` | <a id="mutationmergerequestupdateapprovalruleerrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | | <a id="mutationmergerequestupdateapprovalrulemergerequest"></a>`mergeRequest` | [`MergeRequest`](#mergerequest) | Merge request after mutation. | +### `Mutation.mlModelCreate` + +WARNING: +**Introduced** in 16.8. +This feature is an Experiment. It can be changed or removed at any time. + +Input type: `MlModelCreateInput` + +#### Arguments + +| Name | Type | Description | +| ---- | ---- | ----------- | +| <a id="mutationmlmodelcreateclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| <a id="mutationmlmodelcreatedescription"></a>`description` | [`String`](#string) | Description of the model. | +| <a id="mutationmlmodelcreatename"></a>`name` | [`String!`](#string) | Name of the model. | +| <a id="mutationmlmodelcreateprojectpath"></a>`projectPath` | [`ID!`](#id) | Project the model to mutate is in. | + +#### Fields + +| Name | Type | Description | +| ---- | ---- | ----------- | +| <a id="mutationmlmodelcreateclientmutationid"></a>`clientMutationId` | [`String`](#string) | A unique identifier for the client performing the mutation. | +| <a id="mutationmlmodelcreateerrors"></a>`errors` | [`[String!]!`](#string) | Errors encountered during execution of the mutation. | +| <a id="mutationmlmodelcreatemodel"></a>`model` | [`MlModel`](#mlmodel) | Model after mutation. | + ### `Mutation.namespaceBanDestroy` Input type: `NamespaceBanDestroyInput` @@ -23051,6 +23076,7 @@ Machine learning model in the model registry. | <a id="mlmodel_links"></a>`_links` | [`MLModelLinks!`](#mlmodellinks) | Map of links to perform actions on the model. | | <a id="mlmodelcandidates"></a>`candidates` | [`MlCandidateConnection`](#mlcandidateconnection) | Version candidates of the model. (see [Connections](#connections)) | | <a id="mlmodelcreatedat"></a>`createdAt` | [`Time!`](#time) | Date of creation. | +| <a id="mlmodeldescription"></a>`description` | [`String!`](#string) | Description of the model. | | <a id="mlmodelid"></a>`id` | [`MlModelID!`](#mlmodelid) | ID of the model. | | <a id="mlmodellatestversion"></a>`latestVersion` | [`MlModelVersion`](#mlmodelversion) | Latest version of the model. | | <a id="mlmodelname"></a>`name` | [`String!`](#string) | Name of the model. | diff --git a/doc/user/analytics/repository_analytics.md b/doc/user/analytics/repository_analytics.md index 93171dc3136..8bc163f6f3f 100644 --- a/doc/user/analytics/repository_analytics.md +++ b/doc/user/analytics/repository_analytics.md @@ -8,7 +8,7 @@ info: To determine the technical writer assigned to the Stage/Group associated w Use repository analytics to view information about a project's Git repository: -- Programming languages used in the repository. +- Programming languages used in the repository's default branch. - Code coverage history from last 3 months ([Introduced](https://gitlab.com/gitlab-org/gitlab/-/issues/33743) in GitLab 13.1). - Commit statistics (last month). - Commits per day of month. diff --git a/lib/api/ml/mlflow/registered_models.rb b/lib/api/ml/mlflow/registered_models.rb index a68a2767a74..3f4996a94c0 100644 --- a/lib/api/ml/mlflow/registered_models.rb +++ b/lib/api/ml/mlflow/registered_models.rb @@ -31,13 +31,17 @@ module API optional :tags, type: Array, desc: 'Additional metadata for registered model.' end post 'create', urgency: :low do - present ::Ml::CreateModelService.new( + model = ::Ml::CreateModelService.new( user_project, params[:name], current_user, params[:description], params[:tags] - ).execute, + ).execute + + resource_already_exists! unless model.persisted? + + present model, with: Entities::Ml::Mlflow::RegisteredModel, root: :registered_model rescue ActiveRecord::RecordInvalid diff --git a/spec/graphql/types/ml/model_type_spec.rb b/spec/graphql/types/ml/model_type_spec.rb index 79edea29eb8..078391f135a 100644 --- a/spec/graphql/types/ml/model_type_spec.rb +++ b/spec/graphql/types/ml/model_type_spec.rb @@ -6,7 +6,7 @@ RSpec.describe GitlabSchema.types['MlModel'], feature_category: :mlops do specify { expect(described_class.description).to eq('Machine learning model in the model registry') } it 'includes all the package fields' do - expected_fields = %w[id name versions candidates version_count _links created_at latest_version] + expected_fields = %w[id name versions candidates version_count _links created_at latest_version description] expect(described_class).to include_graphql_fields(*expected_fields) end diff --git a/spec/helpers/ci/status_helper_spec.rb b/spec/helpers/ci/status_helper_spec.rb index 502a535e102..a00a80ac06b 100644 --- a/spec/helpers/ci/status_helper_spec.rb +++ b/spec/helpers/ci/status_helper_spec.rb @@ -8,19 +8,6 @@ RSpec.describe Ci::StatusHelper do let(:success_commit) { double("Ci::Pipeline", status: 'success') } let(:failed_commit) { double("Ci::Pipeline", status: 'failed') } - describe "#pipeline_status_cache_key" do - it "builds a cache key for pipeline status" do - pipeline_status = Gitlab::Cache::Ci::ProjectPipelineStatus.new( - build_stubbed(:project), - pipeline_info: { - sha: "123abc", - status: "success" - } - ) - expect(helper.pipeline_status_cache_key(pipeline_status)).to eq("pipeline-status/123abc-success") - end - end - describe "#render_ci_icon" do subject { helper.render_ci_icon("success") } diff --git a/spec/models/member_spec.rb b/spec/models/member_spec.rb index 32303f76684..38b1cf35bbf 100644 --- a/spec/models/member_spec.rb +++ b/spec/models/member_spec.rb @@ -1095,6 +1095,32 @@ RSpec.describe Member, feature_category: :groups_and_projects do end end + context 'when after_update :post_update_hook' do + let_it_be(:member) { create(:group_member, :developer) } + + context 'when access_level is changed' do + it 'calls NotificationService.update_member' do + expect(NotificationService).to receive_message_chain(:new, :updated_member_access_level).with(member) + + member.update_attribute(:access_level, Member::MAINTAINER) + end + + it 'does not send an email when the access level has not changed' do + expect(NotificationService).not_to receive(:new) + + member.touch + end + end + + context 'when expiration is changed' do + it 'calls the notification service when membership expiry has changed' do + expect(NotificationService).to receive_message_chain(:new, :updated_member_expiration).with(member) + + member.update!(expires_at: 5.days.from_now) + end + end + end + describe 'log_invitation_token_cleanup' do let_it_be(:project) { create :project } diff --git a/spec/models/members/group_member_spec.rb b/spec/models/members/group_member_spec.rb index 7307e76272d..a7508f0e616 100644 --- a/spec/models/members/group_member_spec.rb +++ b/spec/models/members/group_member_spec.rb @@ -202,18 +202,6 @@ RSpec.describe GroupMember, feature_category: :cell do end end - context 'when group member expiration date is updated' do - let_it_be(:group_member) { create(:group_member) } - - it 'emails the user that their group membership expiry has changed' do - expect_next_instance_of(NotificationService) do |notification| - allow(notification).to receive(:updated_group_member_expiration).with(group_member) - end - - group_member.update!(expires_at: 5.days.from_now) - end - end - describe 'refresh_member_authorized_projects' do context 'when importing' do it 'does not refresh' do diff --git a/spec/requests/api/graphql/mutations/ml/models/create_spec.rb b/spec/requests/api/graphql/mutations/ml/models/create_spec.rb new file mode 100644 index 00000000000..0daabeab0d1 --- /dev/null +++ b/spec/requests/api/graphql/mutations/ml/models/create_spec.rb @@ -0,0 +1,48 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe 'Creation of a machine learning model', feature_category: :mlops do + include GraphqlHelpers + + let_it_be(:model) { create(:ml_models) } + let_it_be(:project) { model.project } + let_it_be(:current_user) { project.owner } + + let(:input) { { project_path: project.full_path, name: name, description: description } } + let(:name) { 'some_name' } + let(:description) { 'A description' } + + let(:mutation) { graphql_mutation(:ml_model_create, input) } + let(:mutation_response) { graphql_mutation_response(:ml_model_create) } + + context 'when user is not allowed write changes' do + before do + allow(Ability).to receive(:allowed?).and_call_original + allow(Ability).to receive(:allowed?) + .with(current_user, :write_model_registry, project) + .and_return(false) + end + + it_behaves_like 'a mutation that returns a top-level access error' + end + + context 'when user is allowed write changes' do + it 'creates a models' do + post_graphql_mutation(mutation, current_user: current_user) + + expect(response).to have_gitlab_http_status(:success) + expect(mutation_response['model']).to include( + 'name' => name, + 'description' => description + ) + end + + context 'when name already exists' do + err_msg = "Name has already been taken" + let(:name) { model.name } + + it_behaves_like 'a mutation that returns errors in the response', errors: [err_msg] + end + end +end diff --git a/spec/services/click_house/sync_strategies/base_sync_strategy_spec.rb b/spec/services/click_house/sync_strategies/base_sync_strategy_spec.rb new file mode 100644 index 00000000000..eb9324fd24b --- /dev/null +++ b/spec/services/click_house/sync_strategies/base_sync_strategy_spec.rb @@ -0,0 +1,169 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ClickHouse::SyncStrategies::BaseSyncStrategy, feature_category: :value_stream_management do + let(:strategy) { described_class.new } + + describe '#execute' do + subject(:execute) { strategy.execute } + + context 'when clickhouse configuration database is available', :click_house do + before do + allow(strategy).to receive(:model_class).and_return(::Event) + allow(strategy).to receive(:projections).and_return([:id]) + allow(strategy).to receive(:csv_mapping).and_return({ id: :id }) + allow(strategy).to receive(:insert_query).and_return("INSERT INTO events (id) SETTINGS async_insert=1, + wait_for_async_insert=1 FORMAT CSV") + end + + context 'when there is nothing to sync' do + it 'adds metadata for the worker' do + expect(execute).to eq({ status: :processed, records_inserted: 0, reached_end_of_table: true }) + + events = ClickHouse::Client.select('SELECT * FROM events', :main) + expect(events).to be_empty + end + end + + context 'when syncing records' do + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, group: group) } + let_it_be(:issue) { create(:issue, project: project) } + let_it_be(:project_event2) { create(:event, :closed, project: project, target: issue) } + let_it_be(:event_without_parent) { create(:event, :joined, project: nil, group: nil) } + let_it_be(:group_event) { create(:event, :created, group: group, project: nil) } + let_it_be(:project_event1) { create(:event, :created, project: project, target: issue) } + + it 'inserts all records' do + expect(execute).to eq({ status: :processed, records_inserted: 4, reached_end_of_table: true }) + + expected_records = [ + hash_including('id' => project_event2.id), + hash_including('id' => event_without_parent.id), + hash_including('id' => group_event.id), + hash_including('id' => project_event1.id) + ] + + events = ClickHouse::Client.select('SELECT * FROM events ORDER BY id', :main) + + expect(events).to match(expected_records) + + last_processed_id = ClickHouse::SyncCursor.cursor_for(:events) + expect(last_processed_id).to eq(project_event1.id) + end + + context 'when multiple batches are needed' do + before do + stub_const("#{described_class}::BATCH_SIZE", 1) + stub_const("#{described_class}::INSERT_BATCH_SIZE", 1) + end + + it 'inserts all records' do + expect(execute).to eq({ status: :processed, records_inserted: 4, reached_end_of_table: true }) + + events = ClickHouse::Client.select('SELECT * FROM events', :main) + expect(events.size).to eq(4) + end + + context 'when new records are inserted while processing' do + it 'does not process new records created during the iteration' do + # Simulating the case when there is an insert during the iteration + call_count = 0 + allow(strategy).to receive(:next_batch).and_wrap_original do |method| + call_count += 1 + create(:event) if call_count == 3 + method.call + end + + expect(execute).to eq({ status: :processed, records_inserted: 4, reached_end_of_table: true }) + end + end + end + + context 'when time limit is reached' do + before do + stub_const("#{described_class}::BATCH_SIZE", 1) + end + + it 'stops the processing' do + allow_next_instance_of(Analytics::CycleAnalytics::RuntimeLimiter) do |runtime_limiter| + allow(runtime_limiter).to receive(:over_time?).and_return(false, true) + end + + expect(execute).to eq({ status: :processed, records_inserted: 2, reached_end_of_table: false }) + + last_processed_id = ClickHouse::SyncCursor.cursor_for(:events) + expect(last_processed_id).to eq(event_without_parent.id) + end + end + + context 'when syncing from a certain point' do + before do + ClickHouse::SyncCursor.update_cursor_for(:events, project_event2.id) + end + + it 'syncs records after the cursor' do + expect(execute).to eq({ status: :processed, records_inserted: 3, reached_end_of_table: true }) + + events = ClickHouse::Client.select('SELECT id FROM events ORDER BY id', :main) + + expect(events).to eq([{ 'id' => event_without_parent.id }, { 'id' => group_event.id }, + { 'id' => project_event1.id }]) + end + + context 'when there is nothing to sync' do + it 'does nothing' do + ClickHouse::SyncCursor.update_cursor_for(:events, project_event1.id) + + expect(execute).to eq({ status: :processed, records_inserted: 0, reached_end_of_table: true }) + + events = ClickHouse::Client.select('SELECT id FROM events ORDER BY id', :main) + expect(events).to be_empty + end + end + end + end + end + + context 'when clickhouse is not configured' do + before do + allow(ClickHouse::Client.configuration).to receive(:databases).and_return({}) + end + + it 'skips execution' do + expect(execute).to eq({ status: :disabled }) + end + end + + context 'when exclusive lease error happens' do + it 'skips execution' do + allow(ClickHouse::Client.configuration).to receive(:databases).and_return({ main: :some_db }) + + expect(strategy).to receive(:in_lock).and_raise(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError) + expect(execute).to eq({ status: :skipped }) + end + end + end + + describe '#projections' do + it 'raises a NotImplementedError' do + expect { strategy.send(:projections) }.to raise_error(NotImplementedError, + "Subclasses must implement `projections`") + end + end + + describe '#csv_mapping' do + it 'raises a NotImplementedError' do + expect { strategy.send(:csv_mapping) }.to raise_error(NotImplementedError, + "Subclasses must implement `csv_mapping`") + end + end + + describe '#insert_query' do + it 'raises a NotImplementedError' do + expect { strategy.send(:insert_query) }.to raise_error(NotImplementedError, + "Subclasses must implement `insert_query`") + end + end +end diff --git a/spec/services/click_house/sync_strategies/event_sync_strategy_spec.rb b/spec/services/click_house/sync_strategies/event_sync_strategy_spec.rb new file mode 100644 index 00000000000..05fcf6ddeb3 --- /dev/null +++ b/spec/services/click_house/sync_strategies/event_sync_strategy_spec.rb @@ -0,0 +1,128 @@ +# frozen_string_literal: true + +require 'spec_helper' + +RSpec.describe ClickHouse::SyncStrategies::EventSyncStrategy, feature_category: :value_stream_management do + let(:strategy) { described_class.new } + + describe '#execute' do + subject(:execute) { strategy.execute } + + context 'when syncing records', :click_house do + let_it_be(:group) { create(:group) } + let_it_be(:project) { create(:project, group: group) } + let_it_be(:issue) { create(:issue, project: project) } + let_it_be(:project_event2) { create(:event, :closed, project: project, target: issue) } + let_it_be(:event_without_parent) { create(:event, :joined, project: nil, group: nil) } + let_it_be(:group_event) { create(:event, :created, group: group, project: nil) } + let_it_be(:project_event1) { create(:event, :created, project: project, target: issue) } + # looks invalid but we have some records like this on PRD + + it 'correctly inserts all records' do + expect(execute).to eq({ status: :processed, records_inserted: 4, reached_end_of_table: true }) + + expected_records = [ + hash_including('id' => project_event2.id, 'path' => "#{group.id}/#{project.project_namespace.id}/", + 'target_type' => 'Issue'), + hash_including('id' => event_without_parent.id, 'path' => '', 'target_type' => ''), + hash_including('id' => group_event.id, 'path' => "#{group.id}/", 'target_type' => ''), + hash_including('id' => project_event1.id, 'path' => "#{group.id}/#{project.project_namespace.id}/", + 'target_type' => 'Issue') + ] + + events = ClickHouse::Client.select('SELECT * FROM events ORDER BY id', :main) + + expect(events).to match(expected_records) + + last_processed_id = ClickHouse::SyncCursor.cursor_for(:events) + expect(last_processed_id).to eq(project_event1.id) + end + end + end + + describe '#projections' do + it 'returns correct projections' do + expect(strategy.send(:projections)).to match_array([ + :id, + described_class::PATH_COLUMN, + :author_id, + :target_id, + :target_type, + 'action AS raw_action', + 'EXTRACT(epoch FROM created_at) AS casted_created_at', + 'EXTRACT(epoch FROM updated_at) AS casted_updated_at' + ]) + end + end + + describe '#csv_mapping' do + it 'returns correct csv mapping' do + expect(strategy.send(:csv_mapping)).to eq({ + id: :id, + path: :path, + author_id: :author_id, + target_id: :target_id, + target_type: :target_type, + action: :raw_action, + created_at: :casted_created_at, + updated_at: :casted_updated_at + }) + end + end + + describe '#insert_query' do + let(:expected_query) do + <<~SQL.squish + INSERT INTO events (id, path, author_id, + target_id, target_type, + action, created_at, updated_at) + SETTINGS async_insert=1, + wait_for_async_insert=1 FORMAT CSV + SQL + end + + it 'returns correct insert query' do + expect(strategy.send(:insert_query)).to eq(expected_query) + end + end + + describe '#model_class' do + it 'returns the correct model class' do + expect(strategy.send(:model_class)).to eq(::Event) + end + end + + describe '#enabled?' do + context 'when the clickhouse database is configured the feature flag is enabled' do + before do + allow(ClickHouse::Client.configuration).to receive(:databases).and_return({ main: :some_db }) + stub_feature_flags(event_sync_worker_for_click_house: true) + end + + it 'returns true' do + expect(strategy.send(:enabled?)).to be_truthy + end + end + + context 'when the clickhouse database is not configured' do + before do + allow(ClickHouse::Client.configuration).to receive(:databases).and_return({}) + end + + it 'returns false' do + expect(strategy.send(:enabled?)).to be_falsey + end + end + + context 'when the feature flag is disabled' do + before do + allow(ClickHouse::Client.configuration).to receive(:databases).and_return({ main: :some_db }) + stub_feature_flags(event_sync_worker_for_click_house: false) + end + + it 'returns false' do + expect(strategy.send(:enabled?)).to be_falsey + end + end + end +end diff --git a/spec/services/members/update_service_spec.rb b/spec/services/members/update_service_spec.rb index 3860543a85e..b23f5856575 100644 --- a/spec/services/members/update_service_spec.rb +++ b/spec/services/members/update_service_spec.rb @@ -263,7 +263,7 @@ RSpec.describe Members::UpdateService, feature_category: :groups_and_projects do it 'emails the users that their group membership expiry has changed' do members.each do |member| - expect(notification_service).to receive(:updated_group_member_expiration).with(member) + expect(notification_service).to receive(:updated_member_expiration).with(member) end subject diff --git a/spec/services/ml/create_model_service_spec.rb b/spec/services/ml/create_model_service_spec.rb index 74c1dd5fec7..88e7c00d1f9 100644 --- a/spec/services/ml/create_model_service_spec.rb +++ b/spec/services/ml/create_model_service_spec.rb @@ -50,9 +50,10 @@ RSpec.describe ::Ml::CreateModelService, feature_category: :mlops do let(:name) { existing_model.name } let(:project) { existing_model.project } - it 'raises an error', :aggregate_failures do - expect { create_model }.to raise_error(ActiveRecord::RecordInvalid) + it 'returns a model with errors', :aggregate_failures do + expect(create_model).not_to be_persisted expect(Gitlab::InternalEvents).not_to have_received(:track_event) + expect(create_model.errors.full_messages).to eq(["Name has already been taken"]) end end diff --git a/spec/services/notification_service_spec.rb b/spec/services/notification_service_spec.rb index 0012cc112c1..910e1d85a6b 100644 --- a/spec/services/notification_service_spec.rb +++ b/spec/services/notification_service_spec.rb @@ -3319,18 +3319,6 @@ RSpec.describe NotificationService, :mailer, feature_category: :team_planning do let(:notification_trigger) { group.add_guest(added_user) } end end - - describe '#updated_group_member_expiration' do - let_it_be(:group_member) { create(:group_member) } - - it 'emails the user that their group membership expiry has changed' do - expect_next_instance_of(NotificationService) do |notification| - allow(notification).to receive(:updated_group_member_expiration).with(group_member) - end - - group_member.update!(expires_at: 5.days.from_now) - end - end end describe 'ProjectMember', :deliver_mails_inline do @@ -3509,6 +3497,42 @@ RSpec.describe NotificationService, :mailer, feature_category: :team_planning do end end + describe '#updated_member_expiration' do + subject(:updated_member_expiration) { notification.updated_member_expiration(member) } + + context 'for group member' do + let_it_be(:member) { create(:group_member) } + + it 'triggers a notification about the expiration change' do + updated_member_expiration + + expect_delivery_jobs_count(1) + expect_enqueud_email('Group', member.id, mail: 'member_expiration_date_updated_email') + end + end + + context 'for project member' do + let_it_be(:member) { create(:project_member) } + + it 'does not trigger a notification' do + updated_member_expiration + + expect_delivery_jobs_count(0) + end + end + end + + describe '#updated_member_access_level' do + let_it_be(:member) { create(:group_member) } + + it 'triggers a notification about the access_level change' do + notification.updated_member_access_level(member) + + expect_delivery_jobs_count(1) + expect_enqueud_email('Group', member.id, mail: 'member_access_granted_email') + end + end + context 'guest user in private project', :deliver_mails_inline do let(:private_project) { create(:project, :private) } let(:guest) { create(:user) } diff --git a/spec/support/rspec_order_todo.yml b/spec/support/rspec_order_todo.yml index 56a28004233..0715e56c130 100644 --- a/spec/support/rspec_order_todo.yml +++ b/spec/support/rspec_order_todo.yml @@ -1691,7 +1691,7 @@ - './ee/spec/models/gitlab_subscription_spec.rb' - './ee/spec/models/gitlab_subscriptions/upcoming_reconciliation_spec.rb' - './ee/spec/models/group_deletion_schedule_spec.rb' -- './ee/spec/models/group_member_spec.rb' +- './ee/spec/models/ee/group_member_spec.rb' - './ee/spec/models/group_merge_request_approval_setting_spec.rb' - './ee/spec/models/groups/repository_storage_move_spec.rb' - './ee/spec/models/group_wiki_repository_spec.rb' @@ -1715,7 +1715,7 @@ - './ee/spec/models/label_note_spec.rb' - './ee/spec/models/ldap_group_link_spec.rb' - './ee/spec/models/license_spec.rb' -- './ee/spec/models/member_spec.rb' +- './ee/spec/models/ee/member_spec.rb' - './ee/spec/models/merge_request/blocking_spec.rb' - './ee/spec/models/merge_request_block_spec.rb' - './ee/spec/models/merge_requests/compliance_violation_spec.rb' @@ -1740,7 +1740,7 @@ - './ee/spec/models/project_feature_spec.rb' - './ee/spec/models/project_import_data_spec.rb' - './ee/spec/models/project_import_state_spec.rb' -- './ee/spec/models/project_member_spec.rb' +- './ee/spec/models/ee/project_member_spec.rb' - './ee/spec/models/project_repository_state_spec.rb' - './ee/spec/models/project_security_setting_spec.rb' - './ee/spec/models/protected_branch/required_code_owners_section_spec.rb' diff --git a/spec/support/shared_examples/models/members_notifications_shared_example.rb b/spec/support/shared_examples/models/members_notifications_shared_example.rb index 5c783b5cfa7..17dc927f3cc 100644 --- a/spec/support/shared_examples/models/members_notifications_shared_example.rb +++ b/spec/support/shared_examples/models/members_notifications_shared_example.rb @@ -19,22 +19,6 @@ RSpec.shared_examples 'members notifications' do |entity_type| end end - describe "#after_update" do - let(:member) { create(:"#{entity_type}_member", :developer) } - - it "calls NotificationService.update_#{entity_type}_member" do - expect(notification_service).to receive(:"update_#{entity_type}_member").with(member) - - member.update_attribute(:access_level, Member::MAINTAINER) - end - - it "does not send an email when the access level has not changed" do - expect(notification_service).not_to receive(:"update_#{entity_type}_member") - - member.touch - end - end - describe '#after_commit' do context 'on creation of a member requesting access' do let(:member) do diff --git a/spec/workers/click_house/events_sync_worker_spec.rb b/spec/workers/click_house/events_sync_worker_spec.rb index 9662f26115a..dc3dea24e37 100644 --- a/spec/workers/click_house/events_sync_worker_spec.rb +++ b/spec/workers/click_house/events_sync_worker_spec.rb @@ -11,176 +11,20 @@ RSpec.describe ClickHouse::EventsSyncWorker, feature_category: :value_stream_man ) end - it_behaves_like 'an idempotent worker' do - context 'when the event_sync_worker_for_click_house feature flag is on', :click_house do - before do - stub_feature_flags(event_sync_worker_for_click_house: true) + context 'when worker is enqueued' do + it 'calls ::ClickHouse::SyncStrategies::EventSyncStrategy with correct args' do + expect_next_instance_of(::ClickHouse::SyncStrategies::EventSyncStrategy) do |instance| + expect(instance).to receive(:execute) end - context 'when there is nothing to sync' do - it 'adds metadata for the worker' do - expect(worker).to receive(:log_extra_metadata_on_done).with(:result, - { status: :processed, records_inserted: 0, reached_end_of_table: true }) - - worker.perform - - events = ClickHouse::Client.select('SELECT * FROM events', :main) - expect(events).to be_empty - end - end - - context 'when syncing records' do - let_it_be(:group) { create(:group) } - let_it_be(:project) { create(:project, group: group) } - let_it_be(:issue) { create(:issue, project: project) } - let_it_be(:project_event2) { create(:event, :closed, project: project, target: issue) } - let_it_be(:event_without_parent) { create(:event, :joined, project: nil, group: nil) } - let_it_be(:group_event) { create(:event, :created, group: group, project: nil) } - let_it_be(:project_event1) { create(:event, :created, project: project, target: issue) } - # looks invalid but we have some records like this on PRD - - it 'inserts all records' do - expect(worker).to receive(:log_extra_metadata_on_done).with(:result, - { status: :processed, records_inserted: 4, reached_end_of_table: true }) - - worker.perform - - expected_records = [ - hash_including('id' => project_event2.id, 'path' => "#{group.id}/#{project.project_namespace.id}/", - 'target_type' => 'Issue'), - hash_including('id' => event_without_parent.id, 'path' => '', 'target_type' => ''), - hash_including('id' => group_event.id, 'path' => "#{group.id}/", 'target_type' => ''), - hash_including('id' => project_event1.id, 'path' => "#{group.id}/#{project.project_namespace.id}/", - 'target_type' => 'Issue') - ] - - events = ClickHouse::Client.select('SELECT * FROM events ORDER BY id', :main) - - expect(events).to match(expected_records) - - last_processed_id = ClickHouse::SyncCursor.cursor_for(:events) - expect(last_processed_id).to eq(project_event1.id) - end - - context 'when multiple batches are needed' do - before do - stub_const("#{described_class}::BATCH_SIZE", 1) - stub_const("#{described_class}::INSERT_BATCH_SIZE", 1) - end - - it 'inserts all records' do - expect(worker).to receive(:log_extra_metadata_on_done).with(:result, - { status: :processed, records_inserted: 4, reached_end_of_table: true }) - - worker.perform - - events = ClickHouse::Client.select('SELECT * FROM events', :main) - expect(events.size).to eq(4) - end - - context 'when new records are inserted while processing' do - it 'does not process new records created during the iteration' do - expect(worker).to receive(:log_extra_metadata_on_done).with(:result, - { status: :processed, records_inserted: 4, - reached_end_of_table: true }) - - # Simulating the case when there is an insert during the iteration - call_count = 0 - allow(worker).to receive(:next_batch).and_wrap_original do |method| - call_count += 1 - create(:event) if call_count == 3 - method.call - end - - worker.perform - end - end - end - - context 'when time limit is reached' do - before do - stub_const("#{described_class}::BATCH_SIZE", 1) - end - - it 'stops the processing' do - allow_next_instance_of(Analytics::CycleAnalytics::RuntimeLimiter) do |runtime_limiter| - allow(runtime_limiter).to receive(:over_time?).and_return(false, true) - end - - expect(worker).to receive(:log_extra_metadata_on_done).with(:result, - { status: :processed, records_inserted: 2, reached_end_of_table: false }) - - worker.perform - - last_processed_id = ClickHouse::SyncCursor.cursor_for(:events) - expect(last_processed_id).to eq(event_without_parent.id) - end - end - - context 'when syncing from a certain point' do - before do - ClickHouse::SyncCursor.update_cursor_for(:events, project_event2.id) - end - - it 'syncs records after the cursor' do - expect(worker).to receive(:log_extra_metadata_on_done).with(:result, - { status: :processed, records_inserted: 3, reached_end_of_table: true }) - - worker.perform - - events = ClickHouse::Client.select('SELECT id FROM events ORDER BY id', :main) - expect(events).to eq([{ 'id' => event_without_parent.id }, { 'id' => group_event.id }, - { 'id' => project_event1.id }]) - end - - context 'when there is nothing to sync' do - it 'does nothing' do - expect(worker).to receive(:log_extra_metadata_on_done).with(:result, - { status: :processed, records_inserted: 0, reached_end_of_table: true }) - - ClickHouse::SyncCursor.update_cursor_for(:events, project_event1.id) - worker.perform - - events = ClickHouse::Client.select('SELECT id FROM events ORDER BY id', :main) - expect(events).to be_empty - end - end - end - end - end - - context 'when clickhouse is not configured' do - before do - allow(ClickHouse::Client).to receive(:database_configured?).and_return(false) - end - - it 'skips execution' do - expect(worker).to receive(:log_extra_metadata_on_done).with(:result, { status: :disabled }) - - worker.perform - end - end - end - - context 'when exclusive lease error happens' do - it 'skips execution' do - stub_feature_flags(event_sync_worker_for_click_house: true) - allow(ClickHouse::Client).to receive(:database_configured?).with(:main).and_return(true) - - expect(worker).to receive(:in_lock).and_raise(Gitlab::ExclusiveLeaseHelpers::FailedToObtainLockError) - expect(worker).to receive(:log_extra_metadata_on_done).with(:result, { status: :skipped }) - worker.perform end - end - context 'when the event_sync_worker_for_click_house feature flag is off' do - before do - stub_feature_flags(event_sync_worker_for_click_house: false) - end - - it 'skips execution' do - expect(worker).to receive(:log_extra_metadata_on_done).with(:result, { status: :disabled }) + it 'correctly logs the metadata on done' do + expect_next_instance_of(::ClickHouse::SyncStrategies::EventSyncStrategy) do |instance| + expect(instance).to receive(:execute).and_return({ status: :ok }) + end + expect(worker).to receive(:log_extra_metadata_on_done).with(:result, { status: :ok }) worker.perform end |