Skip to content

G
gitlab-ce
Commit 83143150 authored by Niko Belokolodov's avatar Niko Belokolodov
Browse files
Options

Move PI Danger code to project_helper class 

Instead of using custom file matching logic from ProductIntelligence helper we utilise
standard one from project_helper.
This lets us keep all file name/diff matching in one place.
parent 438c3b27
Hide whitespace changes
Inline Side-by-side
Showing with 21 additions and 128 deletions
danger/product_intelligence/Dangerfile
View file @ 83143150
......@@ -12,12 +12,12 @@ For MR review guidelines, see the [Service Ping review guidelines](https://docs.
MSG
# exit if not matching files or if no product intelligence labels
matching_changed_files = product_intelligence.matching_changed_files
product_intelligence_paths_to_review = project_helper.changes_by_category[:product_intelligence]
labels = product_intelligence.missing_labels
return if matching_changed_files.empty? || labels.empty?
return if product_intelligence_paths_to_review.empty? || labels.empty?
warn format(CHANGED_FILES_MESSAGE, changed_files: helper.markdown_list(matching_changed_files))
warn format(CHANGED_FILES_MESSAGE, changed_files: helper.markdown_list(product_intelligence_paths_to_review))
gitlab.api.update_merge_request(gitlab.mr_json['project_id'],
gitlab.mr_json['iid'],
......
spec/tooling/danger/product_intelligence_spec.rb
View file @ 83143150
......@@ -68,71 +68,4 @@ RSpec.describe Tooling::Danger::ProductIntelligence do
it { is_expected.to be_empty }
end
end
describe '#matching_changed_files' do
subject { product_intelligence.matching_changed_files }
let(:changed_files) do
[
'dashboard/todos_controller.rb',
'components/welcome.vue',
'admin/groups/_form.html.haml'
]
end
context 'with snowplow files changed' do
context 'when vue file changed' do
let(:changed_lines) { ['+data-track-action'] }
it { is_expected.to match_array(['components/welcome.vue']) }
end
context 'when haml file changed' do
let(:changed_lines) { ['+ data: { track_label:'] }
it { is_expected.to match_array(['admin/groups/_form.html.haml']) }
end
context 'when ruby file changed' do
let(:changed_lines) { ['+ Gitlab::Tracking.event'] }
let(:changed_files) { ['dashboard/todos_controller.rb', 'admin/groups/_form.html.haml'] }
it { is_expected.to match_array(['dashboard/todos_controller.rb']) }
end
end
context 'with metrics files changed' do
let(:changed_files) { ['config/metrics/counts_7d/test_metric.yml', 'ee/config/metrics/counts_7d/ee_metric.yml'] }
it { is_expected.to match_array(changed_files) }
end
context 'with metrics files not changed' do
it { is_expected.to be_empty }
end
context 'with tracking files changed' do
let(:changed_files) do
[
'lib/gitlab/tracking.rb',
'spec/lib/gitlab/tracking_spec.rb',
'app/helpers/tracking_helper.rb'
]
end
it { is_expected.to match_array(changed_files) }
end
context 'with usage_data files changed' do
let(:changed_files) do
[
'doc/api/usage_data.md',
'ee/lib/ee/gitlab/usage_data.rb',
'spec/lib/gitlab/usage_data_spec.rb'
]
end
it { is_expected.to match_array(changed_files) }
end
end
end
spec/tooling/danger/project_helper_spec.rb
View file @ 83143150
......@@ -40,7 +40,7 @@ RSpec.describe Tooling::Danger::ProjectHelper do
using RSpec::Parameterized::TableSyntax
before do
allow(fake_git).to receive(:diff_for_file).with('usage_data.rb') { double(:diff, patch: "+ count(User.active)") }
allow(fake_git).to receive(:diff_for_file).with(instance_of(String)) { double(:diff, patch: "+ count(User.active)") }
end
where(:path, :expected_categories) do
......@@ -189,6 +189,10 @@ RSpec.describe Tooling::Danger::ProjectHelper do
'spec/frontend/tracking/foo.js' | [:frontend, :product_intelligence]
'spec/frontend/tracking_spec.js' | [:frontend, :product_intelligence]
'lib/gitlab/usage_database/foo.rb' | [:backend]
'config/metrics/counts_7d/test_metric.yml' | [:product_intelligence]
'config/metrics/schema.json' | [:product_intelligence]
'doc/api/usage_data.md' | [:product_intelligence]
'spec/lib/gitlab/usage_data_spec.rb' | [:product_intelligence]
end
with_them do
......@@ -199,6 +203,9 @@ RSpec.describe Tooling::Danger::ProjectHelper do
context 'having specific changes' do
where(:expected_categories, :patch, :changed_files) do
[:product_intelligence] | '+data-track-action' | ['components/welcome.vue']
[:product_intelligence] | '+ data: { track_label:' | ['admin/groups/_form.html.haml']
[:product_intelligence] | '+ Gitlab::Tracking.event' | ['dashboard/todos_controller.rb', 'admin/groups/_form.html.haml']
[:database, :backend, :product_intelligence] | '+ count(User.active)' | ['usage_data.rb', 'lib/gitlab/usage_data.rb', 'ee/lib/ee/gitlab/usage_data.rb']
[:database, :backend, :product_intelligence] | '+ estimate_batch_distinct_count(User.active)' | ['usage_data.rb']
[:backend, :product_intelligence] | '+ alt_usage_data(User.active)' | ['lib/gitlab/usage_data.rb']
......
tooling/danger/product_intelligence.rb
View file @ 83143150
......@@ -9,26 +9,6 @@ module Tooling
'product intelligence::review pending'
].freeze
TRACKING_FILES = [
'lib/gitlab/tracking.rb',
'spec/lib/gitlab/tracking_spec.rb',
'app/helpers/tracking_helper.rb',
'spec/helpers/tracking_helper_spec.rb',
'app/assets/javascripts/tracking/index.js',
'app/assets/javascripts/tracking/constants.js',
'app/assets/javascripts/tracking/get_standard_context.js',
'spec/frontend/tracking/get_standard_context_spec.js',
'spec/frontend/tracking_spec.js',
'generator_templates/usage_metric_definition/metric_definition.yml',
'lib/generators/gitlab/usage_metric/usage_metric_generator.rb',
'lib/generators/gitlab/usage_metric_definition_generator.rb',
'lib/generators/gitlab/usage_metric_definition/redis_hll_generator.rb',
'spec/lib/generators/gitlab/usage_metric_generator_spec.rb',
'spec/lib/generators/gitlab/usage_metric_definition_generator_spec.rb',
'spec/lib/generators/gitlab/usage_metric_definition/redis_hll_generator_spec.rb',
'config/metrics/schema.json'
].freeze
def missing_labels
return [] if !helper.ci? || helper.mr_has_labels?('growth experiment')
......@@ -38,43 +18,6 @@ module Tooling
labels
end
def matching_changed_files
tracking_changed_files = all_changed_files & TRACKING_FILES
usage_data_changed_files = all_changed_files.grep(%r{(usage_data)})
usage_data_changed_files + tracking_changed_files + metrics_changed_files + snowplow_changed_files
end
private
def all_changed_files
helper.all_changed_files
end
def metrics_changed_files
all_changed_files.grep(%r{((ee/)?config/metrics/.*\.yml)})
end
def matching_files?(file, extension:, pattern:)
return unless file.end_with?(extension)
helper.changed_lines(file).grep(pattern).any?
end
def snowplow_changed_files
js_patterns = Regexp.union(
'Tracking.event',
/\btrack\(/,
'data-track-action'
)
all_changed_files.select do |file|
matching_files?(file, extension: '.rb', pattern: %r{Gitlab::Tracking\.(event|enabled\?|options)$}) ||
matching_files?(file, extension: '.js', pattern: js_patterns) ||
matching_files?(file, extension: '.vue', pattern: js_patterns) ||
matching_files?(file, extension: '.haml', pattern: %r{data: \{ track})
end
end
end
end
end
tooling/danger/project_helper.rb
View file @ 83143150
......@@ -38,6 +38,8 @@ module Tooling
%r{\A((ee|jh)/)?config/feature_flags/} => :feature_flag,
%r{doc/api/usage_data.md} => [:product_intelligence],
%r{\Adoc/.*(\.(md|png|gif|jpg|yml))\z} => :docs,
%r{\A(CONTRIBUTING|LICENSE|MAINTENANCE|PHILOSOPHY|PROCESS|README)(\.md)?\z} => :docs,
%r{\Adata/whats_new/} => :docs,
......@@ -100,6 +102,7 @@ module Tooling
%r{\A((ee|jh)/)?spec/support/shared_contexts/features/} => :test,
%r{\A((ee|jh)/)?spec/support/helpers/features/} => :test,
%r{\A((spec/)?lib/generators/gitlab/usage_metric_)} => [:product_intelligence],
%r{\A((ee|jh)/)?lib/gitlab/usage_data_counters/.*\.yml\z} => [:product_intelligence],
%r{\A((ee|jh)/)?config/metrics/((.*\.yml)|(schema\.json))\z} => [:product_intelligence],
%r{\A((ee|jh)/)?lib/gitlab/usage_data(_counters)?(/|\.rb)} => [:backend, :product_intelligence],
......@@ -108,9 +111,16 @@ module Tooling
spec/lib/gitlab/tracking_spec\.rb |
app/helpers/tracking_helper\.rb |
spec/helpers/tracking_helper_spec\.rb |
(spec/)?lib/generators/gitlab/usage_metric_\S+ |
(spec/)?lib/generators/gitlab/usage_metric_definition/redis_hll_generator(_spec)?\.rb |
lib/generators/rails/usage_metric_definition_generator\.rb |
spec/lib/generators/usage_metric_definition_generator_spec\.rb |
generator_templates/usage_metric_definition/metric_definition\.yml)\z}x => [:backend, :product_intelligence],
%r{gitlab/usage_data(_spec)?\.rb} => [:product_intelligence],
[%r{\.haml\z}, %r{data: \{ track}] => [:product_intelligence],
[%r{\.(rb|haml)\z}, %r{Gitlab::Tracking\.(event|enabled\?|options)$}] => [:product_intelligence],
[%r{\.(vue|js)\z}, %r{(Tracking.event|/\btrack\(/|data-track-action)}] => [:product_intelligence],
%r{\A((ee|jh)/)?app/(?!assets|views)[^/]+} => :backend,
%r{\A((ee|jh)/)?(bin|config|generator_templates|lib|rubocop)/} => :backend,
%r{\A((ee|jh)/)?spec/migrations} => :database,
......
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
GitLab Nexedi Edition | About GitLab | About Nexedi | 沪ICP备2021021310号-2 | 沪ICP备2021021310号-7