Merge branch...

Merge branch '299291-validated-uniqueness-of-the-key_path-on-usage-metric-definition-generator' into 'master'

Validate uniqueness of key_path in usage metric definition generator

See merge request gitlab-org/gitlab!55236

lib/generators/gitlab/usage_metric_definition_generator.rb

@@ -74,6 +74,7 @@ module Gitlab
     def validate!
       raise "--dir option is required" unless input_dir.present?
       raise "Invalid dir #{input_dir}, allowed options are #{VALID_INPUT_DIRS.join(', ')}" unless directory.present?
+      raise "Metric definition with key path '#{key_path}' already exists" if metric_definition_exists?
     end
 
     def ee?
@@ -88,11 +89,23 @@ module Gitlab
     #
     # 20210201124931_g_project_management_issue_title_changed_weekly.yml
     def file_name
-      "#{Time.now.utc.strftime("%Y%m%d%H%M%S")}_#{key_path.split('.').last}"
+      "#{Time.now.utc.strftime("%Y%m%d%H%M%S")}_#{metric_name}"
     end
 
     def directory
       @directory ||= TIME_FRAME_DIRS.find { |d| d.match?(input_dir) }
     end
+
+    def metric_name
+      key_path.split('.').last
+    end
+
+    def metric_definitions
+      @definitions ||= Gitlab::Usage::MetricDefinition.definitions
+    end
+
+    def metric_definition_exists?
+      metric_definitions[key_path].present?
+    end
   end
 end

spec/generator_helper.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+RSpec.configure do |config|
+  # Redirect stdout so specs don't have so much noise
+  config.before(:all) do
+    $stdout = StringIO.new
+  end
+
+  # Reset stdout
+  config.after(:all) do
+    $stdout = STDOUT
+  end
+end

spec/lib/generators/gitlab/usage_metric_definition_generator_spec.rb

 # frozen_string_literal: true
 
-require 'spec_helper'
-require 'rails/generators/testing/behaviour'
+require 'generator_helper'
 
 RSpec.describe Gitlab::UsageMetricDefinitionGenerator do
-  let(:temp_dir) { Dir.mktmpdir }
+  describe 'Validation' do
+    let(:key_path) { 'counter.category.event' }
+    let(:dir) { '7d' }
+    let(:options) { [key_path, '--dir', dir, '--pretend'] }
 
-  before do
-    stub_const("#{described_class}::TOP_LEVEL_DIR", temp_dir)
-  end
+    subject { described_class.start(options) }
+
+    it 'does not raise an error' do
+      expect { subject }.not_to raise_error
+    end
+
+    context 'with a missing directory' do
+      let(:options) { [key_path, '--pretend'] }
 
-  context 'with product_intelligence_metrics_names_suggestions feature ON' do
-    it 'adds name key to metric definition' do
-      stub_feature_flags(product_intelligence_metrics_names_suggestions: true)
+      it 'raises an error' do
+        expect { subject }.to raise_error(RuntimeError)
+      end
+    end
+
+    context 'with an invalid directory' do
+      let(:dir) { '8d' }
+
+      it 'raises an error' do
+        expect { subject }.to raise_error(RuntimeError)
+      end
+    end
 
-      expect(::Gitlab::Usage::Metrics::NamesSuggestions::Generator).to receive(:generate).and_return('some name')
-      described_class.new(['counts_weekly.test_metric'], { 'dir' => '7d' }).invoke_all
-      metric_definition_path = Dir.glob(File.join(temp_dir, 'metrics/counts_7d/*_test_metric.yml')).first
+    context 'with an already existing metric with the same key_path' do
+      before do
+        allow(Gitlab::Usage::MetricDefinition).to receive(:definitions).and_return(Hash[key_path, 'definition'])
+      end
 
-      expect(YAML.safe_load(File.read(metric_definition_path))).to include("name" => "some name")
+      it 'raises an error' do
+        expect { subject }.to raise_error(RuntimeError)
+      end
     end
   end
 
-  context 'with product_intelligence_metrics_names_suggestions feature OFF' do
-    it 'adds name key to metric definition' do
-      stub_feature_flags(product_intelligence_metrics_names_suggestions: false)
+  describe 'Name suggestions' do
+    let(:temp_dir) { Dir.mktmpdir }
+
+    before do
+      stub_const("#{described_class}::TOP_LEVEL_DIR", temp_dir)
+    end
+
+    context 'with product_intelligence_metrics_names_suggestions feature ON' do
+      it 'adds name key to metric definition' do
+        stub_feature_flags(product_intelligence_metrics_names_suggestions: true)
+
+        expect(::Gitlab::Usage::Metrics::NamesSuggestions::Generator).to receive(:generate).and_return('some name')
+        described_class.new(['counts_weekly.test_metric'], { 'dir' => '7d' }).invoke_all
+        metric_definition_path = Dir.glob(File.join(temp_dir, 'metrics/counts_7d/*_test_metric.yml')).first
+
+        expect(YAML.safe_load(File.read(metric_definition_path))).to include("name" => "some name")
+      end
+    end
+
+    context 'with product_intelligence_metrics_names_suggestions feature OFF' do
+      it 'adds name key to metric definition' do
+        stub_feature_flags(product_intelligence_metrics_names_suggestions: false)
 
-      expect(::Gitlab::Usage::Metrics::NamesSuggestions::Generator).not_to receive(:generate)
-      described_class.new(['counts_weekly.test_metric'], { 'dir' => '7d' }).invoke_all
-      metric_definition_path = Dir.glob(File.join(temp_dir, 'metrics/counts_7d/*_test_metric.yml')).first
+        expect(::Gitlab::Usage::Metrics::NamesSuggestions::Generator).not_to receive(:generate)
+        described_class.new(['counts_weekly.test_metric'], { 'dir' => '7d' }).invoke_all
+        metric_definition_path = Dir.glob(File.join(temp_dir, 'metrics/counts_7d/*_test_metric.yml')).first
 
-      expect(YAML.safe_load(File.read(metric_definition_path)).keys).not_to include(:name)
+        expect(YAML.safe_load(File.read(metric_definition_path)).keys).not_to include(:name)
+      end
     end
   end
 end