From eedc4a1c1bcecd3e676f4f03eb1dcd1f72586bfd Mon Sep 17 00:00:00 2001
From: Matt Kasa <mkasa@gitlab.com>
Date: Thu, 20 Jul 2023 12:02:08 -0700
Subject: [PATCH] Detect model scope changes as database changes in Danger
Relates to https://gitlab.com/gitlab-org/gitlab/-/issues/419001
---
danger/database/Dangerfile | 4 +-
spec/tooling/danger/database_spec.rb | 90 +++++++++++++++++++++++++++-
tooling/danger/database.rb | 16 +++++
3 files changed, 106 insertions(+), 4 deletions(-)
diff --git a/danger/database/Dangerfile b/danger/database/Dangerfile
index 1c0e9d721180..6861a04ffc6c 100644
--- a/danger/database/Dangerfile
+++ b/danger/database/Dangerfile
@@ -67,9 +67,7 @@ end
return unless helper.ci?
return if helper.mr_labels.include?(DATABASE_APPROVED_LABEL)
-db_paths_to_review = helper.changes_by_category[:database]
-
-if helper.mr_labels.include?('database') || db_paths_to_review.any?
+if helper.mr_labels.include?('database') || database.changes.any?
message 'This merge request adds or changes files that require a ' \
'review from the [Database team](https://gitlab.com/groups/gl-database/-/group_members).'
diff --git a/spec/tooling/danger/database_spec.rb b/spec/tooling/danger/database_spec.rb
index ddcfa279dc3c..a342014cf6bf 100644
--- a/spec/tooling/danger/database_spec.rb
+++ b/spec/tooling/danger/database_spec.rb
@@ -1,5 +1,6 @@
# frozen_string_literal: true
+require 'rspec-parameterized'
require 'gitlab-dangerfiles'
require 'danger'
require 'danger/plugins/internal/helper'
@@ -41,11 +42,98 @@
let(:cutoff) { Date.parse('2022-10-01') - 21 }
- subject(:database) { fake_danger.new }
+ subject(:database) { fake_danger.new(helper: fake_helper) }
describe '#find_migration_files_before' do
it 'returns migrations that are before the cutoff' do
expect(database.find_migration_files_before(migration_files, cutoff).length).to eq(8)
end
end
+
+ describe '#changes' do
+ using RSpec::Parameterized::TableSyntax
+
+ where do
+ {
+ 'with database changes to a migration file' => {
+ modified_files: %w[
+ db/migrate/20230720114001_test_migration.rb
+ db/schema_migrations/20230720114001
+ db/structure.sql
+ app/models/test.rb
+ ],
+ changed_lines: [],
+ changes_by_category: {
+ database: %w[
+ db/migrate/20230720114001_test_migration.rb
+ db/schema_migrations/20230720114001
+ db/structure.sql
+ ]
+ },
+ impacted_files: %w[
+ db/migrate/20230720114001_test_migration.rb
+ db/schema_migrations/20230720114001
+ db/structure.sql
+ ]
+ },
+ 'with non-database changes' => {
+ modified_files: %w[
+ app/models/test.rb
+ ],
+ changed_lines: %w[
+ +# Comment explaining scope :blah
+ ],
+ changes_by_category: {
+ database: []
+ },
+ impacted_files: []
+ },
+ 'with database changes in a doc' => {
+ modified_files: %w[doc/development/database/test.md],
+ changed_lines: [
+ '+scope :blah, ->() { where(hidden: false) }'
+ ],
+ changes_by_category: {
+ database: []
+ },
+ impacted_files: []
+ },
+ 'with database changes in a model' => {
+ modified_files: %w[app/models/test.rb],
+ changed_lines: [
+ '+# Comment explaining scope :blah',
+ '+scope :blah, ->() { where(hidden: false) }'
+ ],
+ changes_by_category: {
+ database: []
+ },
+ impacted_files: %w[app/models/test.rb]
+ },
+ 'with database changes in a concern' => {
+ modified_files: %w[app/models/concerns/test.rb],
+ changed_lines: [
+ '- .where(hidden: false)',
+ '+ .where(hidden: true)'
+ ],
+ changes_by_category: {
+ database: []
+ },
+ impacted_files: %w[app/models/concerns/test.rb]
+ }
+ }
+ end
+
+ with_them do
+ before do
+ allow(fake_helper).to receive(:modified_files).and_return(modified_files)
+ allow(fake_helper).to receive(:all_changed_files).and_return(modified_files)
+ allow(fake_helper).to receive(:changed_lines).and_return(changed_lines)
+ allow(fake_helper).to receive(:changes_by_category).and_return(changes_by_category)
+ end
+
+ it 'returns database changes' do
+ expect(database.changes).to match impacted_files
+ end
+ end
+ end
end
diff --git a/tooling/danger/database.rb b/tooling/danger/database.rb
index 4cfac7c4af44..71b3ed1a1a5d 100644
--- a/tooling/danger/database.rb
+++ b/tooling/danger/database.rb
@@ -5,6 +5,8 @@ module Danger
module Database
TIMESTAMP_MATCHER = /(?<timestamp>\d{14})/
MIGRATION_MATCHER = %r{\A(ee/)?db/(geo/)?(post_)?migrate/}
+ MODEL_PATHS = %r{\A(ee/)?app/models/}
+ MODEL_CHANGES = %r{^[^#\n]*?(?:scope :|where\(|joins\()}
def find_migration_files_before(file_names, cutoff)
migrations = file_names.select { |f| f.match?(MIGRATION_MATCHER) }
@@ -15,6 +17,20 @@ def find_migration_files_before(file_names, cutoff)
timestamp < cutoff
end
end
+
+ def changes
+ changed_database_paths + changed_model_paths
+ end
+
+ def changed_database_paths
+ helper.changes_by_category[:database]
+ end
+
+ def changed_model_paths
+ helper.all_changed_files.grep(MODEL_PATHS).select do |file|
+ helper.changed_lines(file).any? { |change| change =~ MODEL_CHANGES }
+ end
+ end
end
end
end
--
GitLab