diff options
| -rw-r--r-- | app/models/resource_event.rb (renamed from app/models/concerns/resource_event_tools.rb) | 25 | ||||
| -rw-r--r-- | app/models/resource_label_event.rb | 18 | ||||
| -rw-r--r-- | app/models/resource_milestone_event.rb | 12 | ||||
| -rw-r--r-- | app/models/resource_weight_event.rb | 19 | ||||
| -rw-r--r-- | changelogs/unreleased/207126-more-descriptive-error-messages-in-migration-helpers.yml | 5 | ||||
| -rw-r--r-- | doc/api/scim.md | 2 | ||||
| -rw-r--r-- | doc/ci/variables/README.md | 9 | ||||
| -rw-r--r-- | doc/ci/yaml/README.md | 5 | ||||
| -rw-r--r-- | doc/user/project/deploy_tokens/index.md | 22 | ||||
| -rw-r--r-- | doc/user/search/index.md | 2 | ||||
| -rw-r--r-- | lib/gitlab/database/migration_helpers.rb | 19 | ||||
| -rw-r--r-- | spec/lib/gitlab/database/migration_helpers_spec.rb | 92 | ||||
| -rw-r--r-- | spec/models/resource_label_event_spec.rb | 4 | ||||
| -rw-r--r-- | spec/models/resource_weight_event_spec.rb | 5 | ||||
| -rw-r--r-- | spec/support/shared_examples/resource_events.rb | 13 |
15 files changed, 158 insertions, 94 deletions
diff --git a/app/models/concerns/resource_event_tools.rb b/app/models/resource_event.rb index 7226b9573e1..9b3a211ad43 100644 --- a/app/models/concerns/resource_event_tools.rb +++ b/app/models/resource_event.rb @@ -1,16 +1,27 @@ # frozen_string_literal: true -module ResourceEventTools - extend ActiveSupport::Concern +class ResourceEvent < ApplicationRecord + include Gitlab::Utils::StrongMemoize + include Importable - included do - belongs_to :user + self.abstract_class = true - validates :user, presence: { unless: :importing? }, on: :create + validates :user, presence: { unless: :importing? }, on: :create - validate :exactly_one_issuable + belongs_to :user - scope :created_after, ->(time) { where('created_at > ?', time) } + scope :created_after, ->(time) { where('created_at > ?', time) } + + def discussion_id + strong_memoize(:discussion_id) do + Digest::SHA1.hexdigest(discussion_id_key.join("-")) + end + end + + private + + def discussion_id_key + [self.class.name, created_at, user_id] end def exactly_one_issuable diff --git a/app/models/resource_label_event.rb b/app/models/resource_label_event.rb index 59907f1b962..970d4e1e562 100644 --- a/app/models/resource_label_event.rb +++ b/app/models/resource_label_event.rb @@ -1,10 +1,7 @@ # frozen_string_literal: true -class ResourceLabelEvent < ApplicationRecord - include Importable - include Gitlab::Utils::StrongMemoize +class ResourceLabelEvent < ResourceEvent include CacheMarkdownField - include ResourceEventTools cache_markdown_field :reference @@ -13,8 +10,11 @@ class ResourceLabelEvent < ApplicationRecord belongs_to :label scope :inc_relations, -> { includes(:label, :user) } + scope :by_issue, ->(issue) { where(issue_id: issue.id) } + scope :by_merge_request, ->(merge_request) { where(merge_request_id: merge_request.id) } validates :label, presence: { unless: :importing? }, on: :create + validate :exactly_one_issuable after_save :expire_etag_cache after_destroy :expire_etag_cache @@ -41,12 +41,6 @@ class ResourceLabelEvent < ApplicationRecord issue || merge_request end - def discussion_id(resource = nil) - strong_memoize(:discussion_id) do - Digest::SHA1.hexdigest(discussion_id_key.join("-")) - end - end - def project issuable.project end @@ -109,10 +103,6 @@ class ResourceLabelEvent < ApplicationRecord def resource_parent issuable.project || issuable.group end - - def discussion_id_key - [self.class.name, created_at, user_id] - end end ResourceLabelEvent.prepend_if_ee('EE::ResourceLabelEvent') diff --git a/app/models/resource_milestone_event.rb b/app/models/resource_milestone_event.rb index ba43a1ee363..d362ebc307a 100644 --- a/app/models/resource_milestone_event.rb +++ b/app/models/resource_milestone_event.rb @@ -1,10 +1,6 @@ # frozen_string_literal: true -class ResourceMilestoneEvent < ApplicationRecord - include Gitlab::Utils::StrongMemoize - include Importable - include ResourceEventTools - +class ResourceMilestoneEvent < ResourceEvent belongs_to :issue belongs_to :merge_request belongs_to :milestone @@ -12,6 +8,8 @@ class ResourceMilestoneEvent < ApplicationRecord scope :by_issue, ->(issue) { where(issue_id: issue.id) } scope :by_merge_request, ->(merge_request) { where(merge_request_id: merge_request.id) } + validate :exactly_one_issuable + enum action: { add: 1, remove: 2 @@ -23,8 +21,4 @@ class ResourceMilestoneEvent < ApplicationRecord def self.issuable_attrs %i(issue merge_request).freeze end - - def resource - issue || merge_request - end end diff --git a/app/models/resource_weight_event.rb b/app/models/resource_weight_event.rb index ab288798aed..e0cc0c87a83 100644 --- a/app/models/resource_weight_event.rb +++ b/app/models/resource_weight_event.rb @@ -1,26 +1,9 @@ # frozen_string_literal: true -class ResourceWeightEvent < ApplicationRecord - include Gitlab::Utils::StrongMemoize - - validates :user, presence: true +class ResourceWeightEvent < ResourceEvent validates :issue, presence: true - belongs_to :user belongs_to :issue scope :by_issue, ->(issue) { where(issue_id: issue.id) } - scope :created_after, ->(time) { where('created_at > ?', time) } - - def discussion_id(resource = nil) - strong_memoize(:discussion_id) do - Digest::SHA1.hexdigest(discussion_id_key.join("-")) - end - end - - private - - def discussion_id_key - [self.class.name, created_at, user_id] - end end diff --git a/changelogs/unreleased/207126-more-descriptive-error-messages-in-migration-helpers.yml b/changelogs/unreleased/207126-more-descriptive-error-messages-in-migration-helpers.yml new file mode 100644 index 00000000000..36ed216a505 --- /dev/null +++ b/changelogs/unreleased/207126-more-descriptive-error-messages-in-migration-helpers.yml @@ -0,0 +1,5 @@ +--- +title: Improve error messages of failed migrations +merge_request: 25457 +author: +type: changed diff --git a/doc/api/scim.md b/doc/api/scim.md index cdd635d0627..eaa56b0d0dd 100644 --- a/doc/api/scim.md +++ b/doc/api/scim.md @@ -122,7 +122,7 @@ Parameters: | `userName` | string | yes | Username of the user. | | `emails` | JSON string | yes | Work email. | | `name` | JSON string | yes | Name of the user. | -| `meta` | string | no | Resource type (`User'). | +| `meta` | string | no | Resource type (`User`). | Example request: diff --git a/doc/ci/variables/README.md b/doc/ci/variables/README.md index 643ccd45898..c768c833e7c 100644 --- a/doc/ci/variables/README.md +++ b/doc/ci/variables/README.md @@ -571,9 +571,12 @@ Below you can find supported syntax reference: - `$VARIABLE =~ /^content.*/` - `$VARIABLE_1 !~ /^content.*/` (introduced in GitLab 11.11) - It is possible perform pattern matching against a variable and regular - expression. Expression like this evaluates to truth if matches are found - when using `=~`. It evaluates to truth if matches are not found when `!~` is used. + Variable pattern matching with regular expressions uses the + [RE2 regular expression syntax](https://github.com/google/re2/wiki/Syntax). + Expressions evaluate as `true` if: + + - Matches are found when using `=~`. + - Matches are *not* found when using `!~`. Pattern matching is case-sensitive by default. Use `i` flag modifier, like `/pattern/i` to make a pattern case-insensitive. diff --git a/doc/ci/yaml/README.md b/doc/ci/yaml/README.md index 8931ee43a8a..255ae3f7c13 100644 --- a/doc/ci/yaml/README.md +++ b/doc/ci/yaml/README.md @@ -857,7 +857,10 @@ In this example, if the first rule: `rules:if` differs slightly from `only:variables` by accepting only a single expression string, rather than an array of them. Any set of expressions to be -evaluated should be conjoined into a single expression using `&&` or `||`. For example: +evaluated should be conjoined into a single expression using `&&` or `||`, and use +the [variable matching syntax](../variables/README.md#supported-syntax). + +For example: ```yaml job: diff --git a/doc/user/project/deploy_tokens/index.md b/doc/user/project/deploy_tokens/index.md index 728f09ca787..4479653417c 100644 --- a/doc/user/project/deploy_tokens/index.md +++ b/doc/user/project/deploy_tokens/index.md @@ -9,11 +9,11 @@ at midnight UTC and that they can be only managed by [maintainers](../../permiss ## Creating a Deploy Token -You can create as many deploy tokens as you like from the settings of your project: +You can create as many deploy tokens as you like from the settings of your project. Alternatively, you can also create [group-scoped deploy tokens](#group-deploy-token). 1. Log in to your GitLab account. -1. Go to the project you want to create Deploy Tokens for. -1. Go to **Settings** > **Repository**. +1. Go to the project (or group) you want to create Deploy Tokens for. +1. Go to **{settings}** **Settings** > **CI / CD**. 1. Click on "Expand" on **Deploy Tokens** section. 1. Choose a name, expiry date (optional), and username (optional) for the token. 1. Choose the [desired scopes](#limiting-scopes-of-a-deploy-token). @@ -77,6 +77,22 @@ docker login -u <username> -p <deploy_token> registry.example.com Just replace `<username>` and `<deploy_token>` with the proper values. Then you can simply pull images from your Container Registry. +### Group Deploy Token + +> [Introduced](https://gitlab.com/gitlab-org/gitlab/issues/21765) in GitLab 12.9. + +A deploy token created at the group level can be used across all projects that +belong either to the specific group or to one of its subgroups. + +To use a group deploy token: + +1. [Create](#creating-a-deploy-token) a deploy token for a group. +1. Use it the same way you use a project deploy token when + [cloning a repository](#git-clone-a-repository). + +The scopes applied to a group deploy token (such as `read_repository`) will +apply consistently when cloning the repository of related projects. + ### GitLab Deploy Token > [Introduced][ce-18414] in GitLab 10.8. diff --git a/doc/user/search/index.md b/doc/user/search/index.md index 70ab9af0bcc..407578fd4df 100644 --- a/doc/user/search/index.md +++ b/doc/user/search/index.md @@ -41,7 +41,7 @@ groups: - [Label](../project/labels.md) - My-reaction - Confidential - - Epic ([Introduced](https://gitlab.com/gitlab-org/gitlab/issues/195704) in GitLab 12.8) + - Epic ([Introduced](https://gitlab.com/gitlab-org/gitlab/issues/195704) in GitLab 12.9) - Search for this text 1. Select or type the operator to use for filtering the attribute. The following operators are available: diff --git a/lib/gitlab/database/migration_helpers.rb b/lib/gitlab/database/migration_helpers.rb index f75e943671b..82a84508959 100644 --- a/lib/gitlab/database/migration_helpers.rb +++ b/lib/gitlab/database/migration_helpers.rb @@ -215,7 +215,7 @@ module Gitlab fk_name = name || concurrent_foreign_key_name(source, column) unless foreign_key_exists?(source, name: fk_name) - raise "cannot find #{fk_name} on #{source} table" + raise missing_schema_object_message(source, "foreign key", fk_name) end disable_statement_timeout do @@ -931,7 +931,10 @@ module Gitlab def column_for(table, name) name = name.to_s - columns(table).find { |column| column.name == name } + column = columns(table).find { |column| column.name == name } + raise(missing_schema_object_message(table, "column", name)) if column.nil? + + column end # This will replace the first occurrence of a string in a column with @@ -1166,6 +1169,18 @@ into similar problems in the future (e.g. when new tables are created). private + def missing_schema_object_message(table, type, name) + <<~MESSAGE + Could not find #{type} "#{name}" on table "#{table}" which was referenced during the migration. + This issue could be caused by the database schema straying from the expected state. + + To resolve this issue, please verify: + 1. all previous migrations have completed + 2. the database objects used in this migration match the Rails definition in schema.rb or structure.sql + + MESSAGE + end + def tables_match?(target_table, foreign_key_table) target_table.blank? || foreign_key_table == target_table end diff --git a/spec/lib/gitlab/database/migration_helpers_spec.rb b/spec/lib/gitlab/database/migration_helpers_spec.rb index ce6e8c731e2..1fd6157ce43 100644 --- a/spec/lib/gitlab/database/migration_helpers_spec.rb +++ b/spec/lib/gitlab/database/migration_helpers_spec.rb @@ -383,7 +383,8 @@ describe Gitlab::Database::MigrationHelpers do it 'raises an error' do expect(model).to receive(:foreign_key_exists?).and_return(false) - expect { model.validate_foreign_key(:projects, :user_id) }.to raise_error(/cannot find/) + error_message = /Could not find foreign key "fk_name" on table "projects"/ + expect { model.validate_foreign_key(:projects, :user_id, name: :fk_name) }.to raise_error(error_message) end end end @@ -587,6 +588,8 @@ describe Gitlab::Database::MigrationHelpers do end describe '#add_column_with_default' do + let(:column) { Project.columns.find { |c| c.name == "id" } } + context 'outside of a transaction' do context 'when a column limit is not set' do before do @@ -601,6 +604,9 @@ describe Gitlab::Database::MigrationHelpers do expect(model).to receive(:change_column_default) .with(:projects, :foo, 10) + + expect(model).to receive(:column_for) + .with(:projects, :foo).and_return(column) end it 'adds the column while allowing NULL values' do @@ -655,6 +661,7 @@ describe Gitlab::Database::MigrationHelpers do it 'adds the column with a limit' do allow(model).to receive(:transaction_open?).and_return(false) allow(model).to receive(:transaction).and_yield + allow(model).to receive(:column_for).with(:projects, :foo).and_return(column) allow(model).to receive(:update_column_in_batches).with(:projects, :foo, 10) allow(model).to receive(:change_column_null).with(:projects, :foo, false) allow(model).to receive(:change_column_default).with(:projects, :foo, 10) @@ -721,50 +728,68 @@ describe Gitlab::Database::MigrationHelpers do before do allow(model).to receive(:transaction_open?).and_return(false) - allow(model).to receive(:column_for).and_return(old_column) end - it 'renames a column concurrently' do - expect(model).to receive(:check_trigger_permissions!).with(:users) + context 'when the column to rename exists' do + before do + allow(model).to receive(:column_for).and_return(old_column) + end - expect(model).to receive(:install_rename_triggers_for_postgresql) - .with(trigger_name, '"users"', '"old"', '"new"') + it 'renames a column concurrently' do + expect(model).to receive(:check_trigger_permissions!).with(:users) - expect(model).to receive(:add_column) - .with(:users, :new, :integer, - limit: old_column.limit, - precision: old_column.precision, - scale: old_column.scale) + expect(model).to receive(:install_rename_triggers_for_postgresql) + .with(trigger_name, '"users"', '"old"', '"new"') - expect(model).to receive(:change_column_default) - .with(:users, :new, old_column.default) + expect(model).to receive(:add_column) + .with(:users, :new, :integer, + limit: old_column.limit, + precision: old_column.precision, + scale: old_column.scale) - expect(model).to receive(:update_column_in_batches) + expect(model).to receive(:change_column_default) + .with(:users, :new, old_column.default) - expect(model).to receive(:change_column_null).with(:users, :new, false) + expect(model).to receive(:update_column_in_batches) - expect(model).to receive(:copy_indexes).with(:users, :old, :new) - expect(model).to receive(:copy_foreign_keys).with(:users, :old, :new) + expect(model).to receive(:change_column_null).with(:users, :new, false) + + expect(model).to receive(:copy_indexes).with(:users, :old, :new) + expect(model).to receive(:copy_foreign_keys).with(:users, :old, :new) + + model.rename_column_concurrently(:users, :old, :new) + end - model.rename_column_concurrently(:users, :old, :new) + context 'when default is false' do + let(:old_column) do + double(:column, + type: :boolean, + limit: nil, + default: false, + null: false, + precision: nil, + scale: nil) + end + + it 'copies the default to the new column' do + expect(model).to receive(:change_column_default) + .with(:users, :new, old_column.default) + + model.rename_column_concurrently(:users, :old, :new) + end + end end - context 'when default is false' do - let(:old_column) do - double(:column, - type: :boolean, - limit: nil, - default: false, - null: false, - precision: nil, - scale: nil) + context 'when the column to be renamed does not exist' do + before do + allow(model).to receive(:columns).and_return([]) end - it 'copies the default to the new column' do - expect(model).to receive(:change_column_default) - .with(:users, :new, old_column.default) + it 'raises an error with appropriate message' do + expect(model).to receive(:check_trigger_permissions!).with(:users) - model.rename_column_concurrently(:users, :old, :new) + error_message = /Could not find column "missing_column" on table "users"/ + expect { model.rename_column_concurrently(:users, :missing_column, :new) }.to raise_error(error_message) end end end @@ -1133,8 +1158,9 @@ describe Gitlab::Database::MigrationHelpers do expect(column.name).to eq('id') end - it 'returns nil when a column does not exist' do - expect(model.column_for(:users, :kittens)).to be_nil + it 'raises an error when a column does not exist' do + error_message = /Could not find column "kittens" on table "users"/ + expect { model.column_for(:users, :kittens) }.to raise_error(error_message) end end diff --git a/spec/models/resource_label_event_spec.rb b/spec/models/resource_label_event_spec.rb index a92f5ee93e1..ca887b485a2 100644 --- a/spec/models/resource_label_event_spec.rb +++ b/spec/models/resource_label_event_spec.rb @@ -10,6 +10,10 @@ RSpec.describe ResourceLabelEvent, type: :model do it_behaves_like 'having unique enum values' + it_behaves_like 'a resource event' + it_behaves_like 'a resource event for issues' + it_behaves_like 'a resource event for merge requests' + describe 'associations' do it { is_expected.to belong_to(:user) } it { is_expected.to belong_to(:issue) } diff --git a/spec/models/resource_weight_event_spec.rb b/spec/models/resource_weight_event_spec.rb index 2f00204512e..11b633e1dcf 100644 --- a/spec/models/resource_weight_event_spec.rb +++ b/spec/models/resource_weight_event_spec.rb @@ -3,6 +3,9 @@ require 'spec_helper' RSpec.describe ResourceWeightEvent, type: :model do + it_behaves_like 'a resource event' + it_behaves_like 'a resource event for issues' + let_it_be(:user1) { create(:user) } let_it_be(:user2) { create(:user) } @@ -11,13 +14,11 @@ RSpec.describe ResourceWeightEvent, type: :model do let_it_be(:issue3) { create(:issue, author: user2) } describe 'validations' do - it { is_expected.not_to allow_value(nil).for(:user) } it { is_expected.not_to allow_value(nil).for(:issue) } it { is_expected.to allow_value(nil).for(:weight) } end describe 'associations' do - it { is_expected.to belong_to(:user) } it { is_expected.to belong_to(:issue) } end diff --git a/spec/support/shared_examples/resource_events.rb b/spec/support/shared_examples/resource_events.rb index d7e7349ad6c..963453666c9 100644 --- a/spec/support/shared_examples/resource_events.rb +++ b/spec/support/shared_examples/resource_events.rb @@ -10,8 +10,21 @@ shared_examples 'a resource event' do let_it_be(:issue2) { create(:issue, author: user1) } let_it_be(:issue3) { create(:issue, author: user2) } + describe 'importable' do + it { is_expected.to respond_to(:importing?) } + it { is_expected.to respond_to(:imported?) } + end + describe 'validations' do it { is_expected.not_to allow_value(nil).for(:user) } + + context 'when importing' do + before do + allow(subject).to receive(:importing?).and_return(true) + end + + it { is_expected.to allow_value(nil).for(:user) } + end end describe 'associations' do |