diff --git a/Dangerfile b/Dangerfile index 094d55e8652..228190cd530 100644 --- a/Dangerfile +++ b/Dangerfile @@ -1,23 +1,12 @@ # 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 diff --git a/Gemfile b/Gemfile index 3e3a8c5dde2..fdb30aeb187 100644 --- a/Gemfile +++ b/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 diff --git a/Gemfile.lock b/Gemfile.lock index 6add217bc32..d787b5c0569 100644 --- a/Gemfile.lock +++ b/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) diff --git a/danger/database/Dangerfile b/danger/database/Dangerfile index 3550cb7eabf..5cdad09db6e 100644 --- a/danger/database/Dangerfile +++ b/danger/database/Dangerfile @@ -1,7 +1,13 @@ # frozen_string_literal: true -SCHEMA_NOT_UPDATED_MESSAGE = <<~MSG -**New %s added but %s wasn't updated.** +gitlab_danger = GitlabDanger.new(helper.gitlab_helper) + +SCHEMA_NOT_UPDATED_MESSAGE_SHORT = <<~MSG +New %s added but %s wasn't updated. +MSG + +SCHEMA_NOT_UPDATED_MESSAGE_FULL = <<~MSG +**#{SCHEMA_NOT_UPDATED_MESSAGE_SHORT}** Usually, when adding new %s, %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? diff --git a/danger/documentation/Dangerfile b/danger/documentation/Dangerfile index 96c0d9b7478..ad64c3a6c60 100644 --- a/danger/documentation/Dangerfile +++ b/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 + if GitlabDanger.new(helper.gitlab_helper).ci? + markdown(<<~MARKDOWN) + ## Documentation review -The following files require a review from a technical writer: + The following files require a review from a technical writer: -* #{docs_paths_to_review.map { |path| "`#{path}`" }.join("\n* ")} + * #{docs_paths_to_review.map { |path| "`#{path}`" }.join("\n* ")} -The review does not need to block merging this merge request. See the: + 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 + - [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.' + unless gitlab.mr_labels.include?('Documentation') + warn 'This merge request is missing the ~Documentation label.' + end end end diff --git a/danger/duplicate_yarn_dependencies/Dangerfile b/danger/duplicate_yarn_dependencies/Dangerfile index 25f81ec86a4..492b888d00e 100644 --- a/danger/duplicate_yarn_dependencies/Dangerfile +++ b/danger/duplicate_yarn_dependencies/Dangerfile @@ -1,6 +1,6 @@ # 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 +if GitlabDanger.new(helper.gitlab_helper).ci? + markdown(<<~MARKDOWN) + ## Duplicate yarn dependencies - The following dependencies should be de-duplicated: + The following dependencies should be de-duplicated: - * #{duplicate.map { |path| "`#{path}`" }.join("\n* ")} + * #{duplicate.map { |path| "`#{path}`" }.join("\n* ")} - Please run the following command and commit the changes to `yarn.lock`: + Please run the following command and commit the changes to `yarn.lock`: - ``` - node_modules/.bin/yarn-deduplicate --strategy fewer yarn.lock \\ - && yarn install - ``` -MARKDOWN + ``` + node_modules/.bin/yarn-deduplicate --strategy fewer yarn.lock \\ + && yarn install + ``` + MARKDOWN +end diff --git a/danger/eslint/Dangerfile b/danger/eslint/Dangerfile index 4916cacfd7e..92830bd7706 100644 --- a/danger/eslint/Dangerfile +++ b/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 +if GitlabDanger.new(helper.gitlab_helper).ci? + markdown(<<~MARKDOWN) + ## Disabled eslint rules - The following files have disabled `eslint` rules. Please consider fixing them: + The following files have disabled `eslint` rules. Please consider fixing them: - * #{eslint_candidates.map { |path| "`#{path}`" }.join("\n* ")} + * #{eslint_candidates.map { |path| "`#{path}`" }.join("\n* ")} - Run the following command for more details + 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 + ``` + node_modules/.bin/eslint --report-unused-disable-directives --no-inline-config \\ + #{eslint_candidates.map { |path| " '#{path}'" }.join(" \\\n")} + ``` + MARKDOWN +end diff --git a/danger/frozen_string/Dangerfile b/danger/frozen_string/Dangerfile index b9687ef6b83..8d3ac3dee68 100644 --- a/danger/frozen_string/Dangerfile +++ b/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 + if GitlabDanger.new(helper.gitlab_helper).ci? + markdown(<<~MARKDOWN) + ## Enable Frozen String Literal - The following files should have `#{MAGIC_COMMENT}` on the first line: + The following files should have `#{MAGIC_COMMENT}` on the first line: - * #{files_to_fix.map { |path| "`#{path}`" }.join("\n* ")} - MARKDOWN + * #{files_to_fix.map { |path| "`#{path}`" }.join("\n* ")} + MARKDOWN + end end diff --git a/danger/gemfile/Dangerfile b/danger/gemfile/Dangerfile index dfe64f79d7b..07c4c07cfe8 100644 --- a/danger/gemfile/Dangerfile +++ b/danger/gemfile/Dangerfile @@ -1,5 +1,9 @@ -GEMFILE_LOCK_NOT_UPDATED_MESSAGE = <<~MSG.freeze -**%s was updated but %s wasn't updated.** +GEMFILE_LOCK_NOT_UPDATED_MESSAGE_SHORT = <<~MSG.freeze +%s was updated but %s wasn't updated. +MSG + +GEMFILE_LOCK_NOT_UPDATED_MESSAGE_FULL = <<~MSG.freeze +**#{GEMFILE_LOCK_NOT_UPDATED_MESSAGE_SHORT}** Usually, when %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 diff --git a/danger/prettier/Dangerfile b/danger/prettier/Dangerfile index 37c4b78a213..0be75db8baa 100644 --- a/danger/prettier/Dangerfile +++ b/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 +if GitlabDanger.new(helper.gitlab_helper).ci? + markdown(<<~MARKDOWN) + ## Pretty print Frontend files - The following files should have been pretty printed with `prettier`: + The following files should have been pretty printed with `prettier`: - * #{unpretty.map { |path| "`#{path}`" }.join("\n* ")} + * #{unpretty.map { |path| "`#{path}`" }.join("\n* ")} - Please run + Please run - ``` - node_modules/.bin/prettier --write \\ - #{unpretty.map { |path| " '#{path}'" }.join(" \\\n")} - ``` + ``` + node_modules/.bin/prettier --write \\ + #{unpretty.map { |path| " '#{path}'" }.join(" \\\n")} + ``` - Also consider auto-formatting [on-save]. + Also consider auto-formatting [on-save]. - [on-save]: https://docs.gitlab.com/ee/development/new_fe_guide/style/prettier.html -MARKDOWN + [on-save]: https://docs.gitlab.com/ee/development/new_fe_guide/style/prettier.html + MARKDOWN +end diff --git a/lib/gitlab/danger/helper.rb b/lib/gitlab/danger/helper.rb index 17ad07bfc0c..e2911b4e6c8 100644 --- a/lib/gitlab/danger/helper.rb +++ b/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 diff --git a/lib/gitlab_danger.rb b/lib/gitlab_danger.rb new file mode 100644 index 00000000000..b4768a9546d --- /dev/null +++ b/lib/gitlab_danger.rb @@ -0,0 +1,54 @@ +# 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 diff --git a/lib/tasks/gitlab_danger.rake b/lib/tasks/gitlab_danger.rake new file mode 100644 index 00000000000..e75539f048c --- /dev/null +++ b/lib/tasks/gitlab_danger.rake @@ -0,0 +1,17 @@ +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 diff --git a/spec/lib/gitlab/danger/helper_spec.rb b/spec/lib/gitlab/danger/helper_spec.rb index 710564b7540..1b4d366ce7b 100644 --- a/spec/lib/gitlab/danger/helper_spec.rb +++ b/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 + attr_reader :git, :gitlab - def initialize(git:) + 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 } diff --git a/spec/lib/gitlab_danger_spec.rb b/spec/lib/gitlab_danger_spec.rb new file mode 100644 index 00000000000..623ac20fa7c --- /dev/null +++ b/spec/lib/gitlab_danger_spec.rb @@ -0,0 +1,76 @@ +# 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{#{str}} + + 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