From 4374a38f0e8fbd856458a60f47083fb4d084d695 Mon Sep 17 00:00:00 2001 From: "micael.bergeron" Date: Wed, 15 Nov 2017 09:12:04 -0500 Subject: fix the commit reference pattern from matching other entities --- app/models/commit.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/commit.rb b/app/models/commit.rb index 6dba154a6ea..e28bf28fab3 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -26,7 +26,7 @@ class Commit DIFF_HARD_LIMIT_LINES = 50000 MIN_SHA_LENGTH = 7 - COMMIT_SHA_PATTERN = /\h{#{MIN_SHA_LENGTH},40}/.freeze + COMMIT_SHA_PATTERN = /\b(? Date: Wed, 15 Nov 2017 10:56:17 -0500 Subject: try to fix the failing spec on external refs --- app/models/commit.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/commit.rb b/app/models/commit.rb index e28bf28fab3..b5411b2bf75 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -26,7 +26,7 @@ class Commit DIFF_HARD_LIMIT_LINES = 50000 MIN_SHA_LENGTH = 7 - COMMIT_SHA_PATTERN = /\b(? Date: Wed, 15 Nov 2017 15:47:10 +0100 Subject: Cache the number of user SSH keys By caching the number of personal SSH keys we reduce the number of queries necessary on pages such as ProjectsController#show (which can end up querying this data multiple times). The cache is refreshed/flushed whenever an SSH key is added, removed, or when a user is removed. --- app/models/key.rb | 8 ++++++++ app/models/user.rb | 9 ++++++++- 2 files changed, 16 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/key.rb b/app/models/key.rb index f119b15c737..815fd1de909 100644 --- a/app/models/key.rb +++ b/app/models/key.rb @@ -27,8 +27,10 @@ class Key < ActiveRecord::Base after_commit :add_to_shell, on: :create after_create :post_create_hook + after_create :refresh_user_cache after_commit :remove_from_shell, on: :destroy after_destroy :post_destroy_hook + after_destroy :refresh_user_cache def key=(value) value&.delete!("\n\r") @@ -76,6 +78,12 @@ class Key < ActiveRecord::Base ) end + def refresh_user_cache + return unless user + + Users::KeysCountService.new(user).refresh_cache + end + def post_destroy_hook SystemHooksService.new.execute_hooks_for(self, :destroy) end diff --git a/app/models/user.rb b/app/models/user.rb index ea10e2854d6..be8112749bf 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -170,6 +170,7 @@ class User < ActiveRecord::Base after_save :ensure_namespace_correct after_update :username_changed_hook, if: :username_changed? after_destroy :post_destroy_hook + after_destroy :remove_key_cache after_commit :update_emails_with_primary_email, on: :update, if: -> { previous_changes.key?('email') } after_commit :update_invalid_gpg_signatures, on: :update, if: -> { previous_changes.key?('email') } @@ -624,7 +625,9 @@ class User < ActiveRecord::Base end def require_ssh_key? - keys.count == 0 && Gitlab::ProtocolAccess.allowed?('ssh') + count = Users::KeysCountService.new(self).count + + count.zero? && Gitlab::ProtocolAccess.allowed?('ssh') end def require_password_creation? @@ -886,6 +889,10 @@ class User < ActiveRecord::Base system_hook_service.execute_hooks_for(self, :destroy) end + def remove_key_cache + Users::KeysCountService.new(self).delete_cache + end + def delete_async(deleted_by:, params: {}) block if params[:hard_delete] DeleteUserWorker.perform_async(deleted_by.id, id, params) -- cgit v1.2.1 From 71b2cc1dd8497959306601eece8ebbf008562d07 Mon Sep 17 00:00:00 2001 From: "micael.bergeron" Date: Thu, 16 Nov 2017 09:23:32 -0500 Subject: reverting to the simpler approach --- app/models/commit.rb | 2 +- app/models/note.rb | 14 +++++++++++++- app/models/system_note_metadata.rb | 10 ++++++++++ 3 files changed, 24 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/commit.rb b/app/models/commit.rb index b5411b2bf75..6dba154a6ea 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -26,7 +26,7 @@ class Commit DIFF_HARD_LIMIT_LINES = 50000 MIN_SHA_LENGTH = 7 - COMMIT_SHA_PATTERN = /(? Date: Fri, 10 Nov 2017 20:57:11 +0100 Subject: Optimise getting the pipeline status of commits This adds an optimised way of getting the latest pipeline status for a list of Commit objects (or just a single one). --- app/models/ci/pipeline.rb | 66 +++++++++++++++++++++++++++++++--------- app/models/commit.rb | 9 ++++-- app/models/commit_collection.rb | 44 +++++++++++++++++++++++++++ app/models/merge_request_diff.rb | 4 ++- app/models/repository.rb | 16 ++++++---- 5 files changed, 114 insertions(+), 25 deletions(-) create mode 100644 app/models/commit_collection.rb (limited to 'app/models') diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 19814864e50..c77c837ff3f 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -149,34 +149,70 @@ module Ci end end - # ref can't be HEAD or SHA, can only be branch/tag name - scope :latest, ->(ref = nil) do - max_id = unscope(:select) - .select("max(#{quoted_table_name}.id)") - .group(:ref, :sha) - - if ref - where(ref: ref, id: max_id.where(ref: ref)) - else - where(id: max_id) - end - end scope :internal, -> { where(source: internal_sources) } + # Returns the pipelines in descending order (= newest first), optionally + # limited to a number of references. + # + # ref - The name (or names) of the branch(es)/tag(s) to limit the list of + # pipelines to. + def self.newest_first(ref = nil) + relation = order(id: :desc) + + ref ? relation.where(ref: ref) : relation + end + def self.latest_status(ref = nil) - latest(ref).status + newest_first(ref).pluck(:status).first end def self.latest_successful_for(ref) - success.latest(ref).order(id: :desc).first + newest_first(ref).success.take end def self.latest_successful_for_refs(refs) - success.latest(refs).order(id: :desc).each_with_object({}) do |pipeline, hash| + relation = newest_first(refs).success + + relation.each_with_object({}) do |pipeline, hash| hash[pipeline.ref] ||= pipeline end end + # Returns a Hash containing the latest pipeline status for every given + # commit. + # + # The keys of this Hash are the commit SHAs, the values the statuses. + # + # commits - The list of commit SHAs to get the status for. + # ref - The ref to scope the data to (e.g. "master"). If the ref is not + # given we simply get the latest status for the commits, regardless + # of what refs their pipelines belong to. + def self.latest_status_per_commit(commits, ref = nil) + p1 = arel_table + p2 = arel_table.alias + + # This LEFT JOIN will filter out all but the newest row for every + # combination of (project_id, sha) or (project_id, sha, ref) if a ref is + # given. + cond = p1[:sha].eq(p2[:sha]) + .and(p1[:project_id].eq(p2[:project_id])) + .and(p1[:id].lt(p2[:id])) + + cond = cond.and(p1[:ref].eq(p2[:ref])) if ref + join = p1.join(p2, Arel::Nodes::OuterJoin).on(cond) + + relation = select(:sha, :status) + .where(sha: commits) + .where(p2[:id].eq(nil)) + .joins(join.join_sources) + + relation = relation.where(ref: ref) if ref + + relation.each_with_object({}) do |row, hash| + hash[row[:sha]] = row[:status] + end + end + def self.truncate_sha(sha) sha[0...8] end diff --git a/app/models/commit.rb b/app/models/commit.rb index 6dba154a6ea..a31ebe9cc87 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -80,6 +80,7 @@ class Commit @raw = raw_commit @project = project + @statuses = {} end def id @@ -236,11 +237,13 @@ class Commit end def status(ref = nil) - @statuses ||= {} - return @statuses[ref] if @statuses.key?(ref) - @statuses[ref] = pipelines.latest_status(ref) + @statuses[ref] = project.pipelines.latest_status_per_commit(id, ref)[id] + end + + def set_status_for_ref(ref, status) + @statuses[ref] = status end def signature diff --git a/app/models/commit_collection.rb b/app/models/commit_collection.rb new file mode 100644 index 00000000000..dd93af9df64 --- /dev/null +++ b/app/models/commit_collection.rb @@ -0,0 +1,44 @@ +# frozen_string_literal: true + +# A collection of Commit instances for a specific project and Git reference. +class CommitCollection + include Enumerable + + attr_reader :project, :ref, :commits + + # project - The project the commits belong to. + # commits - The Commit instances to store. + # ref - The name of the ref (e.g. "master"). + def initialize(project, commits, ref = nil) + @project = project + @commits = commits + @ref = ref + end + + def each(&block) + commits.each(&block) + end + + # Sets the pipeline status for every commit. + # + # Setting this status ahead of time removes the need for running a query for + # every commit we're displaying. + def with_pipeline_status + statuses = project.pipelines.latest_status_per_commit(map(&:id), ref) + + each do |commit| + commit.set_status_for_ref(ref, statuses[commit.id]) + end + + self + end + + def respond_to_missing?(message, inc_private = false) + commits.respond_to?(message, inc_private) + end + + # rubocop:disable GitlabSecurity/PublicSend + def method_missing(message, *args, &block) + commits.public_send(message, *args, &block) + end +end diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 1eda0f9cbbd..5382f5cc627 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -284,8 +284,10 @@ class MergeRequestDiff < ActiveRecord::Base def load_commits commits = st_commits.presence || merge_request_diff_commits + commits = commits.map { |commit| Commit.from_hash(commit.to_hash, project) } - commits.map { |commit| Commit.from_hash(commit.to_hash, project) } + CommitCollection + .new(merge_request.source_project, commits, merge_request.source_branch) end def save_diffs diff --git a/app/models/repository.rb b/app/models/repository.rb index 3a89fa9264b..35ee12bdfa1 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -132,7 +132,8 @@ class Repository commits = Gitlab::Git::Commit.where(options) commits = Commit.decorate(commits, @project) if commits.present? - commits + + CommitCollection.new(project, commits, ref) end def commits_between(from, to) @@ -148,11 +149,14 @@ class Repository end raw_repository.gitaly_migrate(:commits_by_message) do |is_enabled| - if is_enabled - find_commits_by_message_by_gitaly(query, ref, path, limit, offset) - else - find_commits_by_message_by_shelling_out(query, ref, path, limit, offset) - end + commits = + if is_enabled + find_commits_by_message_by_gitaly(query, ref, path, limit, offset) + else + find_commits_by_message_by_shelling_out(query, ref, path, limit, offset) + end + + CommitCollection.new(project, commits, ref) end end -- cgit v1.2.1 From 869fc05d81eaced5d68ef28e334ae9ece80f74f7 Mon Sep 17 00:00:00 2001 From: "micael.bergeron" Date: Thu, 16 Nov 2017 11:03:15 -0500 Subject: fix the linting error --- app/models/system_note_metadata.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb index 4065c1594c3..ca0673080af 100644 --- a/app/models/system_note_metadata.rb +++ b/app/models/system_note_metadata.rb @@ -7,7 +7,7 @@ class SystemNoteMetadata < ActiveRecord::Base TYPES_WITH_CROSS_REFERENCES = %w[ cross_reference milestone - ] + ].freeze ICON_TYPES = %w[ commit description merge confidential visible label assignee cross_reference -- cgit v1.2.1 From 181cd299f9e06223e8338e93b1c318c671ccb1aa Mon Sep 17 00:00:00 2001 From: Jacopo Date: Tue, 14 Nov 2017 10:02:39 +0100 Subject: Adds Rubocop rule for line break after guard clause Adds a rubocop rule (with autocorrect) to ensure line break after guard clauses. --- app/models/ci/build.rb | 1 + app/models/ci/pipeline.rb | 2 ++ app/models/clusters/providers/gcp.rb | 1 + app/models/concerns/awardable.rb | 1 + app/models/pages_domain.rb | 5 +++++ app/models/project_services/hipchat_service.rb | 2 ++ app/models/project_services/jira_service.rb | 1 + app/models/project_services/kubernetes_service.rb | 1 + app/models/repository.rb | 2 ++ app/models/user.rb | 2 ++ 10 files changed, 18 insertions(+) (limited to 'app/models') diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 1b2b0d17910..1d9f367183e 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -317,6 +317,7 @@ module Ci def execute_hooks return unless project + build_data = Gitlab::DataBuilder::Build.build(self) project.execute_hooks(build_data.dup, :job_hooks) project.execute_services(build_data.dup, :job_hooks) diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 19814864e50..bcc865357ff 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -300,8 +300,10 @@ module Ci def latest? return false unless ref + commit = project.commit(ref) return false unless commit + commit.sha == sha end diff --git a/app/models/clusters/providers/gcp.rb b/app/models/clusters/providers/gcp.rb index ee2e43ee9dd..7fac32466ab 100644 --- a/app/models/clusters/providers/gcp.rb +++ b/app/models/clusters/providers/gcp.rb @@ -56,6 +56,7 @@ module Clusters before_transition any => [:creating] do |provider, transition| operation_id = transition.args.first raise ArgumentError.new('operation_id is required') unless operation_id.present? + provider.operation_id = operation_id end diff --git a/app/models/concerns/awardable.rb b/app/models/concerns/awardable.rb index 9adc309a22b..d8394415362 100644 --- a/app/models/concerns/awardable.rb +++ b/app/models/concerns/awardable.rb @@ -98,6 +98,7 @@ module Awardable def create_award_emoji(name, current_user) return unless emoji_awardable? + award_emoji.create(name: normalize_name(name), user: current_user) end diff --git a/app/models/pages_domain.rb b/app/models/pages_domain.rb index 43c77f3f2a2..8de42ff9d2e 100644 --- a/app/models/pages_domain.rb +++ b/app/models/pages_domain.rb @@ -65,6 +65,7 @@ class PagesDomain < ActiveRecord::Base def expired? return false unless x509 + current = Time.new current < x509.not_before || x509.not_after < current end @@ -75,6 +76,7 @@ class PagesDomain < ActiveRecord::Base def subject return unless x509 + x509.subject.to_s end @@ -102,6 +104,7 @@ class PagesDomain < ActiveRecord::Base def validate_pages_domain return unless domain + if domain.downcase.ends_with?(Settings.pages.host.downcase) self.errors.add(:domain, "*.#{Settings.pages.host} is restricted") end @@ -109,6 +112,7 @@ class PagesDomain < ActiveRecord::Base def x509 return unless certificate + @x509 ||= OpenSSL::X509::Certificate.new(certificate) rescue OpenSSL::X509::CertificateError nil @@ -116,6 +120,7 @@ class PagesDomain < ActiveRecord::Base def pkey return unless key + @pkey ||= OpenSSL::PKey::RSA.new(key) rescue OpenSSL::PKey::PKeyError, OpenSSL::Cipher::CipherError nil diff --git a/app/models/project_services/hipchat_service.rb b/app/models/project_services/hipchat_service.rb index 976d85246a8..768f0a7472e 100644 --- a/app/models/project_services/hipchat_service.rb +++ b/app/models/project_services/hipchat_service.rb @@ -51,8 +51,10 @@ class HipchatService < Service def execute(data) return unless supported_events.include?(data[:object_kind]) + message = create_message(data) return unless message.present? + gate[room].send('GitLab', message, message_options(data)) # rubocop:disable GitlabSecurity/PublicSend end diff --git a/app/models/project_services/jira_service.rb b/app/models/project_services/jira_service.rb index b487378edd2..1c065e1ddbd 100644 --- a/app/models/project_services/jira_service.rb +++ b/app/models/project_services/jira_service.rb @@ -176,6 +176,7 @@ class JiraService < IssueTrackerService def test_settings return unless client_url.present? + # Test settings by getting the project jira_request { client.ServerInfo.all.attrs } end diff --git a/app/models/project_services/kubernetes_service.rb b/app/models/project_services/kubernetes_service.rb index 5080acffb3c..bc62972dbb0 100644 --- a/app/models/project_services/kubernetes_service.rb +++ b/app/models/project_services/kubernetes_service.rb @@ -182,6 +182,7 @@ class KubernetesService < DeploymentService kubeclient.get_pods(namespace: actual_namespace).as_json rescue KubeException => err raise err unless err.error_code == 404 + [] end diff --git a/app/models/repository.rb b/app/models/repository.rb index 3a89fa9264b..845929fb2cc 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -242,6 +242,7 @@ class Repository Rails.logger.error "Unable to create #{REF_KEEP_AROUND} reference for repository #{path}: #{ex}" rescue Rugged::OSError => ex raise unless ex.message =~ /Failed to create locked file/ && ex.message =~ /File exists/ + Rails.logger.error "Unable to create #{REF_KEEP_AROUND} reference for repository #{path}: #{ex}" end end @@ -662,6 +663,7 @@ class Repository def next_branch(name, opts = {}) branch_ids = self.branch_names.map do |n| next 1 if n == name + result = n.match(/\A#{name}-([0-9]+)\z/) result[1].to_i if result end.compact diff --git a/app/models/user.rb b/app/models/user.rb index ea10e2854d6..5170d87e850 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -1119,6 +1119,7 @@ class User < ActiveRecord::Base # override, from Devise::Validatable def password_required? return false if internal? + super end @@ -1136,6 +1137,7 @@ class User < ActiveRecord::Base # Added according to https://github.com/plataformatec/devise/blob/7df57d5081f9884849ca15e4fde179ef164a575f/README.md#activejob-integration def send_devise_notification(notification, *args) return true unless can?(:receive_notifications) + devise_mailer.__send__(notification, self, *args).deliver_later # rubocop:disable GitlabSecurity/PublicSend end -- cgit v1.2.1 From 732b122644bf56729996b3cc239453f537a798f4 Mon Sep 17 00:00:00 2001 From: Michael Kozono Date: Fri, 6 Oct 2017 16:33:49 -0700 Subject: Add throttle application settings --- app/models/application_setting.rb | 9 +++++++++ 1 file changed, 9 insertions(+) (limited to 'app/models') diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 5e16badabec..a7e0219b03a 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -295,6 +295,15 @@ class ApplicationSetting < ActiveRecord::Base sign_in_text: nil, signup_enabled: Settings.gitlab['signup_enabled'], terminal_max_session_time: 0, + throttle_unauthenticated_enabled: false, + throttle_unauthenticated_requests_per_period: 3600, + throttle_unauthenticated_period_in_seconds: 3600, + throttle_authenticated_web_enabled: false, + throttle_authenticated_web_requests_per_period: 7200, + throttle_authenticated_web_period_in_seconds: 3600, + throttle_authenticated_api_enabled: false, + throttle_authenticated_api_requests_per_period: 7200, + throttle_authenticated_api_period_in_seconds: 3600, two_factor_grace_period: 48, user_default_external: false, polling_interval_multiplier: 1, -- cgit v1.2.1 From ff26ea818cb92eabc8fbea1a8385cb5bc6b0a66f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Fri, 17 Nov 2017 11:48:32 +0000 Subject: Resolve "Performance issues when loading large number of wiki pages" --- app/models/project_wiki.rb | 4 ++-- app/models/wiki_page.rb | 19 ++++++++++++------- 2 files changed, 14 insertions(+), 9 deletions(-) (limited to 'app/models') diff --git a/app/models/project_wiki.rb b/app/models/project_wiki.rb index 3eecbea8cbf..a0af749a93f 100644 --- a/app/models/project_wiki.rb +++ b/app/models/project_wiki.rb @@ -76,8 +76,8 @@ class ProjectWiki # Returns an Array of Gitlab WikiPage instances or an # empty Array if this Wiki has no pages. - def pages - wiki.pages.map { |page| WikiPage.new(self, page, true) } + def pages(limit: nil) + wiki.pages(limit: limit).map { |page| WikiPage.new(self, page, true) } end # Finds a page within the repository based on a tile diff --git a/app/models/wiki_page.rb b/app/models/wiki_page.rb index 5f710961f95..bdfef677ef3 100644 --- a/app/models/wiki_page.rb +++ b/app/models/wiki_page.rb @@ -127,19 +127,24 @@ class WikiPage @version ||= @page.version end - # Returns an array of Gitlab Commit instances. - def versions + def versions(options = {}) return [] unless persisted? - wiki.wiki.page_versions(@page.path) + wiki.wiki.page_versions(@page.path, options) end - def commit - versions.first + def count_versions + return [] unless persisted? + + wiki.wiki.count_page_versions(@page.path) + end + + def last_version + @last_version ||= versions(limit: 1).first end def last_commit_sha - commit&.sha + last_version&.sha end # Returns the Date that this latest version was @@ -151,7 +156,7 @@ class WikiPage # Returns boolean True or False if this instance # is an old version of the page. def historical? - @page.historical? && versions.first.sha != version.sha + @page.historical? && last_version.sha != version.sha end # Returns boolean True or False if this instance -- cgit v1.2.1 From 8b426b63d6044746a71a63b07a709bdc9a2ee832 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Mon, 13 Nov 2017 18:13:03 +0100 Subject: Delete the fork network when removing the last membership --- app/models/fork_network_member.rb | 10 ++++++++++ 1 file changed, 10 insertions(+) (limited to 'app/models') diff --git a/app/models/fork_network_member.rb b/app/models/fork_network_member.rb index 6a9b52a1ef8..eb9417dc34f 100644 --- a/app/models/fork_network_member.rb +++ b/app/models/fork_network_member.rb @@ -4,4 +4,14 @@ class ForkNetworkMember < ActiveRecord::Base belongs_to :forked_from_project, class_name: 'Project' validates :fork_network, :project, presence: true + + after_destroy :cleanup_fork_network + + private + + def cleanup_fork_network + # Explicitly using `#count` makes sure we have the correct number if the + # relation was loaded in the fork_network. + fork_network.destroy if fork_network.fork_network_members.count == 0 + end end -- cgit v1.2.1 From c7cf68bd6ff744e044944acad586e06badc481d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Fri, 17 Nov 2017 14:24:25 +0000 Subject: Changing OAuth lookup to be case insensitive --- app/models/identity.rb | 15 ++++++++++++--- app/models/user.rb | 3 +-- 2 files changed, 13 insertions(+), 5 deletions(-) (limited to 'app/models') diff --git a/app/models/identity.rb b/app/models/identity.rb index ac8094b610e..ff811e19f8a 100644 --- a/app/models/identity.rb +++ b/app/models/identity.rb @@ -1,18 +1,27 @@ class Identity < ActiveRecord::Base include Sortable include CaseSensitivity + belongs_to :user validates :provider, presence: true - validates :extern_uid, allow_blank: true, uniqueness: { scope: :provider } + validates :extern_uid, allow_blank: true, uniqueness: { scope: :provider, case_sensitive: false } validates :user_id, uniqueness: { scope: :provider } + scope :with_provider, ->(provider) { where(provider: provider) } scope :with_extern_uid, ->(provider, extern_uid) do - extern_uid = Gitlab::LDAP::Person.normalize_dn(extern_uid) if provider.starts_with?('ldap') - where(extern_uid: extern_uid, provider: provider) + iwhere(extern_uid: normalize_uid(provider, extern_uid)).with_provider(provider) end def ldap? provider.starts_with?('ldap') end + + def self.normalize_uid(provider, uid) + if provider.to_s.starts_with?('ldap') + Gitlab::LDAP::Person.normalize_dn(uid) + else + uid.to_s + end + end end diff --git a/app/models/user.rb b/app/models/user.rb index be8112749bf..71c34766451 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -269,8 +269,7 @@ class User < ActiveRecord::Base end def for_github_id(id) - joins(:identities) - .where(identities: { provider: :github, extern_uid: id.to_s }) + joins(:identities).merge(Identity.with_extern_uid(:github, id)) end # Find a User by their primary email or any associated secondary email -- cgit v1.2.1 From 64a9e53bd16092e869f88e42a3e69f3f4ba0a23e Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 17 Nov 2017 17:57:48 +0000 Subject: Fix conflict highlighting Conflicts used to take a `Repository` and pass that to `Gitlab::Highlight.highlight`, which would call `#gitattribute` on the repository. Now they use a `Gitlab::Git::Repository`, which didn't have that method defined - but defining it on `Gitlab::Git::Repository` does make it available on `Repository` through `method_missing`, so we can do that and both cases will work. --- app/models/repository.rb | 4 ---- 1 file changed, 4 deletions(-) (limited to 'app/models') diff --git a/app/models/repository.rb b/app/models/repository.rb index 35ee12bdfa1..214f2daa199 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -994,10 +994,6 @@ class Repository raw_repository.ls_files(actual_ref) end - def gitattribute(path, name) - raw_repository.attributes(path)[name] - end - def copy_gitattributes(ref) actual_ref = ref || root_ref begin -- cgit v1.2.1 From b3a5abe4aedd1d4f4a22e01f1f5a5b6e146d6eec Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Xurxo=20Me=CC=81ndez=20Pe=CC=81rez?= Date: Sat, 18 Nov 2017 19:22:11 +0100 Subject: Changed validation error message on wrong milestone dates --- app/models/milestone.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/milestone.rb b/app/models/milestone.rb index 47e6b785c39..e01e52131f0 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -256,7 +256,7 @@ class Milestone < ActiveRecord::Base def start_date_should_be_less_than_due_date if due_date <= start_date - errors.add(:start_date, "Can't be greater than due date") + errors.add(:due_date, "must be greater than start date") end end -- cgit v1.2.1 From d2699aea57eaef6adee25bb453a6096b80dda28f Mon Sep 17 00:00:00 2001 From: Oswaldo Ferreira Date: Mon, 13 Nov 2017 21:36:18 -0200 Subject: Add issue sidebar and toggle_subscription endpoint in board issues data --- app/models/issue.rb | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/issue.rb b/app/models/issue.rb index b5abc8f57b0..a9863a50d84 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -246,7 +246,12 @@ class Issue < ActiveRecord::Base def as_json(options = {}) super(options).tap do |json| - json[:subscribed] = subscribed?(options[:user], project) if options.key?(:user) && options[:user] + if options.key?(:sidebar_endpoints) && project + url_helper = Gitlab::Routing.url_helpers + + json.merge!(issue_sidebar_endpoint: url_helper.project_issue_path(project, self, format: :json, serializer: 'sidebar'), + toggle_subscription_endpoint: url_helper.toggle_subscription_project_issue_path(project, self)) + end if options.key?(:labels) json[:labels] = labels.as_json( -- cgit v1.2.1 From 9ed91479a7bbca1e420cd91a6322493d7ffda749 Mon Sep 17 00:00:00 2001 From: "micael.bergeron" Date: Mon, 20 Nov 2017 13:00:35 -0500 Subject: add the missing spec --- app/models/system_note_metadata.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/system_note_metadata.rb b/app/models/system_note_metadata.rb index ca0673080af..29035480371 100644 --- a/app/models/system_note_metadata.rb +++ b/app/models/system_note_metadata.rb @@ -5,8 +5,8 @@ class SystemNoteMetadata < ActiveRecord::Base # Other notes can always be safely shown as all its references are # in the same project (i.e. with the same permissions) TYPES_WITH_CROSS_REFERENCES = %w[ - cross_reference - milestone + commit cross_reference + close duplicate ].freeze ICON_TYPES = %w[ -- cgit v1.2.1 From f9565e303916ca194ef63b5fd3de541bf1c2a170 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Fri, 3 Nov 2017 14:16:43 +0100 Subject: Batchload blobs for diff generation After installing a new gem, batch-loader, a construct can be used to queue data to be fetched in bulk. The gem was also introduced in both gitlab-org/gitlab-ce!14680 and gitlab-org/gitlab-ce!14846, but those mrs are not merged yet. For the generation of diffs, both the old blob and the new blob need to be loaded. This for every file in the diff, too. Now we collect all these so we do 1 fetch. Three `.allow_n_plus_1_calls` have been removed, which I expect to be valid, but this needs to be confirmed by a full CI run. Possibly closes: - https://gitlab.com/gitlab-org/gitlab-ce/issues/37445 - https://gitlab.com/gitlab-org/gitlab-ce/issues/37599 - https://gitlab.com/gitlab-org/gitlab-ce/issues/37431 --- app/models/blob.rb | 17 ++++++++++++++++- app/models/commit.rb | 4 ++-- app/models/repository.rb | 5 +++++ 3 files changed, 23 insertions(+), 3 deletions(-) (limited to 'app/models') diff --git a/app/models/blob.rb b/app/models/blob.rb index ad0bc2e2ead..29e762724e3 100644 --- a/app/models/blob.rb +++ b/app/models/blob.rb @@ -76,12 +76,24 @@ class Blob < SimpleDelegator new(blob, project) end + def self.lazy(project, commit_id, path) + BatchLoader.for(commit_id: commit_id, path: path).batch do |items, loader| + project.repository.blobs_at(items.map(&:values)).each do |blob| + loader.call({ commit_id: blob.commit_id, path: blob.path }, blob) if blob + end + end + end + def initialize(blob, project = nil) @project = project super(blob) end + def inspect + "#<#{self.class.name} oid:#{id[0..8]} commit:#{commit_id[0..8]} path:#{path}>" + end + # Returns the data of the blob. # # If the blob is a text based blob the content is converted to UTF-8 and any @@ -95,7 +107,10 @@ class Blob < SimpleDelegator end def load_all_data! - super(project.repository) if project + # Endpoint needed: gitlab-org/gitaly#756 + Gitlab::GitalyClient.allow_n_plus_1_calls do + super(project.repository) if project + end end def no_highlighting? diff --git a/app/models/commit.rb b/app/models/commit.rb index a31ebe9cc87..8401d99a08f 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -84,7 +84,7 @@ class Commit end def id - @raw.id + raw.id end def ==(other) @@ -361,7 +361,7 @@ class Commit @deltas ||= raw.deltas end - def diffs(diff_options = nil) + def diffs(diff_options = {}) Gitlab::Diff::FileCollection::Commit.new(self, diff_options: diff_options) end diff --git a/app/models/repository.rb b/app/models/repository.rb index 8a6a8377de9..26d1bc12232 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -478,6 +478,11 @@ class Repository nil end + # items is an Array like: [[oid, path], [oid1, path1]] + def blobs_at(items) + raw_repository.batch_blobs(items).map { |blob| Blob.decorate(blob, project) } + end + def root_ref if raw_repository raw_repository.root_ref -- cgit v1.2.1 From c900c21eef9235306d7d0da42b07aa2de346e263 Mon Sep 17 00:00:00 2001 From: "micael.bergeron" Date: Tue, 21 Nov 2017 08:31:23 -0500 Subject: add `#with_metadata` scope to remove a N+1 from the notes' API --- app/models/note.rb | 1 + 1 file changed, 1 insertion(+) (limited to 'app/models') diff --git a/app/models/note.rb b/app/models/note.rb index 4bbb54ba9bf..50c9caf8529 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -110,6 +110,7 @@ class Note < ActiveRecord::Base includes(:author, :noteable, :updated_by, project: [:project_members, { group: [:group_members] }]) end + scope :with_metadata, -> { includes(:system_note_metadata) } after_initialize :ensure_discussion_id before_validation :nullify_blank_type, :nullify_blank_line_code -- cgit v1.2.1 From 0b9e1e16626eff4cd8ae43ce47ec0f965beaf843 Mon Sep 17 00:00:00 2001 From: Daniel Juarez Date: Tue, 21 Nov 2017 15:47:58 +0000 Subject: Skip confirmation user api --- app/models/user.rb | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'app/models') diff --git a/app/models/user.rb b/app/models/user.rb index 0329d094d09..f98165754ca 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -445,6 +445,10 @@ class User < ActiveRecord::Base skip_confirmation! if bool end + def skip_reconfirmation=(bool) + skip_reconfirmation! if bool + end + def generate_reset_token @reset_token, enc = Devise.token_generator.generate(self.class, :reset_password_token) -- cgit v1.2.1 From 54f1e406f40a31c6d790bdeabc55556a268990e1 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Tue, 21 Nov 2017 17:32:12 +0100 Subject: Use arrays in Pipeline#latest_builds_with_artifacts This changes Ci::Pipeline#latest_builds_with_artifacts so it returns an Array instead of a relation. Whenever we use this data we do so in two steps: 1. Count the number of rows 2. If this number is greater than 0, iterate over the rows By returning an Array instead we only execute 1 query of which the total time/work is less than running either just a COUNT(*) or both queries (in the worst case). On GitLab.com this change should save us a few milliseconds per request to ProjectsController#show. --- app/models/ci/pipeline.rb | 5 ++++- 1 file changed, 4 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index 3ded675bec0..ebbefc51a4f 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -507,7 +507,10 @@ module Ci end def latest_builds_with_artifacts - @latest_builds_with_artifacts ||= builds.latest.with_artifacts + # We purposely cast the builds to an Array here. Because we always use the + # rows if there are more than 0 this prevents us from having to run two + # queries: one to get the count and one to get the rows. + @latest_builds_with_artifacts ||= builds.latest.with_artifacts.to_a end private -- cgit v1.2.1 From 00cd5d93ce2245204356cf550871cfb96ea7dc8e Mon Sep 17 00:00:00 2001 From: "Jacob Vosmaer (GitLab)" Date: Tue, 21 Nov 2017 19:00:05 +0000 Subject: Use Redis cache for branch existence checks --- app/models/repository.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) (limited to 'app/models') diff --git a/app/models/repository.rb b/app/models/repository.rb index 26d1bc12232..2bf21cbdcc4 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -217,11 +217,7 @@ class Repository def branch_exists?(branch_name) return false unless raw_repository - @branch_exists_memo ||= Hash.new do |hash, key| - hash[key] = raw_repository.branch_exists?(key) - end - - @branch_exists_memo[branch_name] + branch_names.include?(branch_name) end def ref_exists?(ref) -- cgit v1.2.1 From 7df1cb528e5d977f5f18b1c82433b3c76220d6db Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Sat, 18 Nov 2017 02:13:45 +0800 Subject: Move identical merged branch check to merged_branch_names --- app/models/repository.rb | 10 ++-------- 1 file changed, 2 insertions(+), 8 deletions(-) (limited to 'app/models') diff --git a/app/models/repository.rb b/app/models/repository.rb index 2bf21cbdcc4..d4ffd7bfc07 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -909,19 +909,13 @@ class Repository end end - def merged_to_root_ref?(branch_or_name, pre_loaded_merged_branches = nil) + def merged_to_root_ref?(branch_or_name) branch = Gitlab::Git::Branch.find(self, branch_or_name) if branch @root_ref_sha ||= commit(root_ref).sha same_head = branch.target == @root_ref_sha - merged = - if pre_loaded_merged_branches - pre_loaded_merged_branches.include?(branch.name) - else - ancestor?(branch.target, @root_ref_sha) - end - + merged = ancestor?(branch.target, @root_ref_sha) !same_head && merged else nil -- cgit v1.2.1 From e826c5d0917a7fe2225fb6ba0862bc56c1ef3fc2 Mon Sep 17 00:00:00 2001 From: Jarka Kadlecova Date: Wed, 22 Nov 2017 14:20:35 +0100 Subject: Fix link text from group context --- app/models/commit.rb | 12 ++++++------ app/models/commit_range.rb | 8 ++++---- app/models/concerns/mentionable.rb | 4 ++-- app/models/concerns/referable.rb | 8 ++++---- app/models/external_issue.rb | 4 ++-- app/models/group.rb | 2 +- app/models/label.rb | 6 +++--- app/models/milestone.rb | 6 +++--- app/models/project.rb | 6 +++--- app/models/snippet.rb | 4 ++-- app/models/user.rb | 2 +- 11 files changed, 31 insertions(+), 31 deletions(-) (limited to 'app/models') diff --git a/app/models/commit.rb b/app/models/commit.rb index 8401d99a08f..6b28d290f99 100644 --- a/app/models/commit.rb +++ b/app/models/commit.rb @@ -109,12 +109,12 @@ class Commit @link_reference_pattern ||= super("commit", /(?#{COMMIT_SHA_PATTERN})/) end - def to_reference(from_project = nil, full: false) - commit_reference(from_project, id, full: full) + def to_reference(from = nil, full: false) + commit_reference(from, id, full: full) end - def reference_link_text(from_project = nil, full: false) - commit_reference(from_project, short_id, full: full) + def reference_link_text(from = nil, full: false) + commit_reference(from, short_id, full: full) end def diff_line_count @@ -381,8 +381,8 @@ class Commit private - def commit_reference(from_project, referable_commit_id, full: false) - reference = project.to_reference(from_project, full: full) + def commit_reference(from, referable_commit_id, full: false) + reference = project.to_reference(from, full: full) if reference.present? "#{reference}#{self.class.reference_prefix}#{referable_commit_id}" diff --git a/app/models/commit_range.rb b/app/models/commit_range.rb index 84e2e8a5dd5..b93c111dabc 100644 --- a/app/models/commit_range.rb +++ b/app/models/commit_range.rb @@ -89,8 +89,8 @@ class CommitRange alias_method :id, :to_s - def to_reference(from_project = nil, full: false) - project_reference = project.to_reference(from_project, full: full) + def to_reference(from = nil, full: false) + project_reference = project.to_reference(from, full: full) if project_reference.present? project_reference + self.class.reference_prefix + self.id @@ -99,8 +99,8 @@ class CommitRange end end - def reference_link_text(from_project = nil) - project_reference = project.to_reference(from_project) + def reference_link_text(from = nil) + project_reference = project.to_reference(from) reference = ref_from + notation + ref_to if project_reference.present? diff --git a/app/models/concerns/mentionable.rb b/app/models/concerns/mentionable.rb index 1db6b2d2fa2..b43eaeaeea0 100644 --- a/app/models/concerns/mentionable.rb +++ b/app/models/concerns/mentionable.rb @@ -31,11 +31,11 @@ module Mentionable # # By default this will be the class name and the result of calling # `to_reference` on the object. - def gfm_reference(from_project = nil) + def gfm_reference(from = nil) # "MergeRequest" > "merge_request" > "Merge request" > "merge request" friendly_name = self.class.to_s.underscore.humanize.downcase - "#{friendly_name} #{to_reference(from_project)}" + "#{friendly_name} #{to_reference(from)}" end # The GFM reference to this Mentionable, which shouldn't be included in its #references. diff --git a/app/models/concerns/referable.rb b/app/models/concerns/referable.rb index 78ac4f324e7..b782e85717e 100644 --- a/app/models/concerns/referable.rb +++ b/app/models/concerns/referable.rb @@ -7,7 +7,7 @@ module Referable # Returns the String necessary to reference this object in Markdown # - # from_project - Refering Project object + # from - Referring parent object # # This should be overridden by the including class. # @@ -17,12 +17,12 @@ module Referable # Issue.last.to_reference(other_project) # => "cross-project#1" # # Returns a String - def to_reference(_from_project = nil, full:) + def to_reference(_from = nil, full:) '' end - def reference_link_text(from_project = nil) - to_reference(from_project) + def reference_link_text(from = nil) + to_reference(from) end included do diff --git a/app/models/external_issue.rb b/app/models/external_issue.rb index 9ff56f229bc..2aaba2e4c90 100644 --- a/app/models/external_issue.rb +++ b/app/models/external_issue.rb @@ -38,11 +38,11 @@ class ExternalIssue @project.id end - def to_reference(_from_project = nil, full: nil) + def to_reference(_from = nil, full: nil) id end - def reference_link_text(from_project = nil) + def reference_link_text(from = nil) return "##{id}" if id =~ /^\d+$/ id diff --git a/app/models/group.rb b/app/models/group.rb index 8cf632fb566..dc4500360b9 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -97,7 +97,7 @@ class Group < Namespace end end - def to_reference(_from_project = nil, full: nil) + def to_reference(_from = nil, full: nil) "#{self.class.reference_prefix}#{full_path}" end diff --git a/app/models/label.rb b/app/models/label.rb index 899028a01a0..b5bfa6ea2dd 100644 --- a/app/models/label.rb +++ b/app/models/label.rb @@ -165,12 +165,12 @@ class Label < ActiveRecord::Base # # Returns a String # - def to_reference(from_project = nil, target_project: nil, format: :id, full: false) + def to_reference(from = nil, target_project: nil, format: :id, full: false) format_reference = label_format_reference(format) reference = "#{self.class.reference_prefix}#{format_reference}" - if from_project - "#{from_project.to_reference(target_project, full: full)}#{reference}" + if from + "#{from.to_reference(target_project, full: full)}#{reference}" else reference end diff --git a/app/models/milestone.rb b/app/models/milestone.rb index e01e52131f0..01458120cda 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -162,18 +162,18 @@ class Milestone < ActiveRecord::Base # Milestone.first.to_reference(cross_namespace_project) # => "gitlab-org/gitlab-ce%1" # Milestone.first.to_reference(same_namespace_project) # => "gitlab-ce%1" # - def to_reference(from_project = nil, format: :name, full: false) + def to_reference(from = nil, format: :name, full: false) format_reference = milestone_format_reference(format) reference = "#{self.class.reference_prefix}#{format_reference}" if project - "#{project.to_reference(from_project, full: full)}#{reference}" + "#{project.to_reference(from, full: full)}#{reference}" else reference end end - def reference_link_text(from_project = nil) + def reference_link_text(from = nil) self.title end diff --git a/app/models/project.rb b/app/models/project.rb index 894ded2a9f6..43a4d86c138 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -760,10 +760,10 @@ class Project < ActiveRecord::Base end end - def to_human_reference(from_project = nil) - if cross_namespace_reference?(from_project) + def to_human_reference(from = nil) + if cross_namespace_reference?(from) name_with_namespace - elsif cross_project_reference?(from_project) + elsif cross_project_reference?(from) name end end diff --git a/app/models/snippet.rb b/app/models/snippet.rb index 9533aa7f555..2a5f07a15c4 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -75,11 +75,11 @@ class Snippet < ActiveRecord::Base @link_reference_pattern ||= super("snippets", /(?\d+)/) end - def to_reference(from_project = nil, full: false) + def to_reference(from = nil, full: false) reference = "#{self.class.reference_prefix}#{id}" if project.present? - "#{project.to_reference(from_project, full: full)}#{reference}" + "#{project.to_reference(from, full: full)}#{reference}" else reference end diff --git a/app/models/user.rb b/app/models/user.rb index f98165754ca..01dfe1b4e51 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -437,7 +437,7 @@ class User < ActiveRecord::Base username end - def to_reference(_from_project = nil, target_project: nil, full: nil) + def to_reference(_from = nil, target_project: nil, full: nil) "#{self.class.reference_prefix}#{username}" end -- cgit v1.2.1 From 991bf24ec8890eca248a00deb4f33f309c9ffb83 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Wed, 15 Nov 2017 17:22:18 +0000 Subject: Use latest_merge_request_diff association Compared to the merge_request_diff association: 1. It's simpler to query. The query uses a foreign key to the merge_request_diffs table, so no ordering is necessary. 2. It's faster for preloading. The merge_request_diff association has to load every diff for the MRs in the set, then discard all but the most recent for each. This association means that Rails can just query for N diffs from N MRs. 3. It's more complicated to update. This is a bidirectional foreign key, so we need to update two tables when adding a diff record. This also means we need to handle this as a special case when importing a GitLab project. There is some juggling with this association in the merge request model: * `MergeRequest#latest_merge_request_diff` is _always_ the latest diff. * `MergeRequest#merge_request_diff` reuses `MergeRequest#latest_merge_request_diff` unless: * Arguments are passed. These are typically to force-reload the association. * It doesn't exist. That means we might be trying to implicitly create a diff. This only seems to happen in specs. * The association is already loaded. This is important for the reasons explained in the comment, which I'll reiterate here: if we a) load a non-latest diff, then b) get its `merge_request`, then c) get that MR's `merge_request_diff`, we should get the diff we loaded in c), even though that's not the latest diff. Basically, `MergeRequest#merge_request_diff` is the latest diff in most cases, but not quite all. --- app/models/ci/build.rb | 2 +- app/models/concerns/manual_inverse_association.rb | 17 +++++++++++ app/models/merge_request.rb | 37 +++++++++++++++++++++++ app/models/merge_request_diff.rb | 5 ++- 4 files changed, 59 insertions(+), 2 deletions(-) create mode 100644 app/models/concerns/manual_inverse_association.rb (limited to 'app/models') diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 1d9f367183e..1e9a3920667 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -243,7 +243,7 @@ module Ci @merge_request ||= begin - merge_requests = MergeRequest.includes(:merge_request_diff) + merge_requests = MergeRequest.includes(:latest_merge_request_diff) .where(source_branch: ref, source_project: pipeline.project) .reorder(iid: :desc) diff --git a/app/models/concerns/manual_inverse_association.rb b/app/models/concerns/manual_inverse_association.rb new file mode 100644 index 00000000000..0fca8feaf89 --- /dev/null +++ b/app/models/concerns/manual_inverse_association.rb @@ -0,0 +1,17 @@ +module ManualInverseAssociation + extend ActiveSupport::Concern + + module ClassMethods + def manual_inverse_association(association, inverse) + define_method(association) do |*args| + super(*args).tap do |value| + next unless value + + child_association = value.association(inverse) + child_association.set_inverse_instance(self) + child_association.target = self + end + end + end + end +end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index f1a5cc73e83..5cf41b2c7cb 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -5,6 +5,8 @@ class MergeRequest < ActiveRecord::Base include Referable include IgnorableColumn include TimeTrackable + include ManualInverseAssociation + include EachBatch ignore_column :locked_at, :ref_fetched @@ -14,9 +16,28 @@ class MergeRequest < ActiveRecord::Base belongs_to :merge_user, class_name: "User" has_many :merge_request_diffs + has_one :merge_request_diff, -> { order('merge_request_diffs.id DESC') }, inverse_of: :merge_request + belongs_to :latest_merge_request_diff, class_name: 'MergeRequestDiff' + manual_inverse_association :latest_merge_request_diff, :merge_request + + # This is the same as latest_merge_request_diff unless: + # 1. There are arguments - in which case we might be trying to force-reload. + # 2. This association is already loaded. + # 3. The latest diff does not exist. + # + # The second one in particular is important - MergeRequestDiff#merge_request + # is the inverse of MergeRequest#merge_request_diff, which means it may not be + # the latest diff, because we could have loaded any diff from this particular + # MR. If we haven't already loaded a diff, then it's fine to load the latest. + def merge_request_diff(*args) + fallback = latest_merge_request_diff if args.empty? && !association(:merge_request_diff).loaded? + + fallback || super + end + belongs_to :head_pipeline, foreign_key: "head_pipeline_id", class_name: "Ci::Pipeline" has_many :events, as: :target, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent @@ -167,6 +188,22 @@ class MergeRequest < ActiveRecord::Base where("merge_requests.id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection end + # This is used after project import, to reset the IDs to the correct + # values. It is not intended to be called without having already scoped the + # relation. + def self.set_latest_merge_request_diff_ids! + update = ' + latest_merge_request_diff_id = ( + SELECT MAX(id) + FROM merge_request_diffs + WHERE merge_requests.id = merge_request_diffs.merge_request_id + )'.squish + + self.each_batch do |batch| + batch.update_all(update) + end + end + WIP_REGEX = /\A\s*(\[WIP\]\s*|WIP:\s*|WIP\s+)+\s*/i.freeze def self.work_in_progress?(title) diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 5382f5cc627..9ee561b5161 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -2,6 +2,7 @@ class MergeRequestDiff < ActiveRecord::Base include Sortable include Importable include Gitlab::EncodingHelper + include ManualInverseAssociation # Prevent store of diff if commits amount more then 500 COMMITS_SAFE_SIZE = 100 @@ -10,6 +11,8 @@ class MergeRequestDiff < ActiveRecord::Base VALID_CLASSES = [Hash, Rugged::Patch, Rugged::Diff::Delta].freeze belongs_to :merge_request + manual_inverse_association :merge_request, :merge_request_diff + has_many :merge_request_diff_files, -> { order(:merge_request_diff_id, :relative_order) } has_many :merge_request_diff_commits, -> { order(:merge_request_diff_id, :relative_order) } @@ -194,7 +197,7 @@ class MergeRequestDiff < ActiveRecord::Base end def latest? - self == merge_request.merge_request_diff + self.id == merge_request.latest_merge_request_diff_id end def compare_with(sha) -- cgit v1.2.1 From 257fd5713485a05460a9170190100643199a7e48 Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Thu, 23 Nov 2017 13:16:14 +0000 Subject: Allow password authentication to be disabled entirely --- app/models/application_setting.rb | 11 ++++++++++- app/models/user.rb | 24 ++++++++++++++++++++---- 2 files changed, 30 insertions(+), 5 deletions(-) (limited to 'app/models') diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index a7e0219b03a..01455a52d2a 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -276,7 +276,8 @@ class ApplicationSetting < ActiveRecord::Base koding_url: nil, max_artifacts_size: Settings.artifacts['max_size'], max_attachment_size: Settings.gitlab['max_attachment_size'], - password_authentication_enabled: Settings.gitlab['password_authentication_enabled'], + password_authentication_enabled_for_web: Settings.gitlab['signin_enabled'], + password_authentication_enabled_for_git: true, performance_bar_allowed_group_id: nil, rsa_key_restriction: 0, plantuml_enabled: false, @@ -474,6 +475,14 @@ class ApplicationSetting < ActiveRecord::Base has_attribute?(attr_name) ? public_send(attr_name) : FORBIDDEN_KEY_VALUE # rubocop:disable GitlabSecurity/PublicSend end + def allow_signup? + signup_enabled? && password_authentication_enabled_for_web? + end + + def password_authentication_enabled? + password_authentication_enabled_for_web? || password_authentication_enabled_for_git? + end + private def ensure_uuid! diff --git a/app/models/user.rb b/app/models/user.rb index f98165754ca..6c773b3ce7d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -633,18 +633,34 @@ class User < ActiveRecord::Base count.zero? && Gitlab::ProtocolAccess.allowed?('ssh') end - def require_password_creation? - password_automatically_set? && allow_password_authentication? + def require_password_creation_for_web? + allow_password_authentication_for_web? && password_automatically_set? + end + + def require_password_creation_for_git? + allow_password_authentication_for_git? && password_automatically_set? end def require_personal_access_token_creation_for_git_auth? - return false if current_application_settings.password_authentication_enabled? || ldap_user? + return false if allow_password_authentication_for_git? || ldap_user? PersonalAccessTokensFinder.new(user: self, impersonation: false, state: 'active').execute.none? end + def require_extra_setup_for_git_auth? + require_password_creation_for_git? || require_personal_access_token_creation_for_git_auth? + end + def allow_password_authentication? - !ldap_user? && current_application_settings.password_authentication_enabled? + allow_password_authentication_for_web? || allow_password_authentication_for_git? + end + + def allow_password_authentication_for_web? + current_application_settings.password_authentication_enabled_for_web? && !ldap_user? + end + + def allow_password_authentication_for_git? + current_application_settings.password_authentication_enabled_for_git? && !ldap_user? end def can_change_username? -- cgit v1.2.1 From 0a4d55a1c9f58f383f23b8f60bbe1acba1b8511c Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Mon, 13 Nov 2017 15:15:42 +0100 Subject: Refactor Hashed Storage migration to add additional migration steps --- app/models/storage/hashed_project.rb | 1 - 1 file changed, 1 deletion(-) (limited to 'app/models') diff --git a/app/models/storage/hashed_project.rb b/app/models/storage/hashed_project.rb index f025f40994e..fae1b64961a 100644 --- a/app/models/storage/hashed_project.rb +++ b/app/models/storage/hashed_project.rb @@ -4,7 +4,6 @@ module Storage delegate :gitlab_shell, :repository_storage_path, to: :project ROOT_PATH_PREFIX = '@hashed'.freeze - STORAGE_VERSION = 1 def initialize(project) @project = project -- cgit v1.2.1 From d0a08ab888db33437c7df4eb37b5805757a6dce4 Mon Sep 17 00:00:00 2001 From: Gabriel Mazetto Date: Sat, 18 Nov 2017 07:26:07 +0100 Subject: Improve storage migration rake task --- app/models/project.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index 894ded2a9f6..f20c0688bc2 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -272,8 +272,9 @@ class Project < ActiveRecord::Base scope :pending_delete, -> { where(pending_delete: true) } scope :without_deleted, -> { where(pending_delete: false) } - scope :with_hashed_storage, -> { where('storage_version >= 1') } - scope :with_legacy_storage, -> { where(storage_version: [nil, 0]) } + scope :with_storage_feature, ->(feature) { where('storage_version >= :version', version: HASHED_STORAGE_FEATURES[feature]) } + scope :without_storage_feature, ->(feature) { where('storage_version < :version OR storage_version IS NULL', version: HASHED_STORAGE_FEATURES[feature]) } + scope :with_unmigrated_storage, -> { where('storage_version < :version OR storage_version IS NULL', version: LATEST_STORAGE_VERSION) } scope :sorted_by_activity, -> { reorder(last_activity_at: :desc) } scope :sorted_by_stars, -> { reorder('projects.star_count DESC') } -- cgit v1.2.1 From 0e6beaf50c9233ca03083691856dea2883f71773 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 15 Nov 2017 16:46:08 +0100 Subject: Clean up repository fetch and mirror methods --- app/models/repository.rb | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) (limited to 'app/models') diff --git a/app/models/repository.rb b/app/models/repository.rb index 2bf21cbdcc4..1b5eb8e2cea 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -972,6 +972,19 @@ class Repository run_git(args).first.lines.map(&:strip) end + def fetch_as_mirror(url, forced: false, fetch_refs: :all, remote_name: nil) + unless remote_name + remote_name = "tmp-#{SecureRandom.hex}" + tmp_remote_name = true + end + + add_remote(remote_name, url) + set_remote_as_mirror(remote_name, fetch_refs: fetch_refs) + fetch_remote(remote_name, forced: forced) + ensure + remove_remote(remote_name) if tmp_remote_name + end + def fetch_remote(remote, forced: false, ssh_auth: nil, no_tags: false) gitlab_shell.fetch_remote(raw_repository, remote, ssh_auth: ssh_auth, forced: forced, no_tags: no_tags) end @@ -1069,6 +1082,10 @@ class Repository raw_repository.fetch_ref(source_repository.raw_repository, source_ref: source_ref, target_ref: target_ref) end + def repository_storage_path + @project.repository_storage_path + end + private # TODO Generice finder, later split this on finders by Ref or Oid @@ -1134,10 +1151,6 @@ class Repository raw_repository.run_git_with_timeout(args, Gitlab::Git::Popen::FAST_GIT_PROCESS_TIMEOUT).first.strip end - def repository_storage_path - @project.repository_storage_path - end - def initialize_raw_repository Gitlab::Git::Repository.new(project.repository_storage, disk_path + '.git', Gitlab::GlRepository.gl_repository(project, is_wiki)) end -- cgit v1.2.1 From 7a1e93d35b7280db8bc4128862c86223d76a8d6d Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Thu, 23 Nov 2017 16:51:55 +0100 Subject: Rename fetch_refs to refmap --- app/models/repository.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/repository.rb b/app/models/repository.rb index 1b5eb8e2cea..c4784fd9235 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -972,14 +972,14 @@ class Repository run_git(args).first.lines.map(&:strip) end - def fetch_as_mirror(url, forced: false, fetch_refs: :all, remote_name: nil) + def fetch_as_mirror(url, forced: false, refmap: :all_refs, remote_name: nil) unless remote_name remote_name = "tmp-#{SecureRandom.hex}" tmp_remote_name = true end add_remote(remote_name, url) - set_remote_as_mirror(remote_name, fetch_refs: fetch_refs) + set_remote_as_mirror(remote_name, refmap: refmap) fetch_remote(remote_name, forced: forced) ensure remove_remote(remote_name) if tmp_remote_name -- cgit v1.2.1 From 5c2c471a835f9588e39ec11fa79571c8b44979f8 Mon Sep 17 00:00:00 2001 From: Felipe Artur Date: Tue, 21 Nov 2017 17:25:37 -0200 Subject: Fix WIP system note not being created --- app/models/concerns/issuable.rb | 7 +++++++ app/models/merge_request.rb | 6 ++++++ 2 files changed, 13 insertions(+) (limited to 'app/models') diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 35090181bd9..e607707475f 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -345,4 +345,11 @@ module Issuable def first_contribution? false end + + ## + # Overriden in MergeRequest + # + def wipless_title_changed(old_title) + old_title != title + end end diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index f1a5cc73e83..09b1697ab61 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -181,6 +181,12 @@ class MergeRequest < ActiveRecord::Base work_in_progress?(title) ? title : "WIP: #{title}" end + # Verifies if title has changed not taking into account WIP prefix + # for merge requests. + def wipless_title_changed(old_title) + self.class.wipless_title(old_title) != self.wipless_title + end + def hook_attrs Gitlab::HookData::MergeRequestBuilder.new(self).build end -- cgit v1.2.1 From ba62143ac34de6cf96da4a19b498b220f7e5154b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 21 Nov 2017 18:13:07 +0100 Subject: Refactor the way we pass `old associations` to issuable's update services MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/models/concerns/issuable.rb | 12 +++++++++--- 1 file changed, 9 insertions(+), 3 deletions(-) (limited to 'app/models') diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index e607707475f..27cd3118f81 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -255,8 +255,10 @@ module Issuable participants(user).include?(user) end - def to_hook_data(user, old_labels: [], old_assignees: [], old_total_time_spent: nil) + def to_hook_data(user, old_associations: {}) changes = previous_changes + old_labels = old_associations.fetch(:labels, []) + old_assignees = old_associations.fetch(:assignees, []) if old_labels != labels changes[:labels] = [old_labels.map(&:hook_attrs), labels.map(&:hook_attrs)] @@ -270,8 +272,12 @@ module Issuable end end - if old_total_time_spent != total_time_spent - changes[:total_time_spent] = [old_total_time_spent, total_time_spent] + if self.respond_to?(:total_time_spent) + old_total_time_spent = old_associations.fetch(:total_time_spent, nil) + + if old_total_time_spent != total_time_spent + changes[:total_time_spent] = [old_total_time_spent, total_time_spent] + end end Gitlab::HookData::IssuableBuilder.new(self).build(user: user, changes: changes) -- cgit v1.2.1 From d6dd9d712ac24a095d0b0506731f9415b7c3b5f5 Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Fri, 24 Nov 2017 12:41:36 +0000 Subject: Fix ProtectedBranch access level validations Before an access_level was required in EE even when an it had been set for a user/group. --- app/models/concerns/protected_branch_access.rb | 10 ---------- app/models/concerns/protected_ref_access.rb | 16 ++++++++++++++++ app/models/protected_tag/create_access_level.rb | 4 ---- 3 files changed, 16 insertions(+), 14 deletions(-) (limited to 'app/models') diff --git a/app/models/concerns/protected_branch_access.rb b/app/models/concerns/protected_branch_access.rb index fde1cc44afa..77307e92f22 100644 --- a/app/models/concerns/protected_branch_access.rb +++ b/app/models/concerns/protected_branch_access.rb @@ -1,12 +1,6 @@ module ProtectedBranchAccess extend ActiveSupport::Concern - ALLOWED_ACCESS_LEVELS ||= [ - Gitlab::Access::MASTER, - Gitlab::Access::DEVELOPER, - Gitlab::Access::NO_ACCESS - ].freeze - included do include ProtectedRefAccess @@ -14,10 +8,6 @@ module ProtectedBranchAccess delegate :project, to: :protected_branch - validates :access_level, presence: true, inclusion: { - in: ALLOWED_ACCESS_LEVELS - } - def self.human_access_levels { Gitlab::Access::MASTER => "Masters", diff --git a/app/models/concerns/protected_ref_access.rb b/app/models/concerns/protected_ref_access.rb index c4f158e569a..665c41c825e 100644 --- a/app/models/concerns/protected_ref_access.rb +++ b/app/models/concerns/protected_ref_access.rb @@ -1,15 +1,31 @@ module ProtectedRefAccess extend ActiveSupport::Concern + ALLOWED_ACCESS_LEVELS = [ + Gitlab::Access::MASTER, + Gitlab::Access::DEVELOPER, + Gitlab::Access::NO_ACCESS + ].freeze + included do scope :master, -> { where(access_level: Gitlab::Access::MASTER) } scope :developer, -> { where(access_level: Gitlab::Access::DEVELOPER) } + + validates :access_level, presence: true, if: :role?, inclusion: { + in: ALLOWED_ACCESS_LEVELS + } end def humanize self.class.human_access_levels[self.access_level] end + # CE access levels are always role-based, + # where as EE allows groups and users too + def role? + true + end + def check_access(user) return true if user.admin? diff --git a/app/models/protected_tag/create_access_level.rb b/app/models/protected_tag/create_access_level.rb index c7e1319719d..d1e81158351 100644 --- a/app/models/protected_tag/create_access_level.rb +++ b/app/models/protected_tag/create_access_level.rb @@ -1,10 +1,6 @@ class ProtectedTag::CreateAccessLevel < ActiveRecord::Base include ProtectedTagAccess - validates :access_level, presence: true, inclusion: { in: [Gitlab::Access::MASTER, - Gitlab::Access::DEVELOPER, - Gitlab::Access::NO_ACCESS] } - def self.human_access_levels { Gitlab::Access::MASTER => "Masters", -- cgit v1.2.1 From 96106287db2b6403809b238689afb592da5e2abf Mon Sep 17 00:00:00 2001 From: James Edwards-Jones Date: Fri, 24 Nov 2017 12:43:02 +0000 Subject: Deduplicate protected ref human_access_levels Previously these were duplicated so they could be different for push/merge, but this was no longer necessary after https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/11232 --- app/models/concerns/protected_branch_access.rb | 8 -------- app/models/concerns/protected_ref_access.rb | 8 +++++++- app/models/protected_tag/create_access_level.rb | 8 -------- 3 files changed, 7 insertions(+), 17 deletions(-) (limited to 'app/models') diff --git a/app/models/concerns/protected_branch_access.rb b/app/models/concerns/protected_branch_access.rb index 77307e92f22..e62f42e8e70 100644 --- a/app/models/concerns/protected_branch_access.rb +++ b/app/models/concerns/protected_branch_access.rb @@ -8,14 +8,6 @@ module ProtectedBranchAccess delegate :project, to: :protected_branch - def self.human_access_levels - { - Gitlab::Access::MASTER => "Masters", - Gitlab::Access::DEVELOPER => "Developers + Masters", - Gitlab::Access::NO_ACCESS => "No one" - }.with_indifferent_access - end - def check_access(user) return false if access_level == Gitlab::Access::NO_ACCESS diff --git a/app/models/concerns/protected_ref_access.rb b/app/models/concerns/protected_ref_access.rb index 665c41c825e..80c9f7d4eb4 100644 --- a/app/models/concerns/protected_ref_access.rb +++ b/app/models/concerns/protected_ref_access.rb @@ -7,6 +7,12 @@ module ProtectedRefAccess Gitlab::Access::NO_ACCESS ].freeze + HUMAN_ACCESS_LEVELS = { + Gitlab::Access::MASTER => "Masters".freeze, + Gitlab::Access::DEVELOPER => "Developers + Masters".freeze, + Gitlab::Access::NO_ACCESS => "No one".freeze + }.freeze + included do scope :master, -> { where(access_level: Gitlab::Access::MASTER) } scope :developer, -> { where(access_level: Gitlab::Access::DEVELOPER) } @@ -17,7 +23,7 @@ module ProtectedRefAccess end def humanize - self.class.human_access_levels[self.access_level] + HUMAN_ACCESS_LEVELS[self.access_level] end # CE access levels are always role-based, diff --git a/app/models/protected_tag/create_access_level.rb b/app/models/protected_tag/create_access_level.rb index d1e81158351..6b6ab3d8279 100644 --- a/app/models/protected_tag/create_access_level.rb +++ b/app/models/protected_tag/create_access_level.rb @@ -1,14 +1,6 @@ class ProtectedTag::CreateAccessLevel < ActiveRecord::Base include ProtectedTagAccess - def self.human_access_levels - { - Gitlab::Access::MASTER => "Masters", - Gitlab::Access::DEVELOPER => "Developers + Masters", - Gitlab::Access::NO_ACCESS => "No one" - }.with_indifferent_access - end - def check_access(user) return false if access_level == Gitlab::Access::NO_ACCESS -- cgit v1.2.1 From 8041a87288906e4b10b86a9a2ab9039036243a5d Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 24 Nov 2017 11:23:14 +0100 Subject: Drastically improve project search performance by no longer searching namespace name --- app/models/project.rb | 30 ++++++++---------------------- 1 file changed, 8 insertions(+), 22 deletions(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index 43a4d86c138..e276bd2422d 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -18,6 +18,7 @@ class Project < ActiveRecord::Base include SelectForProjectAuthorization include Routable include GroupDescendant + include Gitlab::SQL::Pattern extend Gitlab::ConfigHelper extend Gitlab::CurrentSettings @@ -424,32 +425,17 @@ class Project < ActiveRecord::Base # # query - The search query as a String. def search(query) - ptable = arel_table - ntable = Namespace.arel_table - pattern = "%#{query}%" - - # unscoping unnecessary conditions that'll be applied - # when executing `where("projects.id IN (#{union.to_sql})")` - projects = unscoped.select(:id).where( - ptable[:path].matches(pattern) - .or(ptable[:name].matches(pattern)) - .or(ptable[:description].matches(pattern)) - ) - - namespaces = unscoped.select(:id) - .joins(:namespace) - .where(ntable[:name].matches(pattern)) - - union = Gitlab::SQL::Union.new([projects, namespaces]) + pattern = to_pattern(query) - where("projects.id IN (#{union.to_sql})") # rubocop:disable GitlabSecurity/SqlInjection + where( + arel_table[:path].matches(pattern) + .or(arel_table[:name].matches(pattern)) + .or(arel_table[:description].matches(pattern)) + ) end def search_by_title(query) - pattern = "%#{query}%" - table = Project.arel_table - - non_archived.where(table[:name].matches(pattern)) + non_archived.where(arel_table[:name].matches(to_pattern(query))) end def visibility_levels -- cgit v1.2.1 From aedd2cfa5b82c01f82ec26b64880fce2a07fe942 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 24 Nov 2017 11:45:19 +0100 Subject: Use Gitlab::SQL::Pattern where appropriate --- app/models/ci/runner.rb | 3 ++- app/models/group.rb | 14 -------------- app/models/milestone.rb | 3 ++- app/models/namespace.rb | 3 ++- app/models/note.rb | 5 +++++ app/models/snippet.rb | 8 +++----- app/models/user.rb | 2 +- 7 files changed, 15 insertions(+), 23 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index c6509f89117..d91a66ab5c2 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -1,6 +1,7 @@ module Ci class Runner < ActiveRecord::Base extend Gitlab::Ci::Model + include Gitlab::SQL::Pattern RUNNER_QUEUE_EXPIRY_TIME = 60.minutes ONLINE_CONTACT_TIMEOUT = 1.hour @@ -60,7 +61,7 @@ module Ci # Returns an ActiveRecord::Relation. def self.search(query) t = arel_table - pattern = "%#{query}%" + pattern = to_pattern(query) where(t[:token].matches(pattern).or(t[:description].matches(pattern))) end diff --git a/app/models/group.rb b/app/models/group.rb index dc4500360b9..76262acf50c 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -50,20 +50,6 @@ class Group < Namespace Gitlab::Database.postgresql? end - # Searches for groups matching the given query. - # - # This method uses ILIKE on PostgreSQL and LIKE on MySQL. - # - # query - The search query as a String - # - # Returns an ActiveRecord::Relation. - def search(query) - table = Namespace.arel_table - pattern = "%#{query}%" - - where(table[:name].matches(pattern).or(table[:path].matches(pattern))) - end - def sort(method) if method == 'storage_size_desc' # storage_size is a virtual column so we need to diff --git a/app/models/milestone.rb b/app/models/milestone.rb index 01458120cda..e25d72cf947 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -13,6 +13,7 @@ class Milestone < ActiveRecord::Base include Referable include StripAttribute include Milestoneish + include Gitlab::SQL::Pattern cache_markdown_field :title, pipeline: :single_line cache_markdown_field :description @@ -74,7 +75,7 @@ class Milestone < ActiveRecord::Base # Returns an ActiveRecord::Relation. def search(query) t = arel_table - pattern = "%#{query}%" + pattern = to_pattern(query) where(t[:title].matches(pattern).or(t[:description].matches(pattern))) end diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 4d401e7ba18..15bc7032a43 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -9,6 +9,7 @@ class Namespace < ActiveRecord::Base include Routable include AfterCommitQueue include Storage::LegacyNamespace + include Gitlab::SQL::Pattern # Prevent users from creating unreasonably deep level of nesting. # The number 20 was taken based on maximum nesting level of @@ -87,7 +88,7 @@ class Namespace < ActiveRecord::Base # Returns an ActiveRecord::Relation def search(query) t = arel_table - pattern = "%#{query}%" + pattern = to_pattern(query) where(t[:name].matches(pattern).or(t[:path].matches(pattern))) end diff --git a/app/models/note.rb b/app/models/note.rb index 50c9caf8529..d2aa8392229 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -14,6 +14,7 @@ class Note < ActiveRecord::Base include ResolvableNote include IgnorableColumn include Editable + include Gitlab::SQL::Pattern module SpecialRole FIRST_TIME_CONTRIBUTOR = :first_time_contributor @@ -167,6 +168,10 @@ class Note < ActiveRecord::Base def has_special_role?(role, note) note.special_role == role end + + def search(query) + where(arel_table[:note].matches(to_pattern(query))) + end end def cross_reference? diff --git a/app/models/snippet.rb b/app/models/snippet.rb index 2a5f07a15c4..e621404f3ae 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -9,6 +9,7 @@ class Snippet < ActiveRecord::Base include Mentionable include Spammable include Editable + include Gitlab::SQL::Pattern extend Gitlab::CurrentSettings @@ -136,7 +137,7 @@ class Snippet < ActiveRecord::Base # Returns an ActiveRecord::Relation. def search(query) t = arel_table - pattern = "%#{query}%" + pattern = to_pattern(query) where(t[:title].matches(pattern).or(t[:file_name].matches(pattern))) end @@ -149,10 +150,7 @@ class Snippet < ActiveRecord::Base # # Returns an ActiveRecord::Relation. def search_code(query) - table = Snippet.arel_table - pattern = "%#{query}%" - - where(table[:content].matches(pattern)) + where(arel_table[:content].matches(to_pattern(query))) end end end diff --git a/app/models/user.rb b/app/models/user.rb index cf6b36559a8..9a35336c574 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -339,7 +339,7 @@ class User < ActiveRecord::Base def search_with_secondary_emails(query) table = arel_table email_table = Email.arel_table - pattern = "%#{query}%" + pattern = to_pattern(query) matched_by_emails_user_ids = email_table.project(email_table[:user_id]).where(email_table[:email].matches(pattern)) where( -- cgit v1.2.1 From b2c5363da1bdfb4df8693de38f9d83fe203e6e99 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 24 Nov 2017 12:08:16 +0100 Subject: Rename to_fuzzy_arel to fuzzy_arel_match --- app/models/concerns/issuable.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'app/models') diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index e607707475f..176ce1152f1 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -122,7 +122,7 @@ module Issuable # # Returns an ActiveRecord::Relation. def search(query) - title = to_fuzzy_arel(:title, query) + title = fuzzy_arel_match(:title, query) where(title) end @@ -135,8 +135,8 @@ module Issuable # # Returns an ActiveRecord::Relation. def full_search(query) - title = to_fuzzy_arel(:title, query) - description = to_fuzzy_arel(:description, query) + title = fuzzy_arel_match(:title, query) + description = fuzzy_arel_match(:description, query) where(title&.or(description)) end -- cgit v1.2.1 From 7fb1bb01bd669cc46514ed17b1b8822a1d962970 Mon Sep 17 00:00:00 2001 From: George Andrinopoulos Date: Sat, 25 Nov 2017 17:04:45 +0200 Subject: Create issue and merge request destroy services --- app/models/issue.rb | 1 - app/models/merge_request.rb | 1 - 2 files changed, 2 deletions(-) (limited to 'app/models') diff --git a/app/models/issue.rb b/app/models/issue.rb index a9863a50d84..d6ef58d150b 100644 --- a/app/models/issue.rb +++ b/app/models/issue.rb @@ -49,7 +49,6 @@ class Issue < ActiveRecord::Base scope :public_only, -> { where(confidential: false) } after_save :expire_etag_cache - after_commit :update_project_counter_caches, on: :destroy attr_spammable :title, spam_title: true attr_spammable :description, spam_description: true diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 2753e4b16e5..e4d8f486c77 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -52,7 +52,6 @@ class MergeRequest < ActiveRecord::Base after_create :ensure_merge_request_diff, unless: :importing? after_update :reload_diff_if_branch_changed - after_commit :update_project_counter_caches, on: :destroy # When this attribute is true some MR validation is ignored # It allows us to close or modify broken merge requests -- cgit v1.2.1 From da42dfb3cf4a2fb0cdcc1a3b41438516a0bed0e5 Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Fri, 24 Nov 2017 12:24:24 +0100 Subject: Use fuzzy search with minimum length of 3 characters where appropriate --- app/models/ci/runner.rb | 5 +---- app/models/concerns/issuable.rb | 9 ++------- app/models/email.rb | 1 + app/models/milestone.rb | 5 +---- app/models/namespace.rb | 5 +---- app/models/note.rb | 2 +- app/models/project.rb | 10 ++-------- app/models/snippet.rb | 7 ++----- app/models/user.rb | 24 +++++++++--------------- 9 files changed, 20 insertions(+), 48 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index d91a66ab5c2..d39610a8995 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -60,10 +60,7 @@ module Ci # # Returns an ActiveRecord::Relation. def self.search(query) - t = arel_table - pattern = to_pattern(query) - - where(t[:token].matches(pattern).or(t[:description].matches(pattern))) + fuzzy_search(query, [:token, :description]) end def self.contact_time_deadline diff --git a/app/models/concerns/issuable.rb b/app/models/concerns/issuable.rb index 176ce1152f1..81706a5fc4b 100644 --- a/app/models/concerns/issuable.rb +++ b/app/models/concerns/issuable.rb @@ -122,9 +122,7 @@ module Issuable # # Returns an ActiveRecord::Relation. def search(query) - title = fuzzy_arel_match(:title, query) - - where(title) + fuzzy_search(query, [:title]) end # Searches for records with a matching title or description. @@ -135,10 +133,7 @@ module Issuable # # Returns an ActiveRecord::Relation. def full_search(query) - title = fuzzy_arel_match(:title, query) - description = fuzzy_arel_match(:description, query) - - where(title&.or(description)) + fuzzy_search(query, [:title, :description]) end def sort(method, excluded_labels: []) diff --git a/app/models/email.rb b/app/models/email.rb index 2da8b050149..d6516761f0a 100644 --- a/app/models/email.rb +++ b/app/models/email.rb @@ -1,5 +1,6 @@ class Email < ActiveRecord::Base include Sortable + include Gitlab::SQL::Pattern belongs_to :user diff --git a/app/models/milestone.rb b/app/models/milestone.rb index e25d72cf947..c06ee8083f0 100644 --- a/app/models/milestone.rb +++ b/app/models/milestone.rb @@ -74,10 +74,7 @@ class Milestone < ActiveRecord::Base # # Returns an ActiveRecord::Relation. def search(query) - t = arel_table - pattern = to_pattern(query) - - where(t[:title].matches(pattern).or(t[:description].matches(pattern))) + fuzzy_search(query, [:title, :description]) end def filter_by_state(milestones, state) diff --git a/app/models/namespace.rb b/app/models/namespace.rb index 15bc7032a43..fa76729a702 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -87,10 +87,7 @@ class Namespace < ActiveRecord::Base # # Returns an ActiveRecord::Relation def search(query) - t = arel_table - pattern = to_pattern(query) - - where(t[:name].matches(pattern).or(t[:path].matches(pattern))) + fuzzy_search(query, [:name, :path]) end def clean_path(path) diff --git a/app/models/note.rb b/app/models/note.rb index d2aa8392229..340fe087f82 100644 --- a/app/models/note.rb +++ b/app/models/note.rb @@ -170,7 +170,7 @@ class Note < ActiveRecord::Base end def search(query) - where(arel_table[:note].matches(to_pattern(query))) + fuzzy_search(query, [:note]) end end diff --git a/app/models/project.rb b/app/models/project.rb index e276bd2422d..f0068a7e758 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -425,17 +425,11 @@ class Project < ActiveRecord::Base # # query - The search query as a String. def search(query) - pattern = to_pattern(query) - - where( - arel_table[:path].matches(pattern) - .or(arel_table[:name].matches(pattern)) - .or(arel_table[:description].matches(pattern)) - ) + fuzzy_search(query, [:path, :name, :description]) end def search_by_title(query) - non_archived.where(arel_table[:name].matches(to_pattern(query))) + non_archived.fuzzy_search(query, [:name]) end def visibility_levels diff --git a/app/models/snippet.rb b/app/models/snippet.rb index e621404f3ae..05a16f11b59 100644 --- a/app/models/snippet.rb +++ b/app/models/snippet.rb @@ -136,10 +136,7 @@ class Snippet < ActiveRecord::Base # # Returns an ActiveRecord::Relation. def search(query) - t = arel_table - pattern = to_pattern(query) - - where(t[:title].matches(pattern).or(t[:file_name].matches(pattern))) + fuzzy_search(query, [:title, :file_name]) end # Searches for snippets with matching content. @@ -150,7 +147,7 @@ class Snippet < ActiveRecord::Base # # Returns an ActiveRecord::Relation. def search_code(query) - where(arel_table[:content].matches(to_pattern(query))) + fuzzy_search(query, [:content]) end end end diff --git a/app/models/user.rb b/app/models/user.rb index 9a35336c574..14941fd7f98 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -313,9 +313,6 @@ class User < ActiveRecord::Base # # Returns an ActiveRecord::Relation. def search(query) - table = arel_table - pattern = User.to_pattern(query) - order = <<~SQL CASE WHEN users.name = %{query} THEN 0 @@ -325,11 +322,8 @@ class User < ActiveRecord::Base END SQL - where( - table[:name].matches(pattern) - .or(table[:email].matches(pattern)) - .or(table[:username].matches(pattern)) - ).reorder(order % { query: ActiveRecord::Base.connection.quote(query) }, :name) + fuzzy_search(query, [:name, :email, :username]) + .reorder(order % { query: ActiveRecord::Base.connection.quote(query) }, :name) end # searches user by given pattern @@ -337,16 +331,16 @@ class User < ActiveRecord::Base # This method uses ILIKE on PostgreSQL and LIKE on MySQL. def search_with_secondary_emails(query) - table = arel_table email_table = Email.arel_table - pattern = to_pattern(query) - matched_by_emails_user_ids = email_table.project(email_table[:user_id]).where(email_table[:email].matches(pattern)) + matched_by_emails_user_ids = email_table + .project(email_table[:user_id]) + .where(Email.fuzzy_arel_match(:email, query)) where( - table[:name].matches(pattern) - .or(table[:email].matches(pattern)) - .or(table[:username].matches(pattern)) - .or(table[:id].in(matched_by_emails_user_ids)) + fuzzy_arel_match(:name, query) + .or(fuzzy_arel_match(:email, query)) + .or(fuzzy_arel_match(:username, query)) + .or(arel_table[:id].in(matched_by_emails_user_ids)) ) end -- cgit v1.2.1 From 07c7ba1bf4a1d0092d07a23e23caf698512d46e0 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Fri, 24 Nov 2017 16:30:08 +0100 Subject: Allow to drop jobs for deleted projects --- app/models/ci/build.rb | 1 + app/models/commit_status.rb | 16 ++++++++++------ 2 files changed, 11 insertions(+), 6 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 1e9a3920667..4ea040dfad5 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -104,6 +104,7 @@ module Ci end before_transition any => [:failed] do |build| + next unless build.project next if build.retries_max.zero? if build.retries_count < build.retries_max diff --git a/app/models/commit_status.rb b/app/models/commit_status.rb index 6b07dbdf3ea..ee21ed8e420 100644 --- a/app/models/commit_status.rb +++ b/app/models/commit_status.rb @@ -17,6 +17,7 @@ class CommitStatus < ActiveRecord::Base validates :name, presence: true, unless: :importing? alias_attribute :author, :user + alias_attribute :pipeline_id, :commit_id scope :failed_but_allowed, -> do where(allow_failure: true, status: [:failed, :canceled]) @@ -103,26 +104,29 @@ class CommitStatus < ActiveRecord::Base end after_transition do |commit_status, transition| + next unless commit_status.project next if transition.loopback? commit_status.run_after_commit do - if pipeline + if pipeline_id if complete? || manual? - PipelineProcessWorker.perform_async(pipeline.id) + PipelineProcessWorker.perform_async(pipeline_id) else - PipelineUpdateWorker.perform_async(pipeline.id) + PipelineUpdateWorker.perform_async(pipeline_id) end end - StageUpdateWorker.perform_async(commit_status.stage_id) - ExpireJobCacheWorker.perform_async(commit_status.id) + StageUpdateWorker.perform_async(stage_id) + ExpireJobCacheWorker.perform_async(id) end end after_transition any => :failed do |commit_status| + next unless commit_status.project + commit_status.run_after_commit do MergeRequests::AddTodoWhenBuildFailsService - .new(pipeline.project, nil).execute(self) + .new(project, nil).execute(self) end end end -- cgit v1.2.1 From fa35ea13873f81405c5203f0827ea15703615c97 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Mon, 27 Nov 2017 16:58:12 +0100 Subject: Strip leading & trailing whitespaces in CI/CD secret variable keys MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/models/concerns/has_variable.rb | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'app/models') diff --git a/app/models/concerns/has_variable.rb b/app/models/concerns/has_variable.rb index 9585b5583dc..8a241e4374a 100644 --- a/app/models/concerns/has_variable.rb +++ b/app/models/concerns/has_variable.rb @@ -16,6 +16,10 @@ module HasVariable key: Gitlab::Application.secrets.db_key_base, algorithm: 'aes-256-cbc' + def key=(new_key) + super(new_key.to_s.strip) + end + def to_runner_variable { key: key, value: value, public: false } end -- cgit v1.2.1 From 8a55d2c5526ee5cb93bb40075dbbeaa6fcb01f91 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 21 Nov 2017 21:09:55 +0900 Subject: Revert KubernetesService logic in Platforms::Kubernetes --- app/models/clusters/platforms/kubernetes.rb | 123 ++++++++++++++++++++++++++++ 1 file changed, 123 insertions(+) (limited to 'app/models') diff --git a/app/models/clusters/platforms/kubernetes.rb b/app/models/clusters/platforms/kubernetes.rb index 6dc1ee810d3..a1e686d0d12 100644 --- a/app/models/clusters/platforms/kubernetes.rb +++ b/app/models/clusters/platforms/kubernetes.rb @@ -1,7 +1,12 @@ module Clusters module Platforms class Kubernetes < ActiveRecord::Base + include Gitlab::CurrentSettings + include Gitlab::Kubernetes + include ReactiveCaching + self.table_name = 'cluster_platforms_kubernetes' + self.reactive_cache_key = ->(kubernetes) { [kubernetes.class.model_name.singular, kubernetes.cluster_id] } belongs_to :cluster, inverse_of: :platform_kubernetes, class_name: 'Clusters::Cluster' @@ -29,6 +34,8 @@ module Clusters validates :api_url, url: true, presence: true validates :token, presence: true + after_save :clear_reactive_cache! + # TODO: Glue code till we migrate Kubernetes Integration into Platforms::Kubernetes after_destroy :destroy_kubernetes_integration! @@ -55,6 +62,80 @@ module Clusters self.class.namespace_for_project(project) if project end + def predefined_variables + config = YAML.dump(kubeconfig) + + variables = [ + { key: 'KUBE_URL', value: api_url, public: true }, + { key: 'KUBE_TOKEN', value: token, public: false }, + { key: 'KUBE_NAMESPACE', value: actual_namespace, public: true }, + { key: 'KUBECONFIG', value: config, public: false, file: true } + ] + + if ca_pem.present? + variables << { key: 'KUBE_CA_PEM', value: ca_pem, public: true } + variables << { key: 'KUBE_CA_PEM_FILE', value: ca_pem, public: true, file: true } + end + + variables + end + + # Constructs a list of terminals from the reactive cache + # + # Returns nil if the cache is empty, in which case you should try again a + # short time later + def terminals(environment) + with_reactive_cache do |data| + pods = filter_by_label(data[:pods], app: environment.slug) + terminals = pods.flat_map { |pod| terminals_for_pod(api_url, actual_namespace, pod) } + terminals.each { |terminal| add_terminal_auth(terminal, terminal_auth) } + end + end + + # Caches resources in the namespace so other calls don't need to block on + # network access + def calculate_reactive_cache + return unless active? && project && !project.pending_delete? + + # We may want to cache extra things in the future + { pods: read_pods } + end + + def kubeconfig + to_kubeconfig( + url: api_url, + namespace: actual_namespace, + token: token, + ca_pem: ca_pem) + end + + def read_secrets + kubeclient = build_kubeclient! + + kubeclient.get_secrets.as_json + end + + # Returns a hash of all pods in the namespace + def read_pods + kubeclient = build_kubeclient! + + kubeclient.get_pods(namespace: actual_namespace).as_json + rescue KubeException => err + raise err unless err.error_code == 404 + [] + end + + def kubeclient_ssl_options + opts = { verify_ssl: OpenSSL::SSL::VERIFY_PEER } + + if ca_pem.present? + opts[:cert_store] = OpenSSL::X509::Store.new + opts[:cert_store].add_cert(OpenSSL::X509::Certificate.new(ca_pem)) + end + + opts + end + def kubeclient @kubeclient ||= kubernetes_service.kubeclient if manages_kubernetes_service? end @@ -104,6 +185,48 @@ module Clusters def ensure_kubernetes_service @kubernetes_service ||= kubernetes_service || project&.build_kubernetes_service end + + def build_kubeclient!(api_path: 'api', api_version: 'v1') + raise "Incomplete settings" unless api_url && actual_namespace + + unless (username && password) || token + raise "Either username/password or token is required to access API" + end + + ::Kubeclient::Client.new( + join_api_url(api_path), + api_version, + auth_options: kubeclient_auth_options, + ssl_options: kubeclient_ssl_options, + http_proxy_uri: ENV['http_proxy'] + ) + end + + def kubeclient_auth_options + return { username: username, password: password } if username && password + return { bearer_token: token } if token + end + + def join_api_url(api_path) + url = URI.parse(api_url) + prefix = url.path.sub(%r{/+\z}, '') + + url.path = [prefix, api_path].join("/") + + url.to_s + end + + def terminal_auth + { + token: token, + ca_pem: ca_pem, + max_session_time: current_application_settings.terminal_max_session_time + } + end + + def enforce_namespace_to_lower_case + self.namespace = self.namespace&.downcase + end end end end -- cgit v1.2.1 From 0d95ce51be77d2a2a3e8421c56da6a8e5762a44f Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 21 Nov 2017 22:03:07 +0900 Subject: Check diff between KubernetesService and Platforms::Kubernetes. Synchronize again. --- app/models/clusters/platforms/kubernetes.rb | 124 ++++++++++------------ app/models/project_services/kubernetes_service.rb | 5 + 2 files changed, 62 insertions(+), 67 deletions(-) (limited to 'app/models') diff --git a/app/models/clusters/platforms/kubernetes.rb b/app/models/clusters/platforms/kubernetes.rb index a1e686d0d12..549b32ceef1 100644 --- a/app/models/clusters/platforms/kubernetes.rb +++ b/app/models/clusters/platforms/kubernetes.rb @@ -44,12 +44,6 @@ module Clusters delegate :project, to: :cluster, allow_nil: true delegate :enabled?, to: :cluster, allow_nil: true - class << self - def namespace_for_project(project) - "#{project.path}-#{project.id}" - end - end - def actual_namespace if namespace.present? namespace @@ -58,10 +52,6 @@ module Clusters end end - def default_namespace - self.class.namespace_for_project(project) if project - end - def predefined_variables config = YAML.dump(kubeconfig) @@ -101,41 +91,6 @@ module Clusters { pods: read_pods } end - def kubeconfig - to_kubeconfig( - url: api_url, - namespace: actual_namespace, - token: token, - ca_pem: ca_pem) - end - - def read_secrets - kubeclient = build_kubeclient! - - kubeclient.get_secrets.as_json - end - - # Returns a hash of all pods in the namespace - def read_pods - kubeclient = build_kubeclient! - - kubeclient.get_pods(namespace: actual_namespace).as_json - rescue KubeException => err - raise err unless err.error_code == 404 - [] - end - - def kubeclient_ssl_options - opts = { verify_ssl: OpenSSL::SSL::VERIFY_PEER } - - if ca_pem.present? - opts[:cert_store] = OpenSSL::X509::Store.new - opts[:cert_store].add_cert(OpenSSL::X509::Certificate.new(ca_pem)) - end - - opts - end - def kubeclient @kubeclient ||= kubernetes_service.kubeclient if manages_kubernetes_service? end @@ -161,29 +116,19 @@ module Clusters private - def enforce_namespace_to_lower_case - self.namespace = self.namespace&.downcase - end - - # TODO: glue code till we migrate Kubernetes Service into Platforms::Kubernetes class - def manages_kubernetes_service? - return true unless kubernetes_service&.active? - - kubernetes_service.api_url == api_url - end - - def destroy_kubernetes_integration! - return unless manages_kubernetes_service? - - kubernetes_service&.destroy! + def kubeconfig + to_kubeconfig( + url: api_url, + namespace: actual_namespace, + token: token, + ca_pem: ca_pem) end - def kubernetes_service - @kubernetes_service ||= project&.kubernetes_service - end + def default_namespace + return unless project - def ensure_kubernetes_service - @kubernetes_service ||= kubernetes_service || project&.build_kubernetes_service + slug = "#{project.path}-#{project.id}".downcase + slug.gsub(/[^-a-z0-9]/, '-').gsub(/^-+/, '') end def build_kubeclient!(api_path: 'api', api_version: 'v1') @@ -202,9 +147,29 @@ module Clusters ) end + # Returns a hash of all pods in the namespace + def read_pods + kubeclient = build_kubeclient! + + kubeclient.get_pods(namespace: actual_namespace).as_json + rescue KubeException => err + raise err unless err.error_code == 404 + [] + end + + def kubeclient_ssl_options + opts = { verify_ssl: OpenSSL::SSL::VERIFY_PEER } + + if ca_pem.present? + opts[:cert_store] = OpenSSL::X509::Store.new + opts[:cert_store].add_cert(OpenSSL::X509::Certificate.new(ca_pem)) + end + + opts + end + def kubeclient_auth_options - return { username: username, password: password } if username && password - return { bearer_token: token } if token + { bearer_token: token } end def join_api_url(api_path) @@ -227,6 +192,31 @@ module Clusters def enforce_namespace_to_lower_case self.namespace = self.namespace&.downcase end + + def enforce_namespace_to_lower_case + self.namespace = self.namespace&.downcase + end + + # TODO: glue code till we migrate Kubernetes Service into Platforms::Kubernetes class + def manages_kubernetes_service? + return true unless kubernetes_service&.active? + + kubernetes_service.api_url == api_url + end + + def destroy_kubernetes_integration! + return unless manages_kubernetes_service? + + kubernetes_service&.destroy! + end + + def kubernetes_service + @kubernetes_service ||= project&.kubernetes_service + end + + def ensure_kubernetes_service + @kubernetes_service ||= kubernetes_service || project&.build_kubernetes_service + end end end end diff --git a/app/models/project_services/kubernetes_service.rb b/app/models/project_services/kubernetes_service.rb index bc62972dbb0..b82567ce2b3 100644 --- a/app/models/project_services/kubernetes_service.rb +++ b/app/models/project_services/kubernetes_service.rb @@ -1,3 +1,8 @@ +## +# NOTE: +# We'll move this class to Clusters::Platforms::Kubernetes, which contains exactly the same logic. +# After we've migrated data, we'll remove KubernetesService. This would happen in a few months. +# If you're modyfiyng this class, please note that you should update the same change in Clusters::Platforms::Kubernetes. class KubernetesService < DeploymentService include Gitlab::CurrentSettings include Gitlab::Kubernetes -- cgit v1.2.1 From cff5eadd7cca3537db4566126714a376c8e21e9e Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 21 Nov 2017 22:52:05 +0900 Subject: Add deployment platform selector --- app/models/project.rb | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index e276bd2422d..1b201b13548 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -123,7 +123,7 @@ class Project < ActiveRecord::Base has_one :bugzilla_service has_one :gitlab_issue_tracker_service, inverse_of: :project has_one :external_wiki_service - has_one :kubernetes_service, inverse_of: :project + # has_one :kubernetes_service, inverse_of: :project has_one :prometheus_service, inverse_of: :project has_one :mock_ci_service has_one :mock_deployment_service @@ -907,7 +907,17 @@ class Project < ActiveRecord::Base end def deployment_service - @deployment_service ||= deployment_services.reorder(nil).find_by(active: true) + deployment_platform + end + + def kubernetes_service + deployment_platform + end + + # TODO: This will be extended for multiple enviroment clusters + def deployment_platform + @deployment_platform ||= clusters.where(enabled: true).first&.platform_kubernetes + @deployment_platform ||= deployment_services.reorder(nil).find_by(active: true) end def monitoring_services -- cgit v1.2.1 From 3cf53cc611930f875995883e3051015ed9bcb49b Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 21 Nov 2017 22:53:36 +0900 Subject: Fix comments --- app/models/project.rb | 1 + 1 file changed, 1 insertion(+) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index 1b201b13548..512fe77261a 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -915,6 +915,7 @@ class Project < ActiveRecord::Base end # TODO: This will be extended for multiple enviroment clusters + # TODO: Add super nice tests to check this interchangeability def deployment_platform @deployment_platform ||= clusters.where(enabled: true).first&.platform_kubernetes @deployment_platform ||= deployment_services.reorder(nil).find_by(active: true) -- cgit v1.2.1 From a8e2094c6591336405a90e4185e89752d38965bf Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 21 Nov 2017 23:29:33 +0900 Subject: Remove logic which glues with KubernetesService, from Platforms::Kubernetes --- app/models/clusters/cluster.rb | 9 +---- app/models/clusters/platforms/kubernetes.rb | 51 ++--------------------------- 2 files changed, 3 insertions(+), 57 deletions(-) (limited to 'app/models') diff --git a/app/models/clusters/cluster.rb b/app/models/clusters/cluster.rb index 185d9473aab..6d7fb4b7dbf 100644 --- a/app/models/clusters/cluster.rb +++ b/app/models/clusters/cluster.rb @@ -17,8 +17,7 @@ module Clusters # we force autosave to happen when we save `Cluster` model has_one :provider_gcp, class_name: 'Clusters::Providers::Gcp', autosave: true - # We have to ":destroy" it today to ensure that we clean also the Kubernetes Integration - has_one :platform_kubernetes, class_name: 'Clusters::Platforms::Kubernetes', autosave: true, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_one :platform_kubernetes, class_name: 'Clusters::Platforms::Kubernetes' has_one :application_helm, class_name: 'Clusters::Applications::Helm' has_one :application_ingress, class_name: 'Clusters::Applications::Ingress' @@ -29,15 +28,9 @@ module Clusters validates :name, cluster_name: true validate :restrict_modification, on: :update - # TODO: Move back this into Clusters::Platforms::Kubernetes in 10.3 - # We need callback here because `enabled` belongs to Clusters::Cluster - # Callbacks in Clusters::Platforms::Kubernetes will not be called after update - after_save :update_kubernetes_integration! - delegate :status, to: :provider, allow_nil: true delegate :status_reason, to: :provider, allow_nil: true delegate :on_creation?, to: :provider, allow_nil: true - delegate :update_kubernetes_integration!, to: :platform, allow_nil: true delegate :active?, to: :platform_kubernetes, prefix: true, allow_nil: true delegate :installed?, to: :application_helm, prefix: true, allow_nil: true diff --git a/app/models/clusters/platforms/kubernetes.rb b/app/models/clusters/platforms/kubernetes.rb index 549b32ceef1..590d5da24cb 100644 --- a/app/models/clusters/platforms/kubernetes.rb +++ b/app/models/clusters/platforms/kubernetes.rb @@ -36,9 +36,6 @@ module Clusters after_save :clear_reactive_cache! - # TODO: Glue code till we migrate Kubernetes Integration into Platforms::Kubernetes - after_destroy :destroy_kubernetes_integration! - alias_attribute :ca_pem, :ca_cert delegate :project, to: :cluster, allow_nil: true @@ -85,33 +82,14 @@ module Clusters # Caches resources in the namespace so other calls don't need to block on # network access def calculate_reactive_cache - return unless active? && project && !project.pending_delete? + return unless enabled? && project && !project.pending_delete? # We may want to cache extra things in the future { pods: read_pods } end def kubeclient - @kubeclient ||= kubernetes_service.kubeclient if manages_kubernetes_service? - end - - def update_kubernetes_integration! - raise 'Kubernetes service already configured' unless manages_kubernetes_service? - - # This is neccesary, otheriwse enabled? returns true even though cluster updated with enabled: false - cluster.reload - - ensure_kubernetes_service&.update!( - active: enabled?, - api_url: api_url, - namespace: namespace, - token: token, - ca_pem: ca_cert - ) - end - - def active? - manages_kubernetes_service? + @kubeclient ||= build_kubeclient! end private @@ -192,31 +170,6 @@ module Clusters def enforce_namespace_to_lower_case self.namespace = self.namespace&.downcase end - - def enforce_namespace_to_lower_case - self.namespace = self.namespace&.downcase - end - - # TODO: glue code till we migrate Kubernetes Service into Platforms::Kubernetes class - def manages_kubernetes_service? - return true unless kubernetes_service&.active? - - kubernetes_service.api_url == api_url - end - - def destroy_kubernetes_integration! - return unless manages_kubernetes_service? - - kubernetes_service&.destroy! - end - - def kubernetes_service - @kubernetes_service ||= project&.kubernetes_service - end - - def ensure_kubernetes_service - @kubernetes_service ||= kubernetes_service || project&.build_kubernetes_service - end end end end -- cgit v1.2.1 From 45f2d0af4173ae8e06b548c4bef1fabe14353c85 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Wed, 22 Nov 2017 18:31:07 +0900 Subject: Add test suit for platform::kubernetes --- app/models/project.rb | 14 +++++++------- 1 file changed, 7 insertions(+), 7 deletions(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index 512fe77261a..a7f90bc7fc3 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -914,13 +914,6 @@ class Project < ActiveRecord::Base deployment_platform end - # TODO: This will be extended for multiple enviroment clusters - # TODO: Add super nice tests to check this interchangeability - def deployment_platform - @deployment_platform ||= clusters.where(enabled: true).first&.platform_kubernetes - @deployment_platform ||= deployment_services.reorder(nil).find_by(active: true) - end - def monitoring_services services.where(category: :monitoring) end @@ -1858,4 +1851,11 @@ class Project < ActiveRecord::Base raise ex end + + # TODO: This will be extended for multiple enviroment clusters + # TODO: Add super nice tests to check this interchangeability + def deployment_platform + @deployment_platform ||= clusters.where(enabled: true).first&.platform_kubernetes + @deployment_platform ||= deployment_services.reorder(nil).find_by(active: true) + end end -- cgit v1.2.1 From 53da3d976f3705a87edc50dca41748b5e479fc83 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 27 Nov 2017 22:35:16 +0900 Subject: Replce kubernetes_service and deployment_service to deployment_platform --- app/models/ci/pipeline.rb | 2 +- app/models/environment.rb | 4 ++-- app/models/project.rb | 28 ++++++++-------------------- 3 files changed, 11 insertions(+), 23 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index ebbefc51a4f..fd64670f6b0 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -365,7 +365,7 @@ module Ci end def has_kubernetes_active? - project.kubernetes_service&.active? + project.deployment_platform&.active? end def has_stage_seeds? diff --git a/app/models/environment.rb b/app/models/environment.rb index 21a028e351c..bf69b4c50f0 100644 --- a/app/models/environment.rb +++ b/app/models/environment.rb @@ -138,11 +138,11 @@ class Environment < ActiveRecord::Base end def has_terminals? - project.deployment_service.present? && available? && last_deployment.present? + project.deployment_platform.present? && available? && last_deployment.present? end def terminals - project.deployment_service.terminals(self) if has_terminals? + project.deployment_platform.terminals(self) if has_terminals? end def has_metrics? diff --git a/app/models/project.rb b/app/models/project.rb index a7f90bc7fc3..49c56e76dfc 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -123,7 +123,7 @@ class Project < ActiveRecord::Base has_one :bugzilla_service has_one :gitlab_issue_tracker_service, inverse_of: :project has_one :external_wiki_service - # has_one :kubernetes_service, inverse_of: :project + has_one :kubernetes_service, inverse_of: :project has_one :prometheus_service, inverse_of: :project has_one :mock_ci_service has_one :mock_deployment_service @@ -902,16 +902,11 @@ class Project < ActiveRecord::Base @ci_service ||= ci_services.reorder(nil).find_by(active: true) end - def deployment_services - services.where(category: :deployment) - end - - def deployment_service - deployment_platform - end - - def kubernetes_service - deployment_platform + # TODO: This will be extended for multiple enviroment clusters + # TODO: Add super nice tests to check this interchangeability + def deployment_platform + @deployment_platform ||= clusters.where(enabled: true).first&.platform_kubernetes + @deployment_platform ||= services.where(category: :deployment).reorder(nil).find_by(active: true) end def monitoring_services @@ -1556,9 +1551,9 @@ class Project < ActiveRecord::Base end def deployment_variables - return [] unless deployment_service + return [] unless deployment_platform - deployment_service.predefined_variables + deployment_platform.predefined_variables end def auto_devops_variables @@ -1851,11 +1846,4 @@ class Project < ActiveRecord::Base raise ex end - - # TODO: This will be extended for multiple enviroment clusters - # TODO: Add super nice tests to check this interchangeability - def deployment_platform - @deployment_platform ||= clusters.where(enabled: true).first&.platform_kubernetes - @deployment_platform ||= deployment_services.reorder(nil).find_by(active: true) - end end -- cgit v1.2.1 From f6d9dcf8382a00a5f2ae2100b13774b01f0328bb Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Mon, 27 Nov 2017 23:55:25 +0900 Subject: Fix unit tests --- app/models/clusters/platforms/kubernetes.rb | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/clusters/platforms/kubernetes.rb b/app/models/clusters/platforms/kubernetes.rb index 590d5da24cb..39b4f8f7bfc 100644 --- a/app/models/clusters/platforms/kubernetes.rb +++ b/app/models/clusters/platforms/kubernetes.rb @@ -6,7 +6,7 @@ module Clusters include ReactiveCaching self.table_name = 'cluster_platforms_kubernetes' - self.reactive_cache_key = ->(kubernetes) { [kubernetes.class.model_name.singular, kubernetes.cluster_id] } + self.reactive_cache_key = ->(kubernetes) { [kubernetes.class.model_name.singular, kubernetes.id] } belongs_to :cluster, inverse_of: :platform_kubernetes, class_name: 'Clusters::Cluster' @@ -41,6 +41,8 @@ module Clusters delegate :project, to: :cluster, allow_nil: true delegate :enabled?, to: :cluster, allow_nil: true + alias_method :active?, :enabled? + def actual_namespace if namespace.present? namespace -- cgit v1.2.1 From 54c70a775af21863d51bd2b3e96e5998c90ddaf7 Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 28 Nov 2017 01:26:14 +0900 Subject: Fix static analysys --- app/models/clusters/platforms/kubernetes.rb | 1 + app/models/project.rb | 2 +- 2 files changed, 2 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/clusters/platforms/kubernetes.rb b/app/models/clusters/platforms/kubernetes.rb index 39b4f8f7bfc..7ab670cf1ef 100644 --- a/app/models/clusters/platforms/kubernetes.rb +++ b/app/models/clusters/platforms/kubernetes.rb @@ -134,6 +134,7 @@ module Clusters kubeclient.get_pods(namespace: actual_namespace).as_json rescue KubeException => err raise err unless err.error_code == 404 + [] end diff --git a/app/models/project.rb b/app/models/project.rb index 49c56e76dfc..95aabc4ad45 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -905,7 +905,7 @@ class Project < ActiveRecord::Base # TODO: This will be extended for multiple enviroment clusters # TODO: Add super nice tests to check this interchangeability def deployment_platform - @deployment_platform ||= clusters.where(enabled: true).first&.platform_kubernetes + @deployment_platform ||= clusters.find_by(enabled: true)&.platform_kubernetes @deployment_platform ||= services.where(category: :deployment).reorder(nil).find_by(active: true) end -- cgit v1.2.1 From 7277b3b32c2afd26a033ecf81b93319efb65861d Mon Sep 17 00:00:00 2001 From: Shinya Maeda Date: Tue, 28 Nov 2017 03:06:38 +0900 Subject: Fix feature spec --- app/models/project.rb | 1 - 1 file changed, 1 deletion(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index 95aabc4ad45..027c437d7da 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -903,7 +903,6 @@ class Project < ActiveRecord::Base end # TODO: This will be extended for multiple enviroment clusters - # TODO: Add super nice tests to check this interchangeability def deployment_platform @deployment_platform ||= clusters.find_by(enabled: true)&.platform_kubernetes @deployment_platform ||= services.where(category: :deployment).reorder(nil).find_by(active: true) -- cgit v1.2.1 From 4ebbfe5d3e0dbe06346ee0c64a8f62ec11f9b6e8 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Tue, 21 Nov 2017 16:58:08 +0000 Subject: Remove serialised diff and commit columns The st_commits and st_diffs columns on merge_request_diffs historically held the YAML-serialised data for a merge request diff, in a variety of formats. Since 9.5, these have been migrated in the background to two new tables: merge_request_diff_commits and merge_request_diff_files. That has the advantage that we can actually query the data (for instance, to find out how many commits we've stored), and that it can't be in a variety of formats, but must match the new schema. This is the final step of that journey, where we drop those columns and remove all references to them. This is a breaking change to the importer, because we can no longer import diffs created in the old format, and we cannot guarantee the export will be in the new format unless it was generated after this commit. --- app/models/merge_request.rb | 34 +++++++------------ app/models/merge_request_diff.rb | 72 ++++++---------------------------------- 2 files changed, 22 insertions(+), 84 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index e4d8f486c77..2825735abc4 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -283,9 +283,9 @@ class MergeRequest < ActiveRecord::Base if persisted? merge_request_diff.commit_shas elsif compare_commits - compare_commits.reverse.map(&:sha) + compare_commits.to_a.reverse.map(&:sha) else - [] + Array(diff_head_sha) end end @@ -482,7 +482,7 @@ class MergeRequest < ActiveRecord::Base def merge_request_diff_for(diff_refs_or_sha) @merge_request_diffs_by_diff_refs_or_sha ||= Hash.new do |h, diff_refs_or_sha| - diffs = merge_request_diffs.viewable.select_without_diff + diffs = merge_request_diffs.viewable h[diff_refs_or_sha] = if diff_refs_or_sha.is_a?(Gitlab::Diff::DiffRefs) diffs.find_by_diff_refs(diff_refs_or_sha) @@ -906,28 +906,18 @@ class MergeRequest < ActiveRecord::Base # Note that this could also return SHA from now dangling commits # def all_commit_shas - if persisted? - # MySQL doesn't support LIMIT in a subquery. - diffs_relation = - if Gitlab::Database.postgresql? - merge_request_diffs.order(id: :desc).limit(100) - else - merge_request_diffs - end + return commit_shas unless persisted? - column_shas = MergeRequestDiffCommit - .where(merge_request_diff: diffs_relation) - .limit(10_000) - .pluck('sha') + diffs_relation = merge_request_diffs - serialised_shas = merge_request_diffs.where.not(st_commits: nil).flat_map(&:commit_shas) + # MySQL doesn't support LIMIT in a subquery. + diffs_relation = diffs_relation.recent if Gitlab::Database.postgresql? - (column_shas + serialised_shas).uniq - elsif compare_commits - compare_commits.to_a.reverse.map(&:id) - else - [diff_head_sha] - end + MergeRequestDiffCommit + .where(merge_request_diff: diffs_relation) + .limit(10_000) + .pluck('sha') + .uniq end def merge_commit diff --git a/app/models/merge_request_diff.rb b/app/models/merge_request_diff.rb index 9ee561b5161..c37aa0a594b 100644 --- a/app/models/merge_request_diff.rb +++ b/app/models/merge_request_diff.rb @@ -1,14 +1,14 @@ class MergeRequestDiff < ActiveRecord::Base include Sortable include Importable - include Gitlab::EncodingHelper include ManualInverseAssociation + include IgnorableColumn - # Prevent store of diff if commits amount more then 500 + # Don't display more than 100 commits at once COMMITS_SAFE_SIZE = 100 - # Valid types of serialized diffs allowed by Gitlab::Git::Diff - VALID_CLASSES = [Hash, Rugged::Patch, Rugged::Diff::Delta].freeze + ignore_column :st_commits, + :st_diffs belongs_to :merge_request manual_inverse_association :merge_request, :merge_request_diff @@ -16,9 +16,6 @@ class MergeRequestDiff < ActiveRecord::Base has_many :merge_request_diff_files, -> { order(:merge_request_diff_id, :relative_order) } has_many :merge_request_diff_commits, -> { order(:merge_request_diff_id, :relative_order) } - serialize :st_commits # rubocop:disable Cop/ActiveRecordSerialize - serialize :st_diffs # rubocop:disable Cop/ActiveRecordSerialize - state_machine :state, initial: :empty do state :collected state :overflow @@ -32,6 +29,8 @@ class MergeRequestDiff < ActiveRecord::Base scope :viewable, -> { without_state(:empty) } + scope :recent, -> { order(id: :desc).limit(100) } + # All diff information is collected from repository after object is created. # It allows you to override variables like head_commit_sha before getting diff. after_create :save_git_content, unless: :importing? @@ -40,14 +39,6 @@ class MergeRequestDiff < ActiveRecord::Base find_by(start_commit_sha: diff_refs.start_sha, head_commit_sha: diff_refs.head_sha, base_commit_sha: diff_refs.base_sha) end - def self.select_without_diff - select(column_names - ['st_diffs']) - end - - def st_commits - super || [] - end - # Collect information about commits and diff from repository # and save it to the database as serialized data def save_git_content @@ -129,11 +120,7 @@ class MergeRequestDiff < ActiveRecord::Base end def commit_shas - if st_commits.present? - st_commits.map { |commit| commit[:id] } - else - merge_request_diff_commits.map(&:sha) - end + merge_request_diff_commits.map(&:sha) end def diff_refs=(new_diff_refs) @@ -208,34 +195,11 @@ class MergeRequestDiff < ActiveRecord::Base end def commits_count - if st_commits.present? - st_commits.size - else - merge_request_diff_commits.size - end - end - - def utf8_st_diffs - return [] if st_diffs.blank? - - st_diffs.map do |diff| - diff.each do |k, v| - diff[k] = encode_utf8(v) if v.respond_to?(:encoding) - end - end + merge_request_diff_commits.size end private - # Old GitLab implementations may have generated diffs as ["--broken-diff"]. - # Avoid an error 500 by ignoring bad elements. See: - # https://gitlab.com/gitlab-org/gitlab-ce/issues/20776 - def valid_raw_diff?(raw) - return false unless raw.respond_to?(:each) - - raw.any? { |element| VALID_CLASSES.include?(element.class) } - end - def create_merge_request_diff_files(diffs) rows = diffs.map.with_index do |diff, index| diff_hash = diff.to_hash.merge( @@ -259,9 +223,7 @@ class MergeRequestDiff < ActiveRecord::Base end def load_diffs(options) - return Gitlab::Git::DiffCollection.new([]) unless diffs_from_database - - raw = diffs_from_database + raw = merge_request_diff_files.map(&:to_hash) if paths = options[:paths] raw = raw.select do |diff| @@ -272,22 +234,8 @@ class MergeRequestDiff < ActiveRecord::Base Gitlab::Git::DiffCollection.new(raw, options) end - def diffs_from_database - return @diffs_from_database if defined?(@diffs_from_database) - - @diffs_from_database = - if st_diffs.present? - if valid_raw_diff?(st_diffs) - st_diffs - end - elsif merge_request_diff_files.present? - merge_request_diff_files.map(&:to_hash) - end - end - def load_commits - commits = st_commits.presence || merge_request_diff_commits - commits = commits.map { |commit| Commit.from_hash(commit.to_hash, project) } + commits = merge_request_diff_commits.map { |commit| Commit.from_hash(commit.to_hash, project) } CommitCollection .new(merge_request.source_project, commits, merge_request.source_branch) -- cgit v1.2.1 From 3c6a4d63636ba41dad0ce63cf536761fc3b5ef64 Mon Sep 17 00:00:00 2001 From: Sean McGivern Date: Fri, 24 Nov 2017 11:58:05 +0000 Subject: Ensure MRs always use branch refs for comparison If a merge request was created with a branch name that also matched a tag name, we'd generate a comparison to or from the tag respectively, rather than the branch. Merging would still use the branch, of course. To avoid this, ensure that when we get the branch heads, we prepend the reference prefix for branches, which will ensure that we generate the correct comparison. --- app/models/merge_request.rb | 18 +++++++++++++++--- 1 file changed, 15 insertions(+), 3 deletions(-) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index 2753e4b16e5..57b169631f9 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -365,16 +365,28 @@ class MergeRequest < ActiveRecord::Base # We use these attributes to force these to the intended values. attr_writer :target_branch_sha, :source_branch_sha + def source_branch_ref + return @source_branch_sha if @source_branch_sha + return unless source_branch + + Gitlab::Git::BRANCH_REF_PREFIX + source_branch + end + + def target_branch_ref + return @target_branch_sha if @target_branch_sha + return unless target_branch + + Gitlab::Git::BRANCH_REF_PREFIX + target_branch + end + def source_branch_head return unless source_project - source_branch_ref = @source_branch_sha || source_branch source_project.repository.commit(source_branch_ref) if source_branch_ref end def target_branch_head - target_branch_ref = @target_branch_sha || target_branch - target_project.repository.commit(target_branch_ref) if target_branch_ref + target_project.repository.commit(target_branch_ref) end def branch_merge_base_commit -- cgit v1.2.1 From 64e5f996fa01d2e9c8462cfbb647997531eaf6c7 Mon Sep 17 00:00:00 2001 From: Andrew Newdigate Date: Wed, 29 Nov 2017 09:12:12 +0000 Subject: Add timeouts for Gitaly calls --- app/models/application_setting.rb | 26 +++++++++++++++++++++++++- 1 file changed, 25 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/application_setting.rb b/app/models/application_setting.rb index 01455a52d2a..3117c98c846 100644 --- a/app/models/application_setting.rb +++ b/app/models/application_setting.rb @@ -172,6 +172,27 @@ class ApplicationSetting < ActiveRecord::Base end end + validates :gitaly_timeout_default, + presence: true, + numericality: { only_integer: true, greater_than_or_equal_to: 0 } + + validates :gitaly_timeout_medium, + presence: true, + numericality: { only_integer: true, greater_than_or_equal_to: 0 } + validates :gitaly_timeout_medium, + numericality: { less_than_or_equal_to: :gitaly_timeout_default }, + if: :gitaly_timeout_default + validates :gitaly_timeout_medium, + numericality: { greater_than_or_equal_to: :gitaly_timeout_fast }, + if: :gitaly_timeout_fast + + validates :gitaly_timeout_fast, + presence: true, + numericality: { only_integer: true, greater_than_or_equal_to: 0 } + validates :gitaly_timeout_fast, + numericality: { less_than_or_equal_to: :gitaly_timeout_default }, + if: :gitaly_timeout_default + SUPPORTED_KEY_TYPES.each do |type| validates :"#{type}_key_restriction", presence: true, key_restriction: { type: type } end @@ -308,7 +329,10 @@ class ApplicationSetting < ActiveRecord::Base two_factor_grace_period: 48, user_default_external: false, polling_interval_multiplier: 1, - usage_ping_enabled: Settings.gitlab['usage_ping_enabled'] + usage_ping_enabled: Settings.gitlab['usage_ping_enabled'], + gitaly_timeout_fast: 10, + gitaly_timeout_medium: 30, + gitaly_timeout_default: 55 } end -- cgit v1.2.1 From a0527ab806b005eeffd3b4f983e33c4bd76ce6b3 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Tue, 28 Nov 2017 16:50:44 +0100 Subject: Only load branch names for protected branch checks When checking if a branch is protected we don't need all columns of every protected branch row, instead we only care about the names. By using "select" here we reduce the amount of data we need to send over the wire and load into memory. --- app/models/protected_branch.rb | 4 +++- app/models/protected_tag.rb | 4 +++- 2 files changed, 6 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/protected_branch.rb b/app/models/protected_branch.rb index 89bfc5f9a9c..d28fed11ca8 100644 --- a/app/models/protected_branch.rb +++ b/app/models/protected_branch.rb @@ -10,7 +10,9 @@ class ProtectedBranch < ActiveRecord::Base def self.protected?(project, ref_name) return true if project.empty_repo? && default_branch_protected? - self.matching(ref_name, protected_refs: project.protected_branches).present? + refs = project.protected_branches.select(:name) + + self.matching(ref_name, protected_refs: refs).present? end def self.default_branch_protected? diff --git a/app/models/protected_tag.rb b/app/models/protected_tag.rb index f38109c0e52..42a9bcf7723 100644 --- a/app/models/protected_tag.rb +++ b/app/models/protected_tag.rb @@ -5,6 +5,8 @@ class ProtectedTag < ActiveRecord::Base protected_ref_access_levels :create def self.protected?(project, ref_name) - self.matching(ref_name, protected_refs: project.protected_tags).present? + refs = project.protected_tags.select(:name) + + self.matching(ref_name, protected_refs: refs).present? end end -- cgit v1.2.1 From 4925ec50877c9bf1338d41a7676b3644f18370f7 Mon Sep 17 00:00:00 2001 From: Lin Jen-Shin Date: Tue, 28 Nov 2017 21:43:31 +0800 Subject: We could simply count the commits --- app/models/merge_request.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/merge_request.rb b/app/models/merge_request.rb index e232feaeada..bbc01e9677c 100644 --- a/app/models/merge_request.rb +++ b/app/models/merge_request.rb @@ -899,7 +899,8 @@ class MergeRequest < ActiveRecord::Base def compute_diverged_commits_count return 0 unless source_branch_sha && target_branch_sha - Gitlab::Git::Commit.between(target_project.repository.raw_repository, source_branch_sha, target_branch_sha).size + target_project.repository + .count_commits_between(source_branch_sha, target_branch_sha) end private :compute_diverged_commits_count -- cgit v1.2.1 From 9293842d096bf8de9d13ce2a714c646ffba27eb5 Mon Sep 17 00:00:00 2001 From: Grzegorz Bizon Date: Thu, 30 Nov 2017 12:53:35 +0100 Subject: Do not set pipeline source after initialization --- app/models/ci/pipeline.rb | 1 - 1 file changed, 1 deletion(-) (limited to 'app/models') diff --git a/app/models/ci/pipeline.rb b/app/models/ci/pipeline.rb index fd64670f6b0..eebbf7c4218 100644 --- a/app/models/ci/pipeline.rb +++ b/app/models/ci/pipeline.rb @@ -40,7 +40,6 @@ module Ci validates :status, presence: { unless: :importing? } validate :valid_commit_sha, unless: :importing? - after_initialize :set_config_source, if: :new_record? after_create :keep_around_commits, unless: :importing? enum source: { -- cgit v1.2.1 From 70194cfc28009f419f74580cf199fe91aa1754ec Mon Sep 17 00:00:00 2001 From: Christiaan Van den Poel Date: Fri, 1 Dec 2017 11:15:15 +0000 Subject: Adds validation for Project#ci_config_path not to contain leading slash --- app/models/project.rb | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index c6f7f56f311..eaf4f555d3b 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -234,8 +234,8 @@ class Project < ActiveRecord::Base validates :creator, presence: true, on: :create validates :description, length: { maximum: 2000 }, allow_blank: true validates :ci_config_path, - format: { without: /\.{2}/, - message: 'cannot include directory traversal.' }, + format: { without: /(\.{2}|\A\/)/, + message: 'cannot include leading slash or directory traversal.' }, length: { maximum: 255 }, allow_blank: true validates :name, @@ -599,7 +599,7 @@ class Project < ActiveRecord::Base def ci_config_path=(value) # Strip all leading slashes so that //foo -> foo - super(value&.sub(%r{\A/+}, '')&.delete("\0")) + super(value&.delete("\0")) end def import_url=(value) -- cgit v1.2.1 From 020a8482a47de625e2bfaa6f13a12e7cd8fe3600 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Fri, 1 Dec 2017 15:32:57 +0100 Subject: Use commit finder instead of rev parse This has the side effect of making this method rugged call free, which is the reason I actually changed this. --- app/models/repository.rb | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/repository.rb b/app/models/repository.rb index 165dafd83fd..82af299ec5e 100644 --- a/app/models/repository.rb +++ b/app/models/repository.rb @@ -256,7 +256,7 @@ class Repository end def diverging_commit_counts(branch) - root_ref_hash = raw_repository.rev_parse_target(root_ref).oid + root_ref_hash = raw_repository.commit(root_ref).id cache.fetch(:"diverging_commit_counts_#{branch.name}") do # Rugged seems to throw a `ReferenceError` when given branch_names rather # than SHA-1 hashes -- cgit v1.2.1 From ce704a9533f8026edbfd535f5477caf09604a8a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Fri, 1 Dec 2017 16:49:56 +0100 Subject: Add created? to Cluster model --- app/models/clusters/cluster.rb | 4 ++++ 1 file changed, 4 insertions(+) (limited to 'app/models') diff --git a/app/models/clusters/cluster.rb b/app/models/clusters/cluster.rb index 185d9473aab..bc3bb9d00d7 100644 --- a/app/models/clusters/cluster.rb +++ b/app/models/clusters/cluster.rb @@ -62,6 +62,10 @@ module Clusters end end + def created? + status_name == :created + end + def applications [ application_helm || build_application_helm, -- cgit v1.2.1 From 194f7bca9a286942bcd764c836180964e33a1e92 Mon Sep 17 00:00:00 2001 From: Francisco Lopez Date: Thu, 30 Nov 2017 12:52:38 +0100 Subject: Comments from code review applied. Also switched forked_from_project and ForkedProjectLinks to ForkNetworkMember --- app/models/group.rb | 8 ++++++++ app/models/project.rb | 6 +++++- app/models/user.rb | 6 +++++- 3 files changed, 18 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/group.rb b/app/models/group.rb index 76262acf50c..27287f079a4 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -289,6 +289,14 @@ class Group < Namespace "#{parent.full_path}/#{path_was}" end + def group_member(user) + if group_members.loaded? + group_members.find { |gm| gm.user_id == user.id } + else + group_members.find_by(user_id: user) + end + end + private def update_two_factor_requirement diff --git a/app/models/project.rb b/app/models/project.rb index eaf4f555d3b..6f57763b05b 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -1114,7 +1114,11 @@ class Project < ActiveRecord::Base end def project_member(user) - project_members.find_by(user_id: user) + if project_members.loaded? + project_members.find { |member| member.user_id == user.id } + else + project_members.find_by(user_id: user) + end end def default_branch diff --git a/app/models/user.rb b/app/models/user.rb index 14941fd7f98..ee12f541b0f 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -998,7 +998,11 @@ class User < ActiveRecord::Base end def notification_settings_for(source) - notification_settings.find_or_initialize_by(source: source) + if notification_settings.loaded? + notification_settings.find { |notification| notification.source == source } + else + notification_settings.find_or_initialize_by(source: source) + end end # Lazy load global notification setting -- cgit v1.2.1 From 636376dd4d41856cc965718725f06bb8caacfd34 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Mon, 18 Sep 2017 11:15:33 +0200 Subject: WIP --- app/models/ci/artifact.rb | 12 ++++++++++++ 1 file changed, 12 insertions(+) create mode 100644 app/models/ci/artifact.rb (limited to 'app/models') diff --git a/app/models/ci/artifact.rb b/app/models/ci/artifact.rb new file mode 100644 index 00000000000..c66c560037e --- /dev/null +++ b/app/models/ci/artifact.rb @@ -0,0 +1,12 @@ +module Ci + class Artifact < ActiveRecord::Base + belongs_to :build, class_name: "Ci::Build" + belongs_to :project, class_name: "Ci::Build" + + enum type { + archive: 0, + metadata: 1, + trace: 2 + } + end +end -- cgit v1.2.1 From 25df666156279e5b392b429519b4f4ba01eefaac Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 19 Sep 2017 09:14:06 +0200 Subject: Create Ci::Artifacts To allow jobs/builds to have multiple artifacts, and to start seperating concerns from Ci::Build a new model is created: Ci::Artifact. Changes include the updating of the ArtifactUploader to adapt to a slightly different interface. The uploader expects to be initialized with a `Ci::Build`. Futher a migration with the minimal fields, the needed foreign keys and an index. Last, the way this works is by prepending a module to Ci::Build so we can basically override behaviour but if needed use `super` to get the original behaviour. --- app/models/ci/artifact.rb | 28 +++++++++++++++------- app/models/ci/build.rb | 23 ++++++++----------- app/models/concerns/artifact_migratable.rb | 37 ++++++++++++++++++++++++++++++ 3 files changed, 66 insertions(+), 22 deletions(-) create mode 100644 app/models/concerns/artifact_migratable.rb (limited to 'app/models') diff --git a/app/models/ci/artifact.rb b/app/models/ci/artifact.rb index c66c560037e..858609883ce 100644 --- a/app/models/ci/artifact.rb +++ b/app/models/ci/artifact.rb @@ -1,12 +1,24 @@ module Ci class Artifact < ActiveRecord::Base - belongs_to :build, class_name: "Ci::Build" - belongs_to :project, class_name: "Ci::Build" - - enum type { - archive: 0, - metadata: 1, - trace: 2 - } + extend Gitlab::Ci::Model + + belongs_to :project + belongs_to :build, class_name: "Ci::Build", foreign_key: :ci_build_id + + before_save :set_size, if: :file_changed? + + mount_uploader :file, ArtifactUploader + + enum type: { archive: 0, metadata: 1 } + + # Allow us to use `type` as column name, without Rails thinking we're using + # STI: https://stackoverflow.com/a/29663933 + def self.inheritance_column + nil + end + + def set_size + self.size = file.exists? ? file.size : 0 + end end end diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 4ea040dfad5..fae2f5590b4 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -1,5 +1,6 @@ module Ci class Build < CommitStatus + prepend ArtifactMigratable include TokenAuthenticatable include AfterCommitQueue include Presentable @@ -10,6 +11,7 @@ module Ci belongs_to :erased_by, class_name: 'User' has_many :deployments, as: :deployable + has_many :artifacts, class_name: 'Ci::Artifact', foreign_key: :ci_build_id has_one :last_deployment, -> { order('deployments.id DESC') }, as: :deployable, class_name: 'Deployment' has_many :trace_sections, class_name: 'Ci::BuildTraceSection' @@ -326,14 +328,6 @@ module Ci project.running_or_pending_build_count(force: true) end - def artifacts? - !artifacts_expired? && artifacts_file.exists? - end - - def artifacts_metadata? - artifacts? && artifacts_metadata.exists? - end - def artifacts_metadata_entry(path, **options) metadata = Gitlab::Ci::Build::Artifacts::Metadata.new( artifacts_metadata.path, @@ -429,7 +423,7 @@ module Ci Gitlab::Ci::Build::Image.from_services(self) end - def artifacts + def artifacts_options [options[:artifacts]] end @@ -470,6 +464,12 @@ module Ci super(options).merge(when: read_attribute(:when)) end + def update_project_statistics + return unless project + + ProjectCacheWorker.perform_async(project_id, [], [:build_artifacts_size]) + end + private def update_artifacts_size @@ -560,11 +560,6 @@ module Ci pipeline.config_processor.build_attributes(name) end - def update_project_statistics - return unless project - - ProjectCacheWorker.perform_async(project_id, [], [:build_artifacts_size]) - end def update_project_statistics_after_save if previous_changes.include?('artifacts_size') diff --git a/app/models/concerns/artifact_migratable.rb b/app/models/concerns/artifact_migratable.rb new file mode 100644 index 00000000000..a14a278df9f --- /dev/null +++ b/app/models/concerns/artifact_migratable.rb @@ -0,0 +1,37 @@ +# Adapter class to unify the interface between mounted uploaders and the +# Ci::Artifact model +# Meant to be prepended so the interface can stay the same +module ArtifactMigratable + def artifacts_file + artifacts.archive.first&.file || super + end + + def artifacts_metadata + artifacts.metadata.first&.file || super + end + + def artifacts? + byebug + !artifacts_expired? && artifacts_file.exists? + end + + def artifacts_metadata? + artifacts? && artifacts_metadata.exists? + end + + def remove_artifacts_file! + artifacts_file.remove! + end + + def remove_artifacts_metadata! + artifacts_metadata.remove! + end + + def artifacts_file=(file) + artifacts.create(project: project, type: :archive, file: file) + end + + def artifacts_metadata=(file) + artifacts.create(project: project, type: :metadata, file: file) + end +end -- cgit v1.2.1 From 61864a5a5bb523953589c9398a431c4369fbfc76 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Thu, 21 Sep 2017 10:34:12 +0200 Subject: Rename Artifact to JobArtifact, split metadata out Two things at ones, as there was no clean way to seperate the commit and give me feedback from the tests. But the model Artifact is now JobArtifact, and the table does not have a type anymore, but the metadata is now its own model: Ci::JobArtifactMetadata. --- app/models/ci/artifact.rb | 24 ------------------------ app/models/ci/build.rb | 24 +++++++++++++++--------- app/models/ci/job_artifact.rb | 26 ++++++++++++++++++++++++++ app/models/concerns/artifact_migratable.rb | 30 +++++++++++++++++++----------- app/models/project_statistics.rb | 5 ++++- 5 files changed, 64 insertions(+), 45 deletions(-) delete mode 100644 app/models/ci/artifact.rb create mode 100644 app/models/ci/job_artifact.rb (limited to 'app/models') diff --git a/app/models/ci/artifact.rb b/app/models/ci/artifact.rb deleted file mode 100644 index 858609883ce..00000000000 --- a/app/models/ci/artifact.rb +++ /dev/null @@ -1,24 +0,0 @@ -module Ci - class Artifact < ActiveRecord::Base - extend Gitlab::Ci::Model - - belongs_to :project - belongs_to :build, class_name: "Ci::Build", foreign_key: :ci_build_id - - before_save :set_size, if: :file_changed? - - mount_uploader :file, ArtifactUploader - - enum type: { archive: 0, metadata: 1 } - - # Allow us to use `type` as column name, without Rails thinking we're using - # STI: https://stackoverflow.com/a/29663933 - def self.inheritance_column - nil - end - - def set_size - self.size = file.exists? ? file.size : 0 - end - end -end diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index fae2f5590b4..6d0ebd7f932 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -11,10 +11,15 @@ module Ci belongs_to :erased_by, class_name: 'User' has_many :deployments, as: :deployable - has_many :artifacts, class_name: 'Ci::Artifact', foreign_key: :ci_build_id + has_one :last_deployment, -> { order('deployments.id DESC') }, as: :deployable, class_name: 'Deployment' has_many :trace_sections, class_name: 'Ci::BuildTraceSection' + has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id + has_one :job_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id + has_one :job_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id + + # The "environment" field for builds is a String, and is the unexpanded name def persisted_environment @persisted_environment ||= Environment.find_by( @@ -33,7 +38,9 @@ module Ci scope :unstarted, ->() { where(runner_id: nil) } scope :ignore_failures, ->() { where(allow_failure: false) } - scope :with_artifacts, ->() { where.not(artifacts_file: [nil, '']) } + scope :with_artifacts, ->() do + where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)', '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.ci_job_id')) + end scope :with_artifacts_not_expired, ->() { with_artifacts.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) } scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) } scope :last_month, ->() { where('created_at > ?', Date.today - 1.month) } @@ -423,7 +430,7 @@ module Ci Gitlab::Ci::Build::Image.from_services(self) end - def artifacts_options + def artifacts [options[:artifacts]] end @@ -464,12 +471,6 @@ module Ci super(options).merge(when: read_attribute(:when)) end - def update_project_statistics - return unless project - - ProjectCacheWorker.perform_async(project_id, [], [:build_artifacts_size]) - end - private def update_artifacts_size @@ -560,6 +561,11 @@ module Ci pipeline.config_processor.build_attributes(name) end + def update_project_statistics + return unless project + + ProjectCacheWorker.perform_async(project_id, [], [:build_artifacts_size]) + end def update_project_statistics_after_save if previous_changes.include?('artifacts_size') diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb new file mode 100644 index 00000000000..9c709077ac6 --- /dev/null +++ b/app/models/ci/job_artifact.rb @@ -0,0 +1,26 @@ +module Ci + class JobArtifact < ActiveRecord::Base + extend Gitlab::Ci::Model + + belongs_to :project + belongs_to :job, class_name: "Ci::Build", foreign_key: :ci_job_id + + before_save :set_size, if: :file_changed? + after_commit :remove_file!, on: :destroy + + mount_uploader :file, JobArtifactUploader + + enum file_type: { + archive: 1, + metadata: 2 + } + + def self.artifacts_size_for(project) + self.where(project: project).sum(:size) + end + + def set_size + self.size = file.size + end + end +end diff --git a/app/models/concerns/artifact_migratable.rb b/app/models/concerns/artifact_migratable.rb index a14a278df9f..8e331617cfa 100644 --- a/app/models/concerns/artifact_migratable.rb +++ b/app/models/concerns/artifact_migratable.rb @@ -3,15 +3,14 @@ # Meant to be prepended so the interface can stay the same module ArtifactMigratable def artifacts_file - artifacts.archive.first&.file || super + job_archive&.file || super end def artifacts_metadata - artifacts.metadata.first&.file || super + job_metadata&.file || super end def artifacts? - byebug !artifacts_expired? && artifacts_file.exists? end @@ -19,19 +18,28 @@ module ArtifactMigratable artifacts? && artifacts_metadata.exists? end - def remove_artifacts_file! - artifacts_file.remove! + def artifacts_file_changed? + job_archive&.file_changed? || super end - def remove_artifacts_metadata! - artifacts_metadata.remove! + def remove_artifacts_file! + if job_archive + job_archive.destroy + else + super + end end - def artifacts_file=(file) - artifacts.create(project: project, type: :archive, file: file) + def remove_artifacts_metadata! + if job_metadata + job_metadata.destroy + else + super + end end - def artifacts_metadata=(file) - artifacts.create(project: project, type: :metadata, file: file) + def artifacts_size + read_attribute(:artifacts_size).to_i + + job_archive&.size.to_i + job_metadata&.size.to_i end end diff --git a/app/models/project_statistics.rb b/app/models/project_statistics.rb index 715b215d1db..a9c22d9cf18 100644 --- a/app/models/project_statistics.rb +++ b/app/models/project_statistics.rb @@ -35,7 +35,10 @@ class ProjectStatistics < ActiveRecord::Base end def update_build_artifacts_size - self.build_artifacts_size = project.builds.sum(:artifacts_size) + self.build_artifacts_size = + project.builds.sum(:artifacts_size) + + Ci::JobArtifact.artifacts_size_for(self) + + Ci::JobArtifactMetadata.artifacts_size_for(self) end def update_storage_size -- cgit v1.2.1 From 303f165cbae8367c19ea273fc52170c2a354a8d6 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 2 Nov 2017 12:16:50 +0100 Subject: Fix creation of job_artifact_uploader --- app/models/ci/build.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 6d0ebd7f932..8d0c62fcb49 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -15,11 +15,11 @@ module Ci has_one :last_deployment, -> { order('deployments.id DESC') }, as: :deployable, class_name: 'Deployment' has_many :trace_sections, class_name: 'Ci::BuildTraceSection' - has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id + # TODO: what to do with dependent destroy + has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id, dependent: :destroy has_one :job_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id has_one :job_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id - # The "environment" field for builds is a String, and is the unexpanded name def persisted_environment @persisted_environment ||= Environment.find_by( -- cgit v1.2.1 From c7d945758accc6f80ee63c7c3b25782cf7f6f3b0 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Thu, 2 Nov 2017 19:38:25 +0100 Subject: Fix most test failures --- app/models/ci/build.rb | 9 ++++----- app/models/ci/job_artifact.rb | 2 +- app/models/project_statistics.rb | 3 +-- 3 files changed, 6 insertions(+), 8 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 8d0c62fcb49..161800e9061 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -15,10 +15,9 @@ module Ci has_one :last_deployment, -> { order('deployments.id DESC') }, as: :deployable, class_name: 'Deployment' has_many :trace_sections, class_name: 'Ci::BuildTraceSection' - # TODO: what to do with dependent destroy - has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id, dependent: :destroy - has_one :job_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id - has_one :job_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :ci_job_id + has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :job_id, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent + has_one :job_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id + has_one :job_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id # The "environment" field for builds is a String, and is the unexpanded name def persisted_environment @@ -39,7 +38,7 @@ module Ci scope :unstarted, ->() { where(runner_id: nil) } scope :ignore_failures, ->() { where(allow_failure: false) } scope :with_artifacts, ->() do - where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)', '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.ci_job_id')) + where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)', '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id')) end scope :with_artifacts_not_expired, ->() { with_artifacts.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) } scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) } diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index 9c709077ac6..cc28092978c 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -3,7 +3,7 @@ module Ci extend Gitlab::Ci::Model belongs_to :project - belongs_to :job, class_name: "Ci::Build", foreign_key: :ci_job_id + belongs_to :job, class_name: "Ci::Build", foreign_key: :job_id before_save :set_size, if: :file_changed? after_commit :remove_file!, on: :destroy diff --git a/app/models/project_statistics.rb b/app/models/project_statistics.rb index a9c22d9cf18..17b9d2cf7b4 100644 --- a/app/models/project_statistics.rb +++ b/app/models/project_statistics.rb @@ -37,8 +37,7 @@ class ProjectStatistics < ActiveRecord::Base def update_build_artifacts_size self.build_artifacts_size = project.builds.sum(:artifacts_size) + - Ci::JobArtifact.artifacts_size_for(self) + - Ci::JobArtifactMetadata.artifacts_size_for(self) + Ci::JobArtifact.artifacts_size_for(self) end def update_storage_size -- cgit v1.2.1 From 871de0f18581bb03fed5c0d800f8183598a0195f Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 23 Nov 2017 16:57:27 +0100 Subject: Rename artifacts_* to legacy_artifacts_* --- app/models/ci/build.rb | 4 ++-- app/models/concerns/artifact_migratable.rb | 14 +++++++++----- 2 files changed, 11 insertions(+), 7 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 161800e9061..91747da28a1 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -46,8 +46,8 @@ module Ci scope :manual_actions, ->() { where(when: :manual, status: COMPLETED_STATUSES + [:manual]) } scope :ref_protected, -> { where(protected: true) } - mount_uploader :artifacts_file, ArtifactUploader - mount_uploader :artifacts_metadata, ArtifactUploader + mount_uploader :legacy_artifacts_file, LegacyArtifactUploader, mount_on: :artifacts_file + mount_uploader :legacy_artifacts_metadata, LegacyArtifactUploader, mount_on: :artifacts_metadata acts_as_taggable diff --git a/app/models/concerns/artifact_migratable.rb b/app/models/concerns/artifact_migratable.rb index 8e331617cfa..5c647bacf1b 100644 --- a/app/models/concerns/artifact_migratable.rb +++ b/app/models/concerns/artifact_migratable.rb @@ -3,11 +3,15 @@ # Meant to be prepended so the interface can stay the same module ArtifactMigratable def artifacts_file - job_archive&.file || super + job_archive&.file || legacy_artifacts_file + end + + def artifacts_file? + job_archive&.file? || legacy_artifacts_file? end def artifacts_metadata - job_metadata&.file || super + job_metadata&.file || legacy_artifacts_metadata end def artifacts? @@ -19,14 +23,14 @@ module ArtifactMigratable end def artifacts_file_changed? - job_archive&.file_changed? || super + job_archive&.file_changed? || attribute_changed?(:artifacts_file) end def remove_artifacts_file! if job_archive job_archive.destroy else - super + remove_legacy_artifacts_file! end end @@ -34,7 +38,7 @@ module ArtifactMigratable if job_metadata job_metadata.destroy else - super + remove_legacy_artifacts_metadata! end end -- cgit v1.2.1 From f5d3b92155b76d2459014372e00d877b21408f49 Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Mon, 27 Nov 2017 14:31:26 +0100 Subject: Remove Ci::Build#artifacts_file? --- app/models/concerns/artifact_migratable.rb | 4 ---- 1 file changed, 4 deletions(-) (limited to 'app/models') diff --git a/app/models/concerns/artifact_migratable.rb b/app/models/concerns/artifact_migratable.rb index 5c647bacf1b..811a8252459 100644 --- a/app/models/concerns/artifact_migratable.rb +++ b/app/models/concerns/artifact_migratable.rb @@ -6,10 +6,6 @@ module ArtifactMigratable job_archive&.file || legacy_artifacts_file end - def artifacts_file? - job_archive&.file? || legacy_artifacts_file? - end - def artifacts_metadata job_metadata&.file || legacy_artifacts_metadata end -- cgit v1.2.1 From 2a620e7412c8a658bae9a5b7c9b55b9ac97dd58e Mon Sep 17 00:00:00 2001 From: Zeger-Jan van de Weg Date: Tue, 28 Nov 2017 09:38:24 +0100 Subject: Remove hook set by carrierwave too --- app/models/ci/job_artifact.rb | 1 - 1 file changed, 1 deletion(-) (limited to 'app/models') diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index cc28092978c..e02fa021409 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -6,7 +6,6 @@ module Ci belongs_to :job, class_name: "Ci::Build", foreign_key: :job_id before_save :set_size, if: :file_changed? - after_commit :remove_file!, on: :destroy mount_uploader :file, JobArtifactUploader -- cgit v1.2.1 From 2045a771bfa5a1a32ff04f5bc23d58f1170b6359 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 30 Nov 2017 11:06:43 +0100 Subject: Rename `job_archive|metadata` to `artifacts_archive|metadata` --- app/models/ci/build.rb | 4 ++-- app/models/concerns/artifact_migratable.rb | 16 ++++++++-------- app/models/project_statistics.rb | 3 +++ 3 files changed, 13 insertions(+), 10 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 91747da28a1..842323e26e3 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -16,8 +16,8 @@ module Ci has_many :trace_sections, class_name: 'Ci::BuildTraceSection' has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :job_id, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - has_one :job_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id - has_one :job_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id + has_one :artifacts_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id + has_one :artifacts_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id # The "environment" field for builds is a String, and is the unexpanded name def persisted_environment diff --git a/app/models/concerns/artifact_migratable.rb b/app/models/concerns/artifact_migratable.rb index 811a8252459..b99585a0b65 100644 --- a/app/models/concerns/artifact_migratable.rb +++ b/app/models/concerns/artifact_migratable.rb @@ -3,11 +3,11 @@ # Meant to be prepended so the interface can stay the same module ArtifactMigratable def artifacts_file - job_archive&.file || legacy_artifacts_file + artifacts_archive&.file || legacy_artifacts_file end def artifacts_metadata - job_metadata&.file || legacy_artifacts_metadata + artifacts_metadata&.file || legacy_artifacts_metadata end def artifacts? @@ -19,20 +19,20 @@ module ArtifactMigratable end def artifacts_file_changed? - job_archive&.file_changed? || attribute_changed?(:artifacts_file) + artifacts_archive&.file_changed? || attribute_changed?(:artifacts_file) end def remove_artifacts_file! - if job_archive - job_archive.destroy + if artifacts_archive + artifacts_archive.destroy else remove_legacy_artifacts_file! end end def remove_artifacts_metadata! - if job_metadata - job_metadata.destroy + if artifacts_metadata + artifacts_metadata.destroy else remove_legacy_artifacts_metadata! end @@ -40,6 +40,6 @@ module ArtifactMigratable def artifacts_size read_attribute(:artifacts_size).to_i + - job_archive&.size.to_i + job_metadata&.size.to_i + artifacts_archive&.size.to_i + artifacts_metadata&.size.to_i end end diff --git a/app/models/project_statistics.rb b/app/models/project_statistics.rb index 17b9d2cf7b4..b3d4c82583c 100644 --- a/app/models/project_statistics.rb +++ b/app/models/project_statistics.rb @@ -35,6 +35,9 @@ class ProjectStatistics < ActiveRecord::Base end def update_build_artifacts_size + # REMARK: + # We perform dual calculation as we run SQL query: sum does not instantiate AR model. + # The Ci::Build#artifacts_size returns sum of ci_builds.artifacts_size and ci_job_artifacts.file_size self.build_artifacts_size = project.builds.sum(:artifacts_size) + Ci::JobArtifact.artifacts_size_for(self) -- cgit v1.2.1 From 8f01e67980d4ab8a7879800156cdc1dee07a4e8b Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 30 Nov 2017 14:19:07 +0100 Subject: Revert "Rename `job_archive|metadata` to `artifacts_archive|metadata`" This reverts commit 714082e65304ae2ec5c5400c59a68ab63e724aa9. --- app/models/ci/build.rb | 4 ++-- app/models/concerns/artifact_migratable.rb | 16 ++++++++-------- app/models/project_statistics.rb | 3 --- 3 files changed, 10 insertions(+), 13 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 842323e26e3..91747da28a1 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -16,8 +16,8 @@ module Ci has_many :trace_sections, class_name: 'Ci::BuildTraceSection' has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :job_id, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - has_one :artifacts_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id - has_one :artifacts_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id + has_one :job_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id + has_one :job_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id # The "environment" field for builds is a String, and is the unexpanded name def persisted_environment diff --git a/app/models/concerns/artifact_migratable.rb b/app/models/concerns/artifact_migratable.rb index b99585a0b65..811a8252459 100644 --- a/app/models/concerns/artifact_migratable.rb +++ b/app/models/concerns/artifact_migratable.rb @@ -3,11 +3,11 @@ # Meant to be prepended so the interface can stay the same module ArtifactMigratable def artifacts_file - artifacts_archive&.file || legacy_artifacts_file + job_archive&.file || legacy_artifacts_file end def artifacts_metadata - artifacts_metadata&.file || legacy_artifacts_metadata + job_metadata&.file || legacy_artifacts_metadata end def artifacts? @@ -19,20 +19,20 @@ module ArtifactMigratable end def artifacts_file_changed? - artifacts_archive&.file_changed? || attribute_changed?(:artifacts_file) + job_archive&.file_changed? || attribute_changed?(:artifacts_file) end def remove_artifacts_file! - if artifacts_archive - artifacts_archive.destroy + if job_archive + job_archive.destroy else remove_legacy_artifacts_file! end end def remove_artifacts_metadata! - if artifacts_metadata - artifacts_metadata.destroy + if job_metadata + job_metadata.destroy else remove_legacy_artifacts_metadata! end @@ -40,6 +40,6 @@ module ArtifactMigratable def artifacts_size read_attribute(:artifacts_size).to_i + - artifacts_archive&.size.to_i + artifacts_metadata&.size.to_i + job_archive&.size.to_i + job_metadata&.size.to_i end end diff --git a/app/models/project_statistics.rb b/app/models/project_statistics.rb index b3d4c82583c..17b9d2cf7b4 100644 --- a/app/models/project_statistics.rb +++ b/app/models/project_statistics.rb @@ -35,9 +35,6 @@ class ProjectStatistics < ActiveRecord::Base end def update_build_artifacts_size - # REMARK: - # We perform dual calculation as we run SQL query: sum does not instantiate AR model. - # The Ci::Build#artifacts_size returns sum of ci_builds.artifacts_size and ci_job_artifacts.file_size self.build_artifacts_size = project.builds.sum(:artifacts_size) + Ci::JobArtifact.artifacts_size_for(self) -- cgit v1.2.1 From ab02bd69425fe17e65717bed91e99b62e11c0e74 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 30 Nov 2017 14:18:38 +0100 Subject: Use `job_artifacts_archive|metadata` --- app/models/ci/build.rb | 4 ++-- app/models/concerns/artifact_migratable.rb | 16 ++++++++-------- 2 files changed, 10 insertions(+), 10 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 91747da28a1..d5c7cd0bb6a 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -16,8 +16,8 @@ module Ci has_many :trace_sections, class_name: 'Ci::BuildTraceSection' has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :job_id, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - has_one :job_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id - has_one :job_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id + has_one :job_artifacts_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id + has_one :job_artifacts_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id # The "environment" field for builds is a String, and is the unexpanded name def persisted_environment diff --git a/app/models/concerns/artifact_migratable.rb b/app/models/concerns/artifact_migratable.rb index 811a8252459..0460439e9e6 100644 --- a/app/models/concerns/artifact_migratable.rb +++ b/app/models/concerns/artifact_migratable.rb @@ -3,11 +3,11 @@ # Meant to be prepended so the interface can stay the same module ArtifactMigratable def artifacts_file - job_archive&.file || legacy_artifacts_file + job_artifacts_archive&.file || legacy_artifacts_file end def artifacts_metadata - job_metadata&.file || legacy_artifacts_metadata + job_artifacts_metadata&.file || legacy_artifacts_metadata end def artifacts? @@ -19,20 +19,20 @@ module ArtifactMigratable end def artifacts_file_changed? - job_archive&.file_changed? || attribute_changed?(:artifacts_file) + job_artifacts_archive&.file_changed? || attribute_changed?(:artifacts_file) end def remove_artifacts_file! - if job_archive - job_archive.destroy + if job_artifacts_archive + job_artifacts_archive.destroy else remove_legacy_artifacts_file! end end def remove_artifacts_metadata! - if job_metadata - job_metadata.destroy + if job_artifacts_metadata + job_artifacts_metadata.destroy else remove_legacy_artifacts_metadata! end @@ -40,6 +40,6 @@ module ArtifactMigratable def artifacts_size read_attribute(:artifacts_size).to_i + - job_archive&.size.to_i + job_metadata&.size.to_i + job_artifacts_archive&.size.to_i + job_artifacts_metadata&.size.to_i end end -- cgit v1.2.1 From 0464c25f602dc2e4d147ca2ac714d49bf96ddcf2 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Sun, 3 Dec 2017 12:02:11 +0100 Subject: Store expire_at in ci_job_artifacts --- app/models/ci/build.rb | 7 ++++--- app/models/ci/job_artifact.rb | 11 +++++++++++ 2 files changed, 15 insertions(+), 3 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index d5c7cd0bb6a..a19273484ef 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -16,8 +16,8 @@ module Ci has_many :trace_sections, class_name: 'Ci::BuildTraceSection' has_many :job_artifacts, class_name: 'Ci::JobArtifact', foreign_key: :job_id, dependent: :destroy # rubocop:disable Cop/ActiveRecordDependent - has_one :job_artifacts_archive, -> () { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id - has_one :job_artifacts_metadata, -> () { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', foreign_key: :job_id + has_one :job_artifacts_archive, -> { where(file_type: Ci::JobArtifact.file_types[:archive]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id + has_one :job_artifacts_metadata, -> { where(file_type: Ci::JobArtifact.file_types[:metadata]) }, class_name: 'Ci::JobArtifact', inverse_of: :job, foreign_key: :job_id # The "environment" field for builds is a String, and is the unexpanded name def persisted_environment @@ -38,7 +38,7 @@ module Ci scope :unstarted, ->() { where(runner_id: nil) } scope :ignore_failures, ->() { where(allow_failure: false) } scope :with_artifacts, ->() do - where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)', '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id')) + where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)', '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.build_id')) end scope :with_artifacts_not_expired, ->() { with_artifacts.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) } scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) } @@ -386,6 +386,7 @@ module Ci def keep_artifacts! self.update(artifacts_expire_at: nil) + self.job_artifacts.update_all(expire_at: nil) end def coverage_regex diff --git a/app/models/ci/job_artifact.rb b/app/models/ci/job_artifact.rb index e02fa021409..84fc6863567 100644 --- a/app/models/ci/job_artifact.rb +++ b/app/models/ci/job_artifact.rb @@ -21,5 +21,16 @@ module Ci def set_size self.size = file.size end + + def expire_in + expire_at - Time.now if expire_at + end + + def expire_in=(value) + self.expire_at = + if value + ChronicDuration.parse(value)&.seconds&.from_now + end + end end end -- cgit v1.2.1 From 6b6fb11a235893335bc8d67b86c9328d027b72e0 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Sun, 3 Dec 2017 12:04:16 +0100 Subject: Code style --- app/models/ci/build.rb | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index a19273484ef..c41757cf394 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -38,7 +38,8 @@ module Ci scope :unstarted, ->() { where(runner_id: nil) } scope :ignore_failures, ->() { where(allow_failure: false) } scope :with_artifacts, ->() do - where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)', '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.build_id')) + where('(artifacts_file IS NOT NULL AND artifacts_file <> ?) OR EXISTS (?)', + '', Ci::JobArtifact.select(1).where('ci_builds.id = ci_job_artifacts.job_id')) end scope :with_artifacts_not_expired, ->() { with_artifacts.where('artifacts_expire_at IS NULL OR artifacts_expire_at > ?', Time.now) } scope :with_expired_artifacts, ->() { with_artifacts.where('artifacts_expire_at < ?', Time.now) } -- cgit v1.2.1 From cd1b1cbfd681a7ffe46940459a556584efd0d8ec Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 2 Nov 2017 11:30:07 +0100 Subject: Extend controllers to support a new data structure for manual Kubernetes clusters --- app/models/clusters/cluster.rb | 6 ++++++ 1 file changed, 6 insertions(+) (limited to 'app/models') diff --git a/app/models/clusters/cluster.rb b/app/models/clusters/cluster.rb index 6d7fb4b7dbf..e13e367fb92 100644 --- a/app/models/clusters/cluster.rb +++ b/app/models/clusters/cluster.rb @@ -28,6 +28,8 @@ module Clusters validates :name, cluster_name: true validate :restrict_modification, on: :update + validates_associated :provider_gcp, :platform_kubernetes + delegate :status, to: :provider, allow_nil: true delegate :status_reason, to: :provider, allow_nil: true delegate :on_creation?, to: :provider, allow_nil: true @@ -70,6 +72,10 @@ module Clusters return platform_kubernetes if kubernetes? end + def managed? + !user? + end + def first_project return @first_project if defined?(@first_project) -- cgit v1.2.1 From 7af2161ca50484fea77ef816dc98ba66ff848681 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Sun, 3 Dec 2017 15:00:33 +0100 Subject: Fix controllers and links --- app/models/clusters/cluster.rb | 9 ++++++--- 1 file changed, 6 insertions(+), 3 deletions(-) (limited to 'app/models') diff --git a/app/models/clusters/cluster.rb b/app/models/clusters/cluster.rb index e13e367fb92..441cbcb701d 100644 --- a/app/models/clusters/cluster.rb +++ b/app/models/clusters/cluster.rb @@ -17,7 +17,7 @@ module Clusters # we force autosave to happen when we save `Cluster` model has_one :provider_gcp, class_name: 'Clusters::Providers::Gcp', autosave: true - has_one :platform_kubernetes, class_name: 'Clusters::Platforms::Kubernetes' + has_one :platform_kubernetes, class_name: 'Clusters::Platforms::Kubernetes', autosave: true has_one :application_helm, class_name: 'Clusters::Applications::Helm' has_one :application_ingress, class_name: 'Clusters::Applications::Ingress' @@ -28,8 +28,6 @@ module Clusters validates :name, cluster_name: true validate :restrict_modification, on: :update - validates_associated :provider_gcp, :platform_kubernetes - delegate :status, to: :provider, allow_nil: true delegate :status_reason, to: :provider, allow_nil: true delegate :on_creation?, to: :provider, allow_nil: true @@ -95,6 +93,11 @@ module Clusters return false end + if managed? && name_changed? + errors.add(:base, "cannot modify cluster name") + return false + end + true end end -- cgit v1.2.1 From ec09883b3683b6cf59e01933170972d3e3c0faf1 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Sun, 3 Dec 2017 15:04:34 +0100 Subject: Fix prevent modification --- app/models/clusters/platforms/kubernetes.rb | 14 ++++++++++++++ 1 file changed, 14 insertions(+) (limited to 'app/models') diff --git a/app/models/clusters/platforms/kubernetes.rb b/app/models/clusters/platforms/kubernetes.rb index 7ab670cf1ef..9160a169452 100644 --- a/app/models/clusters/platforms/kubernetes.rb +++ b/app/models/clusters/platforms/kubernetes.rb @@ -34,12 +34,15 @@ module Clusters validates :api_url, url: true, presence: true validates :token, presence: true + validate :prevent_modification, on: :update + after_save :clear_reactive_cache! alias_attribute :ca_pem, :ca_cert delegate :project, to: :cluster, allow_nil: true delegate :enabled?, to: :cluster, allow_nil: true + delegate :managed?, to: :cluster, allow_nil: true alias_method :active?, :enabled? @@ -173,6 +176,17 @@ module Clusters def enforce_namespace_to_lower_case self.namespace = self.namespace&.downcase end + + def prevent_modification + return unless managed? + + if api_url_changed? || token_changed? || ca_pem_changed? + errors.add(:base, "cannot modify managed cluster") + return false + end + + true + end end end end -- cgit v1.2.1 From 676f48794663e65a7e7290bba19038cd60e6ffa1 Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Sun, 3 Dec 2017 16:21:59 +0100 Subject: Fix failures --- app/models/ci/build.rb | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index c41757cf394..8738e094510 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -475,11 +475,7 @@ module Ci private def update_artifacts_size - self.artifacts_size = if artifacts_file.exists? - artifacts_file.size - else - nil - end + self.artifacts_size = legacy_artifacts_file&.size end def erase_trace! -- cgit v1.2.1 From 327a9898a226a098b18e80e4950702064ecd38f1 Mon Sep 17 00:00:00 2001 From: Nick Thomas Date: Thu, 30 Nov 2017 21:34:31 +0000 Subject: Fix the fork project functionality for projects with hashed storage Note the dependency on gitlab-shell v5.10.0 --- app/models/project.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index eaf4f555d3b..f9c82029912 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -562,8 +562,7 @@ class Project < ActiveRecord::Base if forked? RepositoryForkWorker.perform_async(id, forked_from_project.repository_storage_path, - forked_from_project.full_path, - self.namespace.full_path) + forked_from_project.disk_path) else RepositoryImportWorker.perform_async(self.id) end -- cgit v1.2.1 From 7c2b7296d4e623ba4eaa19cf03405ee7f2ae1ca9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Francisco=20Javier=20L=C3=B3pez?= Date: Mon, 4 Dec 2017 09:49:53 +0000 Subject: Added default order to UserFinder --- app/models/user.rb | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/user.rb b/app/models/user.rb index 14941fd7f98..5e7fe01c825 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -487,7 +487,11 @@ class User < ActiveRecord::Base end def two_factor_u2f_enabled? - u2f_registrations.exists? + if u2f_registrations.loaded? + u2f_registrations.any? + else + u2f_registrations.exists? + end end def namespace_uniq -- cgit v1.2.1 From a42b43ce7a9146ac1035328f2bc1cfc5f06e9371 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Matija=20=C4=8Cupi=C4=87?= Date: Mon, 4 Dec 2017 16:51:06 +0100 Subject: Remove cluster from Project model --- app/models/project.rb | 1 - 1 file changed, 1 deletion(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index eaf4f555d3b..0cb07e14cd8 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -189,7 +189,6 @@ class Project < ActiveRecord::Base has_one :statistics, class_name: 'ProjectStatistics' has_one :cluster_project, class_name: 'Clusters::Project' - has_one :cluster, through: :cluster_project, class_name: 'Clusters::Cluster' has_many :clusters, through: :cluster_project, class_name: 'Clusters::Cluster' # Container repositories need to remove data from the container registry, -- cgit v1.2.1 From b8a393192529015bc2fa9d04c2782cf96e7ec896 Mon Sep 17 00:00:00 2001 From: Markus Koller Date: Wed, 22 Nov 2017 14:48:38 +0100 Subject: Add custom brand text on new project pages --- app/models/appearance.rb | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/appearance.rb b/app/models/appearance.rb index ff15689ecac..76cfe28742a 100644 --- a/app/models/appearance.rb +++ b/app/models/appearance.rb @@ -2,9 +2,8 @@ class Appearance < ActiveRecord::Base include CacheMarkdownField cache_markdown_field :description + cache_markdown_field :new_project_guidelines - validates :title, presence: true - validates :description, presence: true validates :logo, file_size: { maximum: 1.megabyte } validates :header_logo, file_size: { maximum: 1.megabyte } -- cgit v1.2.1 From 20f78421c82d626a3b43924146b7878fe4777ae8 Mon Sep 17 00:00:00 2001 From: Bob Van Landuyt Date: Thu, 30 Nov 2017 14:26:08 +0100 Subject: Cache the forks in a namespace in the RequestStore On the `show` of a project that is part of a fork network. We check if the user already created a fork of this project in their personal namespace. We do this in several places, so caching the result of this query in the request store prevents us from repeating it. --- app/models/namespace.rb | 12 +++++++++++- 1 file changed, 11 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/namespace.rb b/app/models/namespace.rb index fa76729a702..901dbf2ba69 100644 --- a/app/models/namespace.rb +++ b/app/models/namespace.rb @@ -139,7 +139,17 @@ class Namespace < ActiveRecord::Base def find_fork_of(project) return nil unless project.fork_network - project.fork_network.find_forks_in(projects).first + if RequestStore.active? + forks_in_namespace = RequestStore.fetch("namespaces:#{id}:forked_projects") do + Hash.new do |found_forks, project| + found_forks[project] = project.fork_network.find_forks_in(projects).first + end + end + + forks_in_namespace[project] + else + project.fork_network.find_forks_in(projects).first + end end def lfs_enabled? -- cgit v1.2.1 From 8cce70730c2fb9c705e1f1177f6d1effc665b3c7 Mon Sep 17 00:00:00 2001 From: Jan Provaznik Date: Thu, 24 Aug 2017 08:20:36 +0200 Subject: Create merge request from email * new merge request can be created by sending an email to the specific email address (similar to creating issues by email) * for the first iteration, source branch must be specified in the mail subject, other merge request parameters can not be set yet * user should enable "Receive notifications about your own activity" in user settings to receive a notification about created merge request Part of #32878 --- app/models/project.rb | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) (limited to 'app/models') diff --git a/app/models/project.rb b/app/models/project.rb index 0a7e7617d9b..cc530076bf7 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -752,13 +752,14 @@ class Project < ActiveRecord::Base Gitlab::Routing.url_helpers.project_url(self) end - def new_issue_address(author) + def new_issuable_address(author, address_type) return unless Gitlab::IncomingEmail.supports_issue_creation? && author author.ensure_incoming_email_token! + suffix = address_type == 'merge_request' ? '+merge-request' : '' Gitlab::IncomingEmail.reply_address( - "#{full_path}+#{author.incoming_email_token}") + "#{full_path}#{suffix}+#{author.incoming_email_token}") end def build_commit_note(commit) -- cgit v1.2.1 From 1e6ca3c41ead23c5e433460c8c807ea73d9ec0ef Mon Sep 17 00:00:00 2001 From: Douwe Maan Date: Wed, 29 Nov 2017 16:30:17 +0100 Subject: Consistently schedule Sidekiq jobs --- app/models/group.rb | 1 + app/models/key.rb | 1 + app/models/member.rb | 1 + app/models/service.rb | 2 +- app/models/user.rb | 2 ++ 5 files changed, 6 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/group.rb b/app/models/group.rb index 27287f079a4..505e943e464 100644 --- a/app/models/group.rb +++ b/app/models/group.rb @@ -2,6 +2,7 @@ require 'carrierwave/orm/activerecord' class Group < Namespace include Gitlab::ConfigHelper + include AfterCommitQueue include AccessRequestable include Avatarable include Referable diff --git a/app/models/key.rb b/app/models/key.rb index 815fd1de909..a3f8a5d6dc7 100644 --- a/app/models/key.rb +++ b/app/models/key.rb @@ -2,6 +2,7 @@ require 'digest/md5' class Key < ActiveRecord::Base include Gitlab::CurrentSettings + include AfterCommitQueue include Sortable belongs_to :user diff --git a/app/models/member.rb b/app/models/member.rb index cbbd58f2eaf..2fe5fda985f 100644 --- a/app/models/member.rb +++ b/app/models/member.rb @@ -1,4 +1,5 @@ class Member < ActiveRecord::Base + include AfterCommitQueue include Sortable include Importable include Expirable diff --git a/app/models/service.rb b/app/models/service.rb index fdd2605e3e3..3c4f1885dd0 100644 --- a/app/models/service.rb +++ b/app/models/service.rb @@ -211,7 +211,7 @@ class Service < ActiveRecord::Base def async_execute(data) return unless supported_events.include?(data[:object_kind]) - Sidekiq::Client.enqueue(ProjectServiceWorker, id, data) + ProjectServiceWorker.perform_async(id, data) end def issue_tracker? diff --git a/app/models/user.rb b/app/models/user.rb index 76fd395be9a..38ee4ed50c1 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -7,6 +7,7 @@ class User < ActiveRecord::Base include Gitlab::ConfigHelper include Gitlab::CurrentSettings include Gitlab::SQL::Pattern + include AfterCommitQueue include Avatarable include Referable include Sortable @@ -903,6 +904,7 @@ class User < ActiveRecord::Base def post_destroy_hook log_info("User \"#{name}\" (#{email}) was removed") + system_hook_service.execute_hooks_for(self, :destroy) end -- cgit v1.2.1 From 4b66bdfa1af8fbef5d2af94a62e2522806cc7250 Mon Sep 17 00:00:00 2001 From: Filipa Lacerda Date: Tue, 5 Dec 2017 12:00:02 +0000 Subject: Second iteration of Move Kubernetes from service to Cluster page --- app/models/clusters/cluster.rb | 6 +++++- app/models/clusters/platforms/kubernetes.rb | 14 ++++++++++++++ 2 files changed, 19 insertions(+), 1 deletion(-) (limited to 'app/models') diff --git a/app/models/clusters/cluster.rb b/app/models/clusters/cluster.rb index 6d7fb4b7dbf..45beced1427 100644 --- a/app/models/clusters/cluster.rb +++ b/app/models/clusters/cluster.rb @@ -17,7 +17,7 @@ module Clusters # we force autosave to happen when we save `Cluster` model has_one :provider_gcp, class_name: 'Clusters::Providers::Gcp', autosave: true - has_one :platform_kubernetes, class_name: 'Clusters::Platforms::Kubernetes' + has_one :platform_kubernetes, class_name: 'Clusters::Platforms::Kubernetes', autosave: true has_one :application_helm, class_name: 'Clusters::Applications::Helm' has_one :application_ingress, class_name: 'Clusters::Applications::Ingress' @@ -70,6 +70,10 @@ module Clusters return platform_kubernetes if kubernetes? end + def managed? + !user? + end + def first_project return @first_project if defined?(@first_project) diff --git a/app/models/clusters/platforms/kubernetes.rb b/app/models/clusters/platforms/kubernetes.rb index 7ab670cf1ef..9160a169452 100644 --- a/app/models/clusters/platforms/kubernetes.rb +++ b/app/models/clusters/platforms/kubernetes.rb @@ -34,12 +34,15 @@ module Clusters validates :api_url, url: true, presence: true validates :token, presence: true + validate :prevent_modification, on: :update + after_save :clear_reactive_cache! alias_attribute :ca_pem, :ca_cert delegate :project, to: :cluster, allow_nil: true delegate :enabled?, to: :cluster, allow_nil: true + delegate :managed?, to: :cluster, allow_nil: true alias_method :active?, :enabled? @@ -173,6 +176,17 @@ module Clusters def enforce_namespace_to_lower_case self.namespace = self.namespace&.downcase end + + def prevent_modification + return unless managed? + + if api_url_changed? || token_changed? || ca_pem_changed? + errors.add(:base, "cannot modify managed cluster") + return false + end + + true + end end end end -- cgit v1.2.1 From 1efb219561a3be804c30de8e0a74cbfc85b1a4cd Mon Sep 17 00:00:00 2001 From: Kamil Trzcinski Date: Thu, 30 Nov 2017 23:17:41 +0100 Subject: Perform SQL matching of Build&Runner tags to greatly speed-up job picking --- app/models/ci/build.rb | 19 +++++++++++++++++++ app/models/ci/runner.rb | 6 +++--- 2 files changed, 22 insertions(+), 3 deletions(-) (limited to 'app/models') diff --git a/app/models/ci/build.rb b/app/models/ci/build.rb index 8738e094510..d2402b55184 100644 --- a/app/models/ci/build.rb +++ b/app/models/ci/build.rb @@ -47,6 +47,25 @@ module Ci scope :manual_actions, ->() { where(when: :manual, status: COMPLETED_STATUSES + [:manual]) } scope :ref_protected, -> { where(protected: true) } + scope :matches_tag_ids, -> (tag_ids) do + matcher = ::ActsAsTaggableOn::Tagging + .where(taggable_type: CommitStatus) + .where(context: 'tags') + .where('taggable_id = ci_builds.id') + .where.not(tag_id: tag_ids).select('1') + + where("NOT EXISTS (?)", matcher) + end + + scope :with_any_tags, -> do + matcher = ::ActsAsTaggableOn::Tagging + .where(taggable_type: CommitStatus) + .where(context: 'tags') + .where('taggable_id = ci_builds.id').select('1') + + where("EXISTS (?)", matcher) + end + mount_uploader :legacy_artifacts_file, LegacyArtifactUploader, mount_on: :artifacts_file mount_uploader :legacy_artifacts_metadata, LegacyArtifactUploader, mount_on: :artifacts_metadata diff --git a/app/models/ci/runner.rb b/app/models/ci/runner.rb index d39610a8995..dcbb397fb78 100644 --- a/app/models/ci/runner.rb +++ b/app/models/ci/runner.rb @@ -112,7 +112,7 @@ module Ci def can_pick?(build) return false if self.ref_protected? && !build.protected? - assignable_for?(build.project) && accepting_tags?(build) + assignable_for?(build.project_id) && accepting_tags?(build) end def only_for?(project) @@ -171,8 +171,8 @@ module Ci end end - def assignable_for?(project) - is_shared? || projects.exists?(id: project.id) + def assignable_for?(project_id) + is_shared? || projects.exists?(id: project_id) end def accepting_tags?(build) -- cgit v1.2.1