From 07cc841f2dbf5c35eb06ad009d6fe8718b7a3baf Mon Sep 17 00:00:00 2001 From: Doug Stull Date: Thu, 12 Oct 2023 14:37:08 -0400 Subject: [PATCH 1/4] Add conversation starter for disabling rubocop rule inline to danger - added guidance and context suggestions. --- danger/plugins/rubocop.rb | 12 +++ danger/rubocop/Dangerfile | 5 ++ ee/app/helpers/ee/ci/runners_helper.rb | 4 +- .../rubocop_inline_disable_suggestion_spec.rb | 82 +++++++++++++++++++ .../rubocop_inline_disable_suggestion.rb | 41 ++++++++++ 5 files changed, 142 insertions(+), 2 deletions(-) create mode 100644 danger/plugins/rubocop.rb create mode 100644 danger/rubocop/Dangerfile create mode 100644 spec/tooling/danger/rubocop_inline_disable_suggestion_spec.rb create mode 100644 tooling/danger/rubocop_inline_disable_suggestion.rb diff --git a/danger/plugins/rubocop.rb b/danger/plugins/rubocop.rb new file mode 100644 index 00000000000000..6645a37041beab --- /dev/null +++ b/danger/plugins/rubocop.rb @@ -0,0 +1,12 @@ +# frozen_string_literal: true + +require_relative '../../tooling/danger/rubocop_inline_disable_suggestion' + +module Danger + class Rubocop < ::Danger::Plugin + # Put the helper code somewhere it can be tested + def add_suggestions_for(filename) + Tooling::Danger::RubocopInlineDisableSuggestion.new(filename, context: self).suggest + end + end +end diff --git a/danger/rubocop/Dangerfile b/danger/rubocop/Dangerfile new file mode 100644 index 00000000000000..a53847199dbf58 --- /dev/null +++ b/danger/rubocop/Dangerfile @@ -0,0 +1,5 @@ +# frozen_string_literal: true + +helper.all_changed_files.each do |filename| + rubocop.add_suggestions_for(filename) +end diff --git a/ee/app/helpers/ee/ci/runners_helper.rb b/ee/app/helpers/ee/ci/runners_helper.rb index dcc3ddf29f8aa7..97b27df37b557f 100644 --- a/ee/app/helpers/ee/ci/runners_helper.rb +++ b/ee/app/helpers/ee/ci/runners_helper.rb @@ -26,13 +26,13 @@ def validate_credit_card?(project) end def show_buy_pipeline_minutes?(project, namespace) - return false unless ::Gitlab::Saas.feature_available?('purchases/additional_minutes') + return false unless ::Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks show_out_of_pipeline_minutes_notification?(project, namespace) end def show_pipeline_minutes_notification_dot?(project, namespace) - return false unless ::Gitlab::Saas.feature_available?('purchases/additional_minutes') + return false unless ::Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks return false if notification_dot_acknowledged? show_out_of_pipeline_minutes_notification?(project, namespace) diff --git a/spec/tooling/danger/rubocop_inline_disable_suggestion_spec.rb b/spec/tooling/danger/rubocop_inline_disable_suggestion_spec.rb new file mode 100644 index 00000000000000..49375336cfde75 --- /dev/null +++ b/spec/tooling/danger/rubocop_inline_disable_suggestion_spec.rb @@ -0,0 +1,82 @@ +# frozen_string_literal: true + +require 'gitlab/dangerfiles/spec_helper' + +require_relative '../../../tooling/danger/rubocop_inline_disable_suggestion' +require_relative '../../../tooling/danger/project_helper' + +RSpec.describe Tooling::Danger::RubocopInlineDisableSuggestion, feature_category: :tooling do + include_context "with dangerfile" + + let(:fake_danger) { DangerSpecHelper.fake_danger } + let(:fake_project_helper) { instance_double('Tooling::Danger::ProjectHelper') } + let(:filename) { 'spec/foo_spec.rb' } + + let(:template) do + <<~SUGGESTION_MARKDOWN.chomp + + Consider removing this inline disabling and adhering to the rubocop rule. + If that isn't possible, please provide context as a reply for reviewers. + See [rubocop best practices](https://docs.gitlab.com/ee/development/rubocop_development_guide.html). + SUGGESTION_MARKDOWN + end + + let(:file_lines) do + [ + "def validate_credit_card?(project)", + " !current_user.has_required_credit_card_to_enable_shared_runners?(project)", + " return true if Gitlab.com? # rubocop:disable Some/Cop", + "end", + "\n", + "def show_buy_pipeline_minutes?(project, namespace)", + " return false unless ::Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks", + "\n", + " show_out_of_pipeline_minutes_notification?(project, namespace)", + "end", + "\n", + "def show_pipeline_minutes_notification_dot?(project, namespace)", + " return false unless ::Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks", + " return false if notification_dot_acknowledged?", + "\n", + " show_out_of_pipeline_minutes_notification?(project, namespace)", + "end", + "\n", + "def show_dot?(project, namespace)", + " return false unless ::Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks", + " return false if notification_dot_acknowledged?", + "\n", + " show_out_of_pipeline_minutes_notification?(project, namespace)", + "end" + ] + end + + let(:changed_lines) do + [ + "+ return true if Gitlab.com? # rubocop:disable Some/Cop", + "+end", + "+ return false unless ::Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks", + "+ return false unless ::Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks", + "+ return false unless ::Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks" + ] + end + + subject(:rubocop) { fake_danger.new(helper: fake_helper) } + + before do + allow(rubocop).to receive(:project_helper).and_return(fake_project_helper) + allow(rubocop.helper).to receive(:changed_lines).with(filename).and_return(changed_lines) + allow(rubocop.project_helper).to receive(:file_lines).and_return(file_lines) + + rubocop.define_singleton_method(:add_suggestions_for) do |filename| + Tooling::Danger::RubocopInlineDisableSuggestion.new(filename, context: self).suggest + end + end + + it 'adds comments at the correct lines', :aggregate_failures do + [3, 7, 13, 20].each do |line_number| + expect(rubocop).to receive(:markdown).with(format(template), file: filename, line: line_number) + end + + rubocop.add_suggestions_for(filename) + end +end diff --git a/tooling/danger/rubocop_inline_disable_suggestion.rb b/tooling/danger/rubocop_inline_disable_suggestion.rb new file mode 100644 index 00000000000000..04ec1562bbc432 --- /dev/null +++ b/tooling/danger/rubocop_inline_disable_suggestion.rb @@ -0,0 +1,41 @@ +# frozen_string_literal: true + +require_relative 'suggestion' + +module Tooling + module Danger + class RubocopInlineDisableSuggestion < Suggestion + RUBOCOP_DISABLE_REGEX = /^\+.*#\s*rubocop\s*:\s*(?:disable|todo)\s+/ + MESSAGE = <<~MESSAGE_MARKDOWN + Consider removing this inline disabling and adhering to the rubocop rule. + If that isn't possible, please provide context as a reply for reviewers. + See [rubocop best practices](https://docs.gitlab.com/ee/development/rubocop_development_guide.html). + MESSAGE_MARKDOWN + + def suggest + @commented_lines = [] + @file_lines = project_helper.file_lines(filename) + changed_lines = helper.changed_lines(filename) + + changed_lines.each { |changed_line| parse_line_and_comment(changed_line) } + end + + private + + def parse_line_and_comment(changed_line) + return unless RUBOCOP_DISABLE_REGEX.match?(changed_line) + + # Sometimes line content will exactly match in a file, + # so we want to make sure we comment on all of them correctly without repeating. + @file_lines.each_with_index do |line, line_number| + next if @commented_lines.include?(line_number) + next unless line == changed_line.delete_prefix('+') + + @commented_lines << line_number + + markdown(comment(MESSAGE), file: filename, line: line_number.succ) + end + end + end + end +end -- GitLab From 7744030ddb80d36c866fd625f38db4f0fb06c77c Mon Sep 17 00:00:00 2001 From: Doug Stull Date: Fri, 13 Oct 2023 09:17:21 -0400 Subject: [PATCH 2/4] Update wording and specs --- .../rubocop_inline_disable_suggestion_spec.rb | 97 ++++++++++++------- .../rubocop_inline_disable_suggestion.rb | 34 ++----- 2 files changed, 69 insertions(+), 62 deletions(-) diff --git a/spec/tooling/danger/rubocop_inline_disable_suggestion_spec.rb b/spec/tooling/danger/rubocop_inline_disable_suggestion_spec.rb index 49375336cfde75..bc8ac5c10a26f5 100644 --- a/spec/tooling/danger/rubocop_inline_disable_suggestion_spec.rb +++ b/spec/tooling/danger/rubocop_inline_disable_suggestion_spec.rb @@ -18,46 +18,73 @@ Consider removing this inline disabling and adhering to the rubocop rule. If that isn't possible, please provide context as a reply for reviewers. See [rubocop best practices](https://docs.gitlab.com/ee/development/rubocop_development_guide.html). + + ([Improve this message](https://gitlab.com/gitlab-org/gitlab/-/blob/master/tooling/danger/rubocop_inline_disable_suggestion.rb) + or [have feedback?](https://gitlab.com/gitlab-org/gitlab/-/issues/428157)) SUGGESTION_MARKDOWN end let(:file_lines) do - [ - "def validate_credit_card?(project)", - " !current_user.has_required_credit_card_to_enable_shared_runners?(project)", - " return true if Gitlab.com? # rubocop:disable Some/Cop", - "end", - "\n", - "def show_buy_pipeline_minutes?(project, namespace)", - " return false unless ::Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks", - "\n", - " show_out_of_pipeline_minutes_notification?(project, namespace)", - "end", - "\n", - "def show_pipeline_minutes_notification_dot?(project, namespace)", - " return false unless ::Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks", - " return false if notification_dot_acknowledged?", - "\n", - " show_out_of_pipeline_minutes_notification?(project, namespace)", - "end", - "\n", - "def show_dot?(project, namespace)", - " return false unless ::Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks", - " return false if notification_dot_acknowledged?", - "\n", - " show_out_of_pipeline_minutes_notification?(project, namespace)", - "end" - ] + <<~RUBY.split("\n") + def validate_credit_card?(project) + !current_user.has_required_credit_card_to_enable_shared_runners?(project) + return true if Gitlab.com? # rubocop:disable Some/Cop + end + + def show_buy_pipeline_minutes?(project, namespace) + return false unless ::Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks + + show_out_of_pipeline_minutes_notification?(project, namespace) + end + + def show_pipeline_minutes_notification_dot?(project, namespace) + return false unless ::Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks + return false if notification_dot_acknowledged? + + show_out_of_pipeline_minutes_notification?(project, namespace) + end + + def show_dot?(project, namespace) + return false unless ::Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks + return false if notification_dot_acknowledged? + + show_out_of_pipeline_minutes_notification?(project, namespace) + end + + def show_other_dot?(project, namespace) + return false unless ::Gitlab.com? # rubocop: disable Gitlab/AvoidGitlabInstanceChecks + return false if notification_dot_acknowledged? + + show_out_of_pipeline_minutes_notification?(project, namespace) + end + + def show_my_dot?(project, namespace) + return false unless ::Gitlab.com? # rubocop:todo Gitlab/AvoidGitlabInstanceChecks + return false if notification_dot_acknowledged? + + show_out_of_pipeline_minutes_notification?(project, namespace) + end + + def show_my_other_dot?(project, namespace) + return false unless ::Gitlab.com? # rubocop: todo Gitlab/AvoidGitlabInstanceChecks + return false if notification_dot_acknowledged? + + show_out_of_pipeline_minutes_notification?(project, namespace) + end + RUBY end let(:changed_lines) do - [ - "+ return true if Gitlab.com? # rubocop:disable Some/Cop", - "+end", - "+ return false unless ::Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks", - "+ return false unless ::Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks", - "+ return false unless ::Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks" - ] + <<~DIFF.split("\n") + + return true if Gitlab.com? # rubocop:disable Some/Cop + +end + + return false unless ::Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks + + return false unless ::Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks + + return false unless ::Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks + + return false unless ::Gitlab.com? # rubocop: disable Gitlab/AvoidGitlabInstanceChecks + + return false unless ::Gitlab.com? # rubocop:todo Gitlab/AvoidGitlabInstanceChecks + + return false unless ::Gitlab.com? # rubocop: todo Gitlab/AvoidGitlabInstanceChecks + DIFF end subject(:rubocop) { fake_danger.new(helper: fake_helper) } @@ -73,8 +100,8 @@ end it 'adds comments at the correct lines', :aggregate_failures do - [3, 7, 13, 20].each do |line_number| - expect(rubocop).to receive(:markdown).with(format(template), file: filename, line: line_number) + [3, 7, 13, 20, 27, 34, 41].each do |line_number| + expect(rubocop).to receive(:markdown).with(template, file: filename, line: line_number) end rubocop.add_suggestions_for(filename) diff --git a/tooling/danger/rubocop_inline_disable_suggestion.rb b/tooling/danger/rubocop_inline_disable_suggestion.rb index 04ec1562bbc432..9f4852f7515205 100644 --- a/tooling/danger/rubocop_inline_disable_suggestion.rb +++ b/tooling/danger/rubocop_inline_disable_suggestion.rb @@ -5,37 +5,17 @@ module Tooling module Danger class RubocopInlineDisableSuggestion < Suggestion - RUBOCOP_DISABLE_REGEX = /^\+.*#\s*rubocop\s*:\s*(?:disable|todo)\s+/ - MESSAGE = <<~MESSAGE_MARKDOWN + MATCH = /^\+.*#\s*rubocop\s*:\s*(?:disable|todo)\s+/ + REPLACEMENT = nil + + SUGGESTION = <<~MESSAGE_MARKDOWN Consider removing this inline disabling and adhering to the rubocop rule. If that isn't possible, please provide context as a reply for reviewers. See [rubocop best practices](https://docs.gitlab.com/ee/development/rubocop_development_guide.html). - MESSAGE_MARKDOWN - - def suggest - @commented_lines = [] - @file_lines = project_helper.file_lines(filename) - changed_lines = helper.changed_lines(filename) - - changed_lines.each { |changed_line| parse_line_and_comment(changed_line) } - end - - private - def parse_line_and_comment(changed_line) - return unless RUBOCOP_DISABLE_REGEX.match?(changed_line) - - # Sometimes line content will exactly match in a file, - # so we want to make sure we comment on all of them correctly without repeating. - @file_lines.each_with_index do |line, line_number| - next if @commented_lines.include?(line_number) - next unless line == changed_line.delete_prefix('+') - - @commented_lines << line_number - - markdown(comment(MESSAGE), file: filename, line: line_number.succ) - end - end + ([Improve this message](https://gitlab.com/gitlab-org/gitlab/-/blob/master/tooling/danger/rubocop_inline_disable_suggestion.rb) + or [have feedback?](https://gitlab.com/gitlab-org/gitlab/-/issues/428157)) + MESSAGE_MARKDOWN end end end -- GitLab From 403b46c63a657b173c67b114337626a36fa06b45 Mon Sep 17 00:00:00 2001 From: Doug Stull Date: Fri, 13 Oct 2023 09:24:53 -0400 Subject: [PATCH 3/4] Back out example failures in runners helper --- ee/app/helpers/ee/ci/runners_helper.rb | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/ee/app/helpers/ee/ci/runners_helper.rb b/ee/app/helpers/ee/ci/runners_helper.rb index 97b27df37b557f..dcc3ddf29f8aa7 100644 --- a/ee/app/helpers/ee/ci/runners_helper.rb +++ b/ee/app/helpers/ee/ci/runners_helper.rb @@ -26,13 +26,13 @@ def validate_credit_card?(project) end def show_buy_pipeline_minutes?(project, namespace) - return false unless ::Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks + return false unless ::Gitlab::Saas.feature_available?('purchases/additional_minutes') show_out_of_pipeline_minutes_notification?(project, namespace) end def show_pipeline_minutes_notification_dot?(project, namespace) - return false unless ::Gitlab.com? # rubocop:disable Gitlab/AvoidGitlabInstanceChecks + return false unless ::Gitlab::Saas.feature_available?('purchases/additional_minutes') return false if notification_dot_acknowledged? show_out_of_pipeline_minutes_notification?(project, namespace) -- GitLab From d0217f6ca13893c560c75423b45d796af2e7eea3 Mon Sep 17 00:00:00 2001 From: Doug Stull Date: Fri, 13 Oct 2023 09:30:24 -0400 Subject: [PATCH 4/4] Remove parenthesis around feedback message --- .../danger/rubocop_inline_disable_suggestion_spec.rb | 6 ++++-- tooling/danger/rubocop_inline_disable_suggestion.rb | 6 ++++-- 2 files changed, 8 insertions(+), 4 deletions(-) diff --git a/spec/tooling/danger/rubocop_inline_disable_suggestion_spec.rb b/spec/tooling/danger/rubocop_inline_disable_suggestion_spec.rb index bc8ac5c10a26f5..94dd5192d74f56 100644 --- a/spec/tooling/danger/rubocop_inline_disable_suggestion_spec.rb +++ b/spec/tooling/danger/rubocop_inline_disable_suggestion_spec.rb @@ -19,8 +19,10 @@ If that isn't possible, please provide context as a reply for reviewers. See [rubocop best practices](https://docs.gitlab.com/ee/development/rubocop_development_guide.html). - ([Improve this message](https://gitlab.com/gitlab-org/gitlab/-/blob/master/tooling/danger/rubocop_inline_disable_suggestion.rb) - or [have feedback?](https://gitlab.com/gitlab-org/gitlab/-/issues/428157)) + ---- + + [Improve this message](https://gitlab.com/gitlab-org/gitlab/-/blob/master/tooling/danger/rubocop_inline_disable_suggestion.rb) + or [have feedback](https://gitlab.com/gitlab-org/gitlab/-/issues/428157)? SUGGESTION_MARKDOWN end diff --git a/tooling/danger/rubocop_inline_disable_suggestion.rb b/tooling/danger/rubocop_inline_disable_suggestion.rb index 9f4852f7515205..4d1bff9856b089 100644 --- a/tooling/danger/rubocop_inline_disable_suggestion.rb +++ b/tooling/danger/rubocop_inline_disable_suggestion.rb @@ -13,8 +13,10 @@ class RubocopInlineDisableSuggestion < Suggestion If that isn't possible, please provide context as a reply for reviewers. See [rubocop best practices](https://docs.gitlab.com/ee/development/rubocop_development_guide.html). - ([Improve this message](https://gitlab.com/gitlab-org/gitlab/-/blob/master/tooling/danger/rubocop_inline_disable_suggestion.rb) - or [have feedback?](https://gitlab.com/gitlab-org/gitlab/-/issues/428157)) + ---- + + [Improve this message](https://gitlab.com/gitlab-org/gitlab/-/blob/master/tooling/danger/rubocop_inline_disable_suggestion.rb) + or [have feedback](https://gitlab.com/gitlab-org/gitlab/-/issues/428157)? MESSAGE_MARKDOWN end end -- GitLab