diff options
Diffstat (limited to 'danger')
| -rw-r--r-- | danger/changelog/Dangerfile | 6 | ||||
| -rw-r--r-- | danger/commit_messages/Dangerfile | 251 | ||||
| -rw-r--r-- | danger/documentation/Dangerfile | 41 | ||||
| -rw-r--r-- | danger/plugins/helper.rb | 37 | ||||
| -rw-r--r-- | danger/roulette/Dangerfile | 81 |
5 files changed, 238 insertions, 178 deletions
diff --git a/danger/changelog/Dangerfile b/danger/changelog/Dangerfile index 530c6638653..63b2f6f5c5c 100644 --- a/danger/changelog/Dangerfile +++ b/danger/changelog/Dangerfile @@ -16,16 +16,12 @@ consider adding any of the %<labels>s labels. #{SEE_DOC} MSG -def ee? - ENV['CI_PROJECT_NAME'] == 'gitlab-ee' || File.exist?('../../CHANGELOG-EE.md') -end - def ee_changelog?(changelog_path) changelog_path =~ /unreleased-ee/ end def ce_port_changelog?(changelog_path) - ee? && !ee_changelog?(changelog_path) + helper.ee? && !ee_changelog?(changelog_path) end def check_changelog(path) diff --git a/danger/commit_messages/Dangerfile b/danger/commit_messages/Dangerfile index abb36098629..9be1ce2ff86 100644 --- a/danger/commit_messages/Dangerfile +++ b/danger/commit_messages/Dangerfile @@ -2,6 +2,9 @@ require 'json' +URL_LIMIT_SUBJECT = "https://chris.beams.io/posts/git-commit/#limit-50" +URL_GIT_COMMIT = "https://chris.beams.io/posts/git-commit/" + # rubocop: disable Style/SignalException # rubocop: disable Metrics/CyclomaticComplexity # rubocop: disable Metrics/PerceivedComplexity @@ -61,11 +64,12 @@ def too_many_changed_lines?(commit) lines_changed_in_commit(commit) >= 30 end -def lint_commits(commits) - failures = false - emoji_checker = EmojiChecker.new +def emoji_checker + @emoji_checker ||= EmojiChecker.new +end - unicode_emoji_regex = %r(( +def unicode_emoji_regex + @unicode_emoji_regex ||= %r(( [\u{1F300}-\u{1F5FF}] | [\u{1F1E6}-\u{1F1FF}] | [\u{2700}-\u{27BF}] | @@ -74,129 +78,138 @@ def lint_commits(commits) [\u{1F680}-\u{1F6FF}] | [\u{2600}-\u{26FF}] ))x +end + +def lint_commit(commit) + # For now we'll ignore merge commits, as getting rid of those is a problem + # separate from enforcing good commit messages. + return false if commit.message.start_with?('Merge branch') + + # We ignore revert commits as they are well structured by Git already + return false if commit.message.start_with?('Revert "') + + failures = false + subject, separator, details = commit.message.split("\n", 3) + + if subject.split.length < 3 + fail_commit( + commit, + 'The commit subject must contain at least three words' + ) + + failures = true + end + + if subject.length > 72 + fail_commit( + commit, + 'The commit subject may not be longer than 72 characters' + ) + + failures = true + elsif subject.length > 50 + warn_commit( + commit, + "This commit's subject line is acceptable, but please try to [reduce it to 50 characters](#{URL_LIMIT_SUBJECT})." + ) + end + + unless subject_starts_with_capital?(subject) + fail_commit(commit, 'The commit subject must start with a capital letter') + failures = true + end + + if subject.end_with?('.') + fail_commit(commit, 'The commit subject must not end with a period') + failures = true + end + + if separator && !separator.empty? + fail_commit( + commit, + 'The commit subject and body must be separated by a blank line' + ) + + failures = true + end + + details&.each_line do |line| + line = line.strip - commits.each do |commit| - # For now we'll ignore merge commits, as getting rid of those is a problem - # separate from enforcing good commit messages. - next if commit.message.start_with?('Merge branch') - - subject, separator, details = commit.message.split("\n", 3) - - if subject.split.length < 3 - fail_commit( - commit, - 'The commit subject must contain at least three words' - ) - - failures = true - end - - if subject.length > 72 - fail_commit( - commit, - 'The commit subject may not be longer than 72 characters' - ) - - failures = true - elsif subject.length > 50 - warn_commit( - commit, - "This commit's subject line could be improved. " \ - 'Commit subjects are ideally no longer than roughly 50 characters, ' \ - 'though we allow up to 72 characters in the subject. ' \ - 'If possible, try to reduce the length of the subject to roughly 50 characters.' - ) - end - - unless subject_starts_with_capital?(subject) - fail_commit(commit, 'The commit subject must start with a capital letter') - failures = true - end - - if subject.end_with?('.') - fail_commit(commit, 'The commit subject must not end with a period') - failures = true - end - - if separator && !separator.empty? - fail_commit( - commit, - 'The commit subject and body must be separated by a blank line' - ) - - failures = true - end - - details&.each_line do |line| - line = line.strip - - next if line.length <= 72 - - url_size = line.scan(%r((https?://\S+))).sum { |(url)| url.length } - - # If the line includes a URL, we'll allow it to exceed 72 characters, but - # only if the line _without_ the URL does not exceed this limit. - next if line.length - url_size <= 72 - - fail_commit( - commit, - 'The commit body should not contain more than 72 characters per line' - ) - - failures = true - end - - if !details && too_many_changed_lines?(commit) - fail_commit( - commit, - 'Commits that change 30 or more lines across at least three files ' \ - 'must describe these changes in the commit body' - ) - - failures = true - end - - if emoji_checker.includes_emoji?(commit.message) - fail_commit( - commit, - 'Avoid the use of Markdown Emoji such as `:+1:`. ' \ - 'These add no value to the commit message, ' \ - 'and are displayed as plain text outside of GitLab' - ) - - failures = true - end - - if commit.message.match?(unicode_emoji_regex) - fail_commit( - commit, - 'Avoid the use of Unicode Emoji. ' \ - 'These add no value to the commit message, ' \ - 'and may not be displayed properly everywhere' - ) - - failures = true - end - - if commit.message.match?(%r(([\w\-\/]+)?(#|!|&|%)\d+)) - fail_commit( - commit, - 'Use full URLs instead of short references ' \ - '(`gitlab-org/gitlab-ce#123` or `!123`), as short references are ' \ - 'displayed as plain text outside of GitLab' - ) - - failures = true - end + next if line.length <= 72 + + url_size = line.scan(%r((https?://\S+))).sum { |(url)| url.length } + + # If the line includes a URL, we'll allow it to exceed 72 characters, but + # only if the line _without_ the URL does not exceed this limit. + next if line.length - url_size <= 72 + + fail_commit( + commit, + 'The commit body should not contain more than 72 characters per line' + ) + + failures = true + end + + if !details && too_many_changed_lines?(commit) + fail_commit( + commit, + 'Commits that change 30 or more lines across at least three files ' \ + 'must describe these changes in the commit body' + ) + + failures = true + end + + if emoji_checker.includes_emoji?(commit.message) + fail_commit( + commit, + 'Avoid the use of Markdown Emoji such as `:+1:`. ' \ + 'These add no value to the commit message, ' \ + 'and are displayed as plain text outside of GitLab' + ) + + failures = true + end + + if commit.message.match?(unicode_emoji_regex) + fail_commit( + commit, + 'Avoid the use of Unicode Emoji. ' \ + 'These add no value to the commit message, ' \ + 'and may not be displayed properly everywhere' + ) + + failures = true + end + + if commit.message.match?(%r(([\w\-\/]+)?(#|!|&|%)\d+\b)) + fail_commit( + commit, + 'Use full URLs instead of short references ' \ + '(`gitlab-org/gitlab-ce#123` or `!123`), as short references are ' \ + 'displayed as plain text outside of GitLab' + ) + + failures = true + end + + failures +end + +def lint_commits(commits) + failed = commits.select do |commit| + lint_commit(commit) end - if failures + if failed.any? markdown(<<~MARKDOWN) ## Commit message standards One or more commit messages do not meet our Git commit message standards. For more information on how to write a good commit message, take a look at - [How to Write a Git Commit Message](https://chris.beams.io/posts/git-commit/). + [How to Write a Git Commit Message](#{URL_GIT_COMMIT}). Here is an example of a good commit message: diff --git a/danger/documentation/Dangerfile b/danger/documentation/Dangerfile index 52af837c261..0cf478b4f89 100644 --- a/danger/documentation/Dangerfile +++ b/danger/documentation/Dangerfile @@ -1,47 +1,36 @@ # frozen_string_literal: true -# All the files/directories that should be reviewed by the Docs team. -DOCS_FILES = [ - 'doc/' -].freeze - -def docs_paths_requiring_review(files) - files.select do |file| - DOCS_FILES.any? { |pattern| file.start_with?(pattern) } - end -end - -docs_paths_to_review = docs_paths_requiring_review(helper.all_changed_files) +docs_paths_to_review = helper.changes_by_category[:docs] unless docs_paths_to_review.empty? - message 'This merge request adds or changes files that require a ' \ - 'review from the Docs team.' + message 'This merge request adds or changes files that require a review ' \ + 'from the Technical Writing team whether before or after merging.' markdown(<<~MARKDOWN) -## Docs review +## Documentation review -The following files require a review from the Documentation team: +The following files will require a review from the [Technical Writing](https://about.gitlab.com/handbook/product/technical-writing/) team: * #{docs_paths_to_review.map { |path| "`#{path}`" }.join("\n* ")} -When your content is ready for review, assign the MR to a technical writer -according to the [DevOps stages](https://about.gitlab.com/handbook/product/categories/#devops-stages) -in the table below. If necessary, mention them in a comment explaining what needs -to be reviewed. +Per the [documentation workflows](https://docs.gitlab.com/ee/development/documentation/workflow.html), the review may occur prior to merging this MR **or** after a maintainer has agreed to review and merge this MR without a tech writer review. (Meanwhile, you are welcome to involve a technical writer at any time if you have questions about writing or updating the documentation. GitLabbers are also welcome to use the [#docs](https://gitlab.slack.com/archives/C16HYA2P5) channel on Slack.) + +The technical writer who, by default, will review this content or collaborate as needed is dependent on the [DevOps stage](https://about.gitlab.com/handbook/product/categories/#devops-stages) to which the content applies: | Tech writer | Stage(s) | | ------------ | ------------------------------------------------------------ | | `@marcia` | ~Create ~Release + ~"development guidelines" | -| `@axil` | ~Distribution ~Gitaly ~Gitter ~Monitoring ~Package ~Secure | +| `@axil` | ~Distribution ~Gitaly ~Gitter ~Monitor ~Package ~Secure | | `@eread` | ~Manage ~Configure ~Geo ~Verify | | `@mikelewis` | ~Plan | -You are welcome to mention them sooner if you have questions about writing or -updating the documentation. GitLabbers are also welcome to use the -[#docs](https://gitlab.slack.com/archives/C16HYA2P5) channel on Slack. +**To request a review prior to merging** + +- Assign the MR to the applicable technical writer, above. If you are not sure which category the change falls within, or the change is not part of one of these categories, mention one of the usernames above. + +**To request a review to commence after merging** -If you are not sure which category the change falls within, or the change is not -part of one of these categories, mention one of the usernames above. +- Create an issue for a doc review [using the Doc Review template](https://gitlab.com/gitlab-org/gitlab-ce/issues/new?issuable_template=Doc%20Review) and assign it to the applicable technicial writer from the table. MARKDOWN unless gitlab.mr_labels.include?('Documentation') diff --git a/danger/plugins/helper.rb b/danger/plugins/helper.rb index f4eb9119266..581c0720083 100644 --- a/danger/plugins/helper.rb +++ b/danger/plugins/helper.rb @@ -1,34 +1,15 @@ # frozen_string_literal: true +require 'net/http' +require 'yaml' + +require_relative '../../lib/gitlab/danger/helper' + module Danger - # Common helper functions for our danger scripts - # If we find ourselves repeating code in our danger files, we might as well put them in here. + # Common helper functions for our danger scripts. See Gitlab::Danger::Helper + # for more details class Helper < Plugin - # Returns a list of all files that have been added, modified or renamed. - # `git.modified_files` might contain paths that already have been renamed, - # so we need to remove them from the list. - # - # Considering these changes: - # - # - A new_file.rb - # - D deleted_file.rb - # - M modified_file.rb - # - R renamed_file_before.rb -> renamed_file_after.rb - # - # it will return - # ``` - # [ 'new_file.rb', 'modified_file.rb', 'renamed_file_after.rb' ] - # ``` - # - # @return [Array<String>] - def all_changed_files - Set.new - .merge(git.added_files.to_a) - .merge(git.modified_files.to_a) - .merge(git.renamed_files.map { |x| x[:after] }) - .subtract(git.renamed_files.map { |x| x[:before] }) - .to_a - .sort - end + # Put the helper code somewhere it can be tested + include Gitlab::Danger::Helper end end diff --git a/danger/roulette/Dangerfile b/danger/roulette/Dangerfile new file mode 100644 index 00000000000..6cf54d0f854 --- /dev/null +++ b/danger/roulette/Dangerfile @@ -0,0 +1,81 @@ +# frozen_string_literal: true + +MESSAGE = <<MARKDOWN +## Reviewer roulette + +Changes that require review have been detected! A merge request is normally +reviewed by both a reviewer and a maintainer in its primary category (e.g. +~frontend or ~backend), and by a maintainer in all other categories. +MARKDOWN + +CATEGORY_TABLE_HEADER = <<MARKDOWN + +To spread load more evenly across eligible reviewers, Danger has randomly picked +a candidate for each review slot. Feel free to override this selection if you +think someone else would be better-suited, or the chosen person is unavailable. + +Once you've decided who will review this merge request, mention them as you +normally would! Danger does not (yet?) automatically notify them for you. + +| Category | Reviewer | Maintainer | +| -------- | -------- | ---------- | +MARKDOWN + +UNKNOWN_FILES_MESSAGE = <<MARKDOWN + +These files couldn't be categorised, so Danger was unable to suggest a reviewer. +Please consider creating a merge request to +[add support](https://gitlab.com/gitlab-org/gitlab-ce/blob/master/lib/gitlab/danger/helper.rb) +for them. +MARKDOWN + +def spin(team, project, category) + reviewers = team.select { |member| member.reviewer?(project, category) } + maintainers = team.select { |member| member.maintainer?(project, category) } + + # TODO: filter out people who are currently not in the office + # TODO: take CODEOWNERS into account? + + reviewer = reviewers[rand(reviewers.size)] + maintainer = maintainers[rand(maintainers.size)] + + "| #{helper.label_for_category(category)} | #{reviewer&.markdown_name} | #{maintainer&.markdown_name} |" +end + +def build_list(items) + list = items.map { |filename| "* `#{filename}`" }.join("\n") + + if items.size > 10 + "\n<details>\n\n#{list}\n\n</details>" + else + list + end +end + +changes = helper.changes_by_category + +# Ignore any files that are known but uncategoried. Prompt for any unknown files +changes.delete(:none) +categories = changes.keys - [:unknown] + +unless changes.empty? + team = + begin + helper.project_team + rescue => err + warn("Reviewer roulette failed to load team data: #{err.message}") + [] + end + + # Exclude the MR author from the team for selection purposes + team.delete_if { |teammate| teammate.username == gitlab.mr_author } + + project = helper.project_name + unknown = changes.fetch(:unknown, []) + + rows = categories.map { |category| spin(team, project, category) } + + markdown(MESSAGE) + markdown(CATEGORY_TABLE_HEADER + rows.join("\n")) unless rows.empty? + markdown(UNKNOWN_FILES_MESSAGE + build_list(unknown)) unless unknown.empty? +end |