Commit 37e4d6d9 authored by Toon Claes's avatar Toon Claes Committed by Mayra Cabrera

DRY up conditions for files require DB review

Stop using two separate lists for the conditions which files require a
database review.

Related discussion:
https://gitlab.com/gitlab-org/gitlab-ce/merge_requests/30156#note_187732053
parent 507dfc66
# frozen_string_literal: true # frozen_string_literal: true
# All the files/directories that should be reviewed by the DB team.
DB_FILES = [
'db/',
'app/models/project_authorization.rb',
'app/services/users/refresh_authorized_projects_service.rb',
'lib/gitlab/background_migration.rb',
'lib/gitlab/background_migration/',
'lib/gitlab/database.rb',
'lib/gitlab/database/',
'lib/gitlab/github_import.rb',
'lib/gitlab/github_import/',
'lib/gitlab/sql/',
'rubocop/cop/migration',
'ee/lib/gitlab/database/'
].freeze
SCHEMA_NOT_UPDATED_MESSAGE = <<~MSG SCHEMA_NOT_UPDATED_MESSAGE = <<~MSG
**New %<migrations>s added but %<schema>s wasn't updated.** **New %<migrations>s added but %<schema>s wasn't updated.**
...@@ -24,20 +8,6 @@ updated too (unless the migration isn't changing the DB schema ...@@ -24,20 +8,6 @@ updated too (unless the migration isn't changing the DB schema
and isn't the most recent one). and isn't the most recent one).
MSG MSG
def database_paths_requiring_review(files)
to_review = []
files.each do |file|
review = DB_FILES.any? do |pattern|
file.start_with?(pattern)
end
to_review << file if review
end
to_review
end
non_geo_db_schema_updated = !git.modified_files.grep(%r{\Adb/schema\.rb}).empty? non_geo_db_schema_updated = !git.modified_files.grep(%r{\Adb/schema\.rb}).empty?
geo_db_schema_updated = !git.modified_files.grep(%r{\Aee/db/geo/schema\.rb}).empty? geo_db_schema_updated = !git.modified_files.grep(%r{\Aee/db/geo/schema\.rb}).empty?
...@@ -52,7 +22,7 @@ if geo_migration_created && !geo_db_schema_updated ...@@ -52,7 +22,7 @@ if geo_migration_created && !geo_db_schema_updated
warn format(SCHEMA_NOT_UPDATED_MESSAGE, migrations: 'Geo migrations', schema: gitlab.html_link("ee/db/geo/schema.rb")) warn format(SCHEMA_NOT_UPDATED_MESSAGE, migrations: 'Geo migrations', schema: gitlab.html_link("ee/db/geo/schema.rb"))
end end
db_paths_to_review = database_paths_requiring_review(helper.all_changed_files) db_paths_to_review = helper.changes_by_category[:database]
unless db_paths_to_review.empty? unless db_paths_to_review.empty?
message 'This merge request adds or changes files that require a ' \ message 'This merge request adds or changes files that require a ' \
......
...@@ -103,6 +103,11 @@ module Gitlab ...@@ -103,6 +103,11 @@ module Gitlab
yarn\.lock yarn\.lock
)\z}x => :frontend, )\z}x => :frontend,
%r{\A(ee/)?db/} => :database,
%r{\A(ee/)?lib/gitlab/(database|background_migration|sql|github_import)(/|\.rb)} => :database,
%r{\A(app/models/project_authorization|app/services/users/refresh_authorized_projects_service)(/|\.rb)} => :database,
%r{\Arubocop/cop/migration(/|\.rb)} => :database,
%r{\A(ee/)?app/(?!assets|views)[^/]+} => :backend, %r{\A(ee/)?app/(?!assets|views)[^/]+} => :backend,
%r{\A(ee/)?(bin|config|danger|generator_templates|lib|rubocop|scripts)/} => :backend, %r{\A(ee/)?(bin|config|danger|generator_templates|lib|rubocop|scripts)/} => :backend,
%r{\A(ee/)?spec/features/} => :test, %r{\A(ee/)?spec/features/} => :test,
...@@ -112,7 +117,6 @@ module Gitlab ...@@ -112,7 +117,6 @@ module Gitlab
%r{\A(Dangerfile|Gemfile|Gemfile.lock|Procfile|Rakefile|\.gitlab-ci\.yml)\z} => :backend, %r{\A(Dangerfile|Gemfile|Gemfile.lock|Procfile|Rakefile|\.gitlab-ci\.yml)\z} => :backend,
%r{\A[A-Z_]+_VERSION\z} => :backend, %r{\A[A-Z_]+_VERSION\z} => :backend,
%r{\A(ee/)?db/} => :database,
%r{\A(ee/)?qa/} => :qa, %r{\A(ee/)?qa/} => :qa,
# Files that don't fit into any category are marked with :none # Files that don't fit into any category are marked with :none
......
...@@ -87,13 +87,13 @@ describe Gitlab::Danger::Helper do ...@@ -87,13 +87,13 @@ describe Gitlab::Danger::Helper do
describe '#changes_by_category' do describe '#changes_by_category' do
it 'categorizes changed files' do it 'categorizes changed files' do
expect(fake_git).to receive(:added_files) { %w[foo foo.md foo.rb foo.js db/foo qa/foo ee/changelogs/foo.yml] } expect(fake_git).to receive(:added_files) { %w[foo foo.md foo.rb foo.js db/foo lib/gitlab/database/foo.rb qa/foo ee/changelogs/foo.yml] }
allow(fake_git).to receive(:modified_files) { [] } allow(fake_git).to receive(:modified_files) { [] }
allow(fake_git).to receive(:renamed_files) { [] } allow(fake_git).to receive(:renamed_files) { [] }
expect(helper.changes_by_category).to eq( expect(helper.changes_by_category).to eq(
backend: %w[foo.rb], backend: %w[foo.rb],
database: %w[db/foo], database: %w[db/foo lib/gitlab/database/foo.rb],
frontend: %w[foo.js], frontend: %w[foo.js],
none: %w[ee/changelogs/foo.yml foo.md], none: %w[ee/changelogs/foo.yml foo.md],
qa: %w[qa/foo], qa: %w[qa/foo],
...@@ -159,9 +159,22 @@ describe Gitlab::Danger::Helper do ...@@ -159,9 +159,22 @@ describe Gitlab::Danger::Helper do
'ee/FOO_VERSION' | :unknown 'ee/FOO_VERSION' | :unknown
'db/foo' | :database 'db/foo' | :database
'qa/foo' | :qa 'ee/db/foo' | :database
'app/models/project_authorization.rb' | :database
'app/services/users/refresh_authorized_projects_service.rb' | :database
'lib/gitlab/background_migration.rb' | :database
'lib/gitlab/background_migration/foo' | :database
'ee/lib/gitlab/background_migration/foo' | :database
'lib/gitlab/database.rb' | :database
'lib/gitlab/database/foo' | :database
'ee/lib/gitlab/database/foo' | :database
'lib/gitlab/github_import.rb' | :database
'lib/gitlab/github_import/foo' | :database
'lib/gitlab/sql/foo' | :database
'rubocop/cop/migration/foo' | :database
'qa/foo' | :qa
'ee/qa/foo' | :qa 'ee/qa/foo' | :qa
'changelogs/foo' | :none 'changelogs/foo' | :none
......
Markdown is supported
0%
or
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment