Merge branch '66596-allow-danger-to-be-run-locally-2' into 'master'

Break up Danger rules into local or CI only

See merge request gitlab-org/gitlab-ce!32747

Dangerfile

 # frozen_string_literal: true
+
+require_relative 'lib/gitlab_danger'
+
 danger.import_plugin('danger/plugins/helper.rb')
 danger.import_plugin('danger/plugins/roulette.rb')
 unless helper.release_automation?
-  danger.import_dangerfile(path: 'danger/metadata')
-  danger.import_dangerfile(path: 'danger/changes_size')
-  danger.import_dangerfile(path: 'danger/changelog')
-  danger.import_dangerfile(path: 'danger/specs')
-  danger.import_dangerfile(path: 'danger/gemfile')
-  danger.import_dangerfile(path: 'danger/database')
-  danger.import_dangerfile(path: 'danger/documentation')
-  danger.import_dangerfile(path: 'danger/frozen_string')
-  danger.import_dangerfile(path: 'danger/commit_messages')
-  danger.import_dangerfile(path: 'danger/duplicate_yarn_dependencies')
-  danger.import_dangerfile(path: 'danger/prettier')
-  danger.import_dangerfile(path: 'danger/eslint')
-  danger.import_dangerfile(path: 'danger/roulette')
-  danger.import_dangerfile(path: 'danger/single_codebase')
-  danger.import_dangerfile(path: 'danger/gitlab_ui_wg')
-  danger.import_dangerfile(path: 'danger/ce_ee_vue_templates')
-  danger.import_dangerfile(path: 'danger/only_documentation')
+  GitlabDanger.new(helper.gitlab_helper).rule_names.each do |file|
+    danger.import_dangerfile(path: File.join('danger', file))
+  end
 end

Gemfile

@@ -318,6 +318,7 @@ end
 group :development do
   gem 'foreman', '~> 0.84.0'
   gem 'brakeman', '~> 4.2', require: false
+  gem 'danger', '~> 6.0', require: false
   gem 'letter_opener_web', '~> 1.3.4'
   gem 'rblineprof', '~> 0.3.6', platform: :mri, require: false

Gemfile.lock

@@ -140,9 +140,15 @@ GEM
       numerizer (~> 0.1.1)
     chunky_png (1.3.5)
     citrus (3.0.2)
+    claide (1.0.3)
+    claide-plugins (0.9.2)
+      cork
+      nap
+      open4 (~> 1.3)
     coderay (1.1.2)
     coercible (1.0.0)
       descendants_tracker (~> 0.0.1)
+    colored2 (3.1.2)
     commonmarker (0.17.13)
       ruby-enum (~> 0.5)
     concord (0.1.5)
@@ -151,6 +157,8 @@ GEM
     concurrent-ruby (1.1.5)
     connection_pool (2.2.2)
     contracts (0.11.0)
+    cork (0.3.0)
+      colored2 (~> 3.1)
     crack (0.4.3)
       safe_yaml (~> 1.0.0)
     crass (1.0.4)
@@ -158,6 +166,19 @@ GEM
     css_parser (1.5.0)
       addressable
     daemons (1.2.6)
+    danger (6.0.9)
+      claide (~> 1.0)
+      claide-plugins (>= 0.9.2)
+      colored2 (~> 3.1)
+      cork (~> 0.1)
+      faraday (~> 0.9)
+      faraday-http-cache (~> 2.0)
+      git (~> 1.5)
+      kramdown (~> 2.0)
+      kramdown-parser-gfm (~> 1.0)
+      no_proxy_fix
+      octokit (~> 4.7)
+      terminal-table (~> 1)
     database_cleaner (1.7.0)
     debug_inspector (0.0.3)
     debugger-ruby_core_source (1.3.8)
@@ -227,6 +248,8 @@ GEM
       railties (>= 3.0.0)
     faraday (0.12.2)
       multipart-post (>= 1.2, < 3)
+    faraday-http-cache (2.0.0)
+      faraday (~> 0.8)
     faraday_middleware (0.12.2)
       faraday (>= 0.7.4, < 1.0)
     faraday_middleware-multi_json (0.0.6)
@@ -308,6 +331,7 @@ GEM
       gettext_i18n_rails (>= 0.7.1)
       po_to_json (>= 1.0.0)
       rails (>= 3.2.0)
+    git (1.5.0)
     gitaly (1.58.0)
       grpc (~> 1.0)
     github-markup (1.7.0)
@@ -478,6 +502,9 @@ GEM
     kgio (2.11.2)
     knapsack (1.17.0)
       rake
+    kramdown (2.1.0)
+    kramdown-parser-gfm (1.1.0)
+      kramdown (~> 2.0)
     kubeclient (4.2.2)
       http (~> 3.0)
       recursive-open-struct (~> 1.0, >= 1.0.4)
@@ -535,10 +562,12 @@ GEM
     mustermann-grape (1.0.0)
       mustermann (~> 1.0.0)
     nakayoshi_fork (0.0.4)
+    nap (1.1.0)
     net-ldap (0.16.0)
     net-ssh (5.2.0)
     netrc (0.11.0)
     nio4r (2.3.1)
+    no_proxy_fix (0.1.2)
     nokogiri (1.10.4)
       mini_portile2 (~> 2.4.0)
     nokogumbo (1.5.0)
@@ -615,6 +644,7 @@ GEM
       addressable (~> 2.5)
       omniauth (~> 1.3)
       openid_connect (~> 1.1)
+    open4 (1.3.4)
     openid_connect (1.1.6)
       activemodel
       attr_required (>= 1.0.0)
@@ -926,6 +956,8 @@ GEM
       ffi
     sysexits (1.2.0)
     temple (0.8.0)
+    terminal-table (1.8.0)
+      unicode-display_width (~> 1.1, >= 1.1.1)
     test-prof (0.2.5)
     text (1.3.1)
     thin (1.7.2)
@@ -1055,6 +1087,7 @@ DEPENDENCIES
   concurrent-ruby (~> 1.1)
   connection_pool (~> 2.0)
   creole (~> 0.5.0)
+  danger (~> 6.0)
   database_cleaner (~> 1.7.0)
   deckar01-task_list (= 2.2.0)
   default_value_for (~> 3.2.0)

danger/database/Dangerfile

 # frozen_string_literal: true
-SCHEMA_NOT_UPDATED_MESSAGE = <<~MSG
-**New %<migrations>s added but %<schema>s wasn't updated.**
+gitlab_danger = GitlabDanger.new(helper.gitlab_helper)
+
+SCHEMA_NOT_UPDATED_MESSAGE_SHORT = <<~MSG
+New %<migrations>s added but %<schema>s wasn't updated.
+MSG
+
+SCHEMA_NOT_UPDATED_MESSAGE_FULL = <<~MSG
+**#{SCHEMA_NOT_UPDATED_MESSAGE_SHORT}**
 Usually, when adding new %<migrations>s, %<schema>s should be
 updated too (unless the migration isn't changing the DB schema
@@ -29,14 +35,18 @@ geo_db_schema_updated = !git.modified_files.grep(%r{\Aee/db/geo/schema\.rb}).emp
 non_geo_migration_created = !git.added_files.grep(%r{\A(db/(post_)?migrate)/}).empty?
 geo_migration_created = !git.added_files.grep(%r{\Aee/db/geo/(post_)?migrate/}).empty?
+format_str = gitlab_danger.ci? ? SCHEMA_NOT_UPDATED_MESSAGE_FULL : SCHEMA_NOT_UPDATED_MESSAGE_SHORT
+
 if non_geo_migration_created && !non_geo_db_schema_updated
-  warn format(SCHEMA_NOT_UPDATED_MESSAGE, migrations: 'migrations', schema: gitlab.html_link("db/schema.rb"))
+  warn format(format_str, migrations: 'migrations', schema: gitlab_danger.html_link("db/schema.rb"))
 end
 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(format_str, migrations: 'Geo migrations', schema: gitlab_danger.html_link("ee/db/geo/schema.rb"))
 end
+return unless gitlab_danger.ci?
+
 db_paths_to_review = helper.changes_by_category[:database]
 if gitlab.mr_labels.include?('database') || db_paths_to_review.any?

danger/documentation/Dangerfile

@@ -6,20 +6,22 @@ unless docs_paths_to_review.empty?
   message 'This merge request adds or changes files that require a review ' \
     'from the Technical Writing team.'
-  markdown(<<~MARKDOWN)
-## Documentation review
-The following files require a review from a technical writer:
-* #{docs_paths_to_review.map { |path| "`#{path}`" }.join("\n* ")}
-The review does not need to block merging this merge request. See the:
-- [DevOps stages](https://about.gitlab.com/handbook/product/categories/#devops-stages) for the appropriate technical writer for this review.
-- [Documentation workflows](https://docs.gitlab.com/ee/development/documentation/workflow.html) for information on when to assign a merge request for review.
-  MARKDOWN
-  unless gitlab.mr_labels.include?('Documentation')
-    warn 'This merge request is missing the ~Documentation label.'
+  if GitlabDanger.new(helper.gitlab_helper).ci?
+    markdown(<<~MARKDOWN)
+      ## Documentation review
+      The following files require a review from a technical writer:
+      * #{docs_paths_to_review.map { |path| "`#{path}`" }.join("\n* ")}
+      The review does not need to block merging this merge request. See the:
+      - [DevOps stages](https://about.gitlab.com/handbook/product/categories/#devops-stages) for the appropriate technical writer for this review.
+      - [Documentation workflows](https://docs.gitlab.com/ee/development/documentation/workflow.html) for information on when to assign a merge request for review.
+    MARKDOWN
+    unless gitlab.mr_labels.include?('Documentation')
+      warn 'This merge request is missing the ~Documentation label.'
+    end
   end
 end

danger/duplicate_yarn_dependencies/Dangerfile

 # frozen_string_literal: true
-return unless helper.all_changed_files.include? 'yarn.lock'
+return unless helper.all_changed_files.include?('yarn.lock')
 duplicate = `node_modules/.bin/yarn-deduplicate --list --strategy fewer yarn.lock`
              .split(/$/)
@@ -11,17 +11,19 @@ return if duplicate.empty?
 warn 'This merge request has introduced duplicated yarn dependencies.'
-markdown(<<~MARKDOWN)
-  ## Duplicate yarn dependencies
-  The following dependencies should be de-duplicated:
-  * #{duplicate.map { |path| "`#{path}`" }.join("\n* ")}
-  Please run the following command and commit the changes to `yarn.lock`:
-  ```
-  node_modules/.bin/yarn-deduplicate --strategy fewer yarn.lock \\
-    && yarn install
-  ```
-MARKDOWN
+if GitlabDanger.new(helper.gitlab_helper).ci?
+  markdown(<<~MARKDOWN)
+    ## Duplicate yarn dependencies
+    The following dependencies should be de-duplicated:
+    * #{duplicate.map { |path| "`#{path}`" }.join("\n* ")}
+    Please run the following command and commit the changes to `yarn.lock`:
+    ```
+    node_modules/.bin/yarn-deduplicate --strategy fewer yarn.lock \\
+      && yarn install
+    ```
+  MARKDOWN
+end

danger/eslint/Dangerfile

@@ -13,17 +13,19 @@ return if eslint_candidates.empty?
 warn 'This merge request changed files with disabled eslint rules. Please consider fixing them.'
-markdown(<<~MARKDOWN)
-  ## Disabled eslint rules
-  The following files have disabled `eslint` rules. Please consider fixing them:
-  * #{eslint_candidates.map { |path| "`#{path}`" }.join("\n* ")}
-  Run the following command for more details
-  ```
-  node_modules/.bin/eslint --report-unused-disable-directives --no-inline-config \\
-  #{eslint_candidates.map { |path| "  '#{path}'" }.join(" \\\n")}
-  ```
-MARKDOWN
+if GitlabDanger.new(helper.gitlab_helper).ci?
+  markdown(<<~MARKDOWN)
+    ## Disabled eslint rules
+    The following files have disabled `eslint` rules. Please consider fixing them:
+    * #{eslint_candidates.map { |path| "`#{path}`" }.join("\n* ")}
+    Run the following command for more details
+    ```
+    node_modules/.bin/eslint --report-unused-disable-directives --no-inline-config \\
+    #{eslint_candidates.map { |path| "  '#{path}'" }.join(" \\\n")}
+    ```
+  MARKDOWN
+end

danger/frozen_string/Dangerfile

@@ -16,11 +16,13 @@ if files_to_fix.any?
   warn 'This merge request adds files that do not enforce frozen string literal. ' \
     'See https://gitlab.com/gitlab-org/gitlab-ce/issues/47424 for more information.'
-  markdown(<<~MARKDOWN)
-    ## Enable Frozen String Literal
-    The following files should have `#{MAGIC_COMMENT}` on the first line:
-    * #{files_to_fix.map { |path| "`#{path}`" }.join("\n* ")}
-  MARKDOWN
+  if GitlabDanger.new(helper.gitlab_helper).ci?
+    markdown(<<~MARKDOWN)
+      ## Enable Frozen String Literal
+      The following files should have `#{MAGIC_COMMENT}` on the first line:
+      * #{files_to_fix.map { |path| "`#{path}`" }.join("\n* ")}
+    MARKDOWN
+  end
 end

danger/gemfile/Dangerfile

-GEMFILE_LOCK_NOT_UPDATED_MESSAGE = <<~MSG.freeze
-**%<gemfile>s was updated but %<gemfile_lock>s wasn't updated.**
+GEMFILE_LOCK_NOT_UPDATED_MESSAGE_SHORT = <<~MSG.freeze
+%<gemfile>s was updated but %<gemfile_lock>s wasn't updated.
+MSG
+
+GEMFILE_LOCK_NOT_UPDATED_MESSAGE_FULL = <<~MSG.freeze
+**#{GEMFILE_LOCK_NOT_UPDATED_MESSAGE_SHORT}**
 Usually, when %<gemfile>s is updated, you should run
 ```
@@ -19,5 +23,14 @@ gemfile_modified = git.modified_files.include?("Gemfile")
 gemfile_lock_modified = git.modified_files.include?("Gemfile.lock")
 if gemfile_modified && !gemfile_lock_modified
-  warn format(GEMFILE_LOCK_NOT_UPDATED_MESSAGE, gemfile: gitlab.html_link("Gemfile"), gemfile_lock: gitlab.html_link("Gemfile.lock"))
+  gitlab_danger = GitlabDanger.new(helper.gitlab_helper)
+
+  format_str = gitlab_danger.ci? ? GEMFILE_LOCK_NOT_UPDATED_MESSAGE_FULL : GEMFILE_LOCK_NOT_UPDATED_MESSAGE_SHORT
+
+  message = format(format_str,
+    gemfile: gitlab_danger.html_link("Gemfile"),
+    gemfile_lock: gitlab_danger.html_link("Gemfile.lock")
+  )
+
+  warn(message)
 end

danger/prettier/Dangerfile

@@ -19,21 +19,23 @@ return if unpretty.empty?
 warn 'This merge request changed frontend files without pretty printing them.'
-markdown(<<~MARKDOWN)
-  ## Pretty print Frontend files
-  The following files should have been pretty printed with `prettier`:
-  * #{unpretty.map { |path| "`#{path}`" }.join("\n* ")}
-  Please run
-  ```
-  node_modules/.bin/prettier --write \\
-  #{unpretty.map { |path| "  '#{path}'" }.join(" \\\n")}
-  ```
-  Also consider auto-formatting [on-save].
-  [on-save]: https://docs.gitlab.com/ee/development/new_fe_guide/style/prettier.html
-MARKDOWN
+if GitlabDanger.new(helper.gitlab_helper).ci?
+  markdown(<<~MARKDOWN)
+    ## Pretty print Frontend files
+    The following files should have been pretty printed with `prettier`:
+    * #{unpretty.map { |path| "`#{path}`" }.join("\n* ")}
+    Please run
+    ```
+    node_modules/.bin/prettier --write \\
+    #{unpretty.map { |path| "  '#{path}'" }.join(" \\\n")}
+    ```
+    Also consider auto-formatting [on-save].
+    [on-save]: https://docs.gitlab.com/ee/development/new_fe_guide/style/prettier.html
+  MARKDOWN
+end

lib/gitlab/danger/helper.rb

@@ -38,8 +38,17 @@ module Gitlab
         ENV['CI_PROJECT_NAME'] == 'gitlab-ee' || File.exist?('../../CHANGELOG-EE.md')
       end
+      def gitlab_helper
+        # Unfortunately the following does not work:
+        # - respond_to?(:gitlab)
+        # - respond_to?(:gitlab, true)
+        gitlab
+      rescue NoMethodError
+        nil
+      end
+
       def release_automation?
-        gitlab.mr_author == RELEASE_TOOLS_BOT
+        gitlab_helper&.mr_author == RELEASE_TOOLS_BOT
       end
       def project_name

lib/gitlab_danger.rb

+# frozen_string_literal: true
+
+class GitlabDanger
+  LOCAL_RULES ||= %w[
+    changes_size
+    gemfile
+    documentation
+    frozen_string
+    duplicate_yarn_dependencies
+    prettier
+    eslint
+    database
+  ].freeze
+
+  CI_ONLY_RULES ||= %w[
+    metadata
+    changelog
+    specs
+    commit_messages
+    roulette
+    single_codebase
+    gitlab_ui_wg
+    ce_ee_vue_templates
+    only_documentation
+  ].freeze
+
+  MESSAGE_PREFIX = '==>'.freeze
+
+  attr_reader :gitlab_danger_helper
+
+  def initialize(gitlab_danger_helper)
+    @gitlab_danger_helper = gitlab_danger_helper
+  end
+
+  def self.local_warning_message
+    "#{MESSAGE_PREFIX} Only the following Danger rules can be run locally: #{LOCAL_RULES.join(', ')}"
+  end
+
+  def self.success_message
+    "#{MESSAGE_PREFIX} No Danger rule violations!"
+  end
+
+  def rule_names
+    ci? ? LOCAL_RULES | CI_ONLY_RULES : LOCAL_RULES
+  end
+
+  def html_link(str)
+    self.ci? ? gitlab_danger_helper.html_link(str) : str
+  end
+
+  def ci?
+    !gitlab_danger_helper.nil?
+  end
+end

lib/tasks/gitlab_danger.rake

+desc 'Run local Danger rules'
+task :danger_local do
+  require 'gitlab_danger'
+  require 'gitlab/popen'
+
+  puts("#{GitlabDanger.local_warning_message}\n")
+
+  # _status will _always_ be 0, regardless of failure or success :(
+  output, _status = Gitlab::Popen.popen(%w{danger dry_run})
+
+  if output.empty?
+    puts(GitlabDanger.success_message)
+  else
+    puts(output)
+    exit(1)
+  end
+end

spec/lib/gitlab/danger/helper_spec.rb

@@ -11,16 +11,62 @@ describe Gitlab::Danger::Helper do
   class FakeDanger
     include Gitlab::Danger::Helper
-    attr_reader :git
-    def initialize(git:)
+    attr_reader :git, :gitlab
+    def initialize(git:, gitlab:)
       @git = git
+      @gitlab = gitlab
     end
   end
   let(:fake_git) { double('fake-git') }
-  subject(:helper) { FakeDanger.new(git: fake_git) }
+  let(:mr_author) { nil }
+  let(:fake_gitlab) { double('fake-gitlab', mr_author: mr_author) }
+
+  subject(:helper) { FakeDanger.new(git: fake_git, gitlab: fake_gitlab) }
+
+  describe '#gitlab_helper' do
+    context 'when gitlab helper is not available' do
+      let(:fake_gitlab) { nil }
+
+      it 'returns nil' do
+        expect(helper.gitlab_helper).to be_nil
+      end
+    end
+
+    context 'when gitlab helper is available' do
+      it 'returns the gitlab helper' do
+        expect(helper.gitlab_helper).to eq(fake_gitlab)
+      end
+    end
+  end
+
+  describe '#release_automation?' do
+    context 'when gitlab helper is not available' do
+      it 'returns false' do
+        expect(helper.release_automation?).to be_falsey
+      end
+    end
+
+    context 'when gitlab helper is available' do
+      context "but the MR author isn't the RELEASE_TOOLS_BOT" do
+        let(:mr_author) { 'johnmarston' }
+
+        it 'returns false' do
+          expect(helper.release_automation?).to be_falsey
+        end
+      end
+
+      context 'and the MR author is the RELEASE_TOOLS_BOT' do
+        let(:mr_author) { described_class::RELEASE_TOOLS_BOT }
+
+        it 'returns true' do
+          expect(helper.release_automation?).to be_truthy
+        end
+      end
+    end
+  end
   describe '#all_changed_files' do
     subject { helper.all_changed_files }

spec/lib/gitlab_danger_spec.rb

+# frozen_string_literal: true
+
+require 'spec_helper'
+
+describe GitlabDanger do
+  let(:gitlab_danger_helper) { nil }
+
+  subject { described_class.new(gitlab_danger_helper) }
+
+  describe '.local_warning_message' do
+    it 'returns an informational message with rules that can run' do
+      expect(described_class.local_warning_message).to eq('==> Only the following Danger rules can be run locally: changes_size, gemfile, documentation, frozen_string, duplicate_yarn_dependencies, prettier, eslint, database')
+    end
+  end
+
+  describe '.success_message' do
+    it 'returns an informational success message' do
+      expect(described_class.success_message).to eq('==> No Danger rule violations!')
+    end
+  end
+
+  describe '#rule_names' do
+    context 'when running locally' do
+      it 'returns local only rules' do
+        expect(subject.rule_names).to eq(described_class::LOCAL_RULES)
+      end
+    end
+
+    context 'when running under CI' do
+      let(:gitlab_danger_helper) { double('danger_gitlab_helper') }
+
+      it 'returns all rules' do
+        expect(subject.rule_names).to eq(described_class::LOCAL_RULES | described_class::CI_ONLY_RULES)
+      end
+    end
+  end
+
+  describe '#html_link' do
+    context 'when running locally' do
+      it 'returns the same string' do
+        str = 'something'
+
+        expect(subject.html_link(str)).to eq(str)
+      end
+    end
+
+    context 'when running under CI' do
+      let(:gitlab_danger_helper) { double('danger_gitlab_helper') }
+
+      it 'returns a HTML link formatted version of the string' do
+        str = 'something'
+        html_formatted_str = %Q{<a href="#{str}">#{str}</a>}
+
+        expect(gitlab_danger_helper).to receive(:html_link).with(str).and_return(html_formatted_str)
+
+        expect(subject.html_link(str)).to eq(html_formatted_str)
+      end
+    end
+  end
+
+  describe '#ci?' do
+    context 'when gitlab_danger_helper is not available' do
+      it 'returns false' do
+        expect(subject.ci?).to be_falsey
+      end
+    end
+
+    context 'when gitlab_danger_helper is available' do
+      let(:gitlab_danger_helper) { double('danger_gitlab_helper') }
+
+      it 'returns true' do
+        expect(subject.ci?).to be_truthy
+      end
+    end
+  end
+end