summaryrefslogtreecommitdiff
path: root/danger
diff options
context:
space:
mode:
authorRémy Coutable <remy@rymai.me>2018-07-10 12:10:54 +0200
committerRémy Coutable <remy@rymai.me>2018-07-11 11:52:03 +0200
commitab87e7bab1d5cc20c7b69644843bfcb1f3f16918 (patch)
tree2f908718378fbe6984d65781ea76c623ace57eb6 /danger
parentdc629bb6b8146477fdbf9fcd11d10ebedc785029 (diff)
downloadgitlab-ce-ab87e7bab1d5cc20c7b69644843bfcb1f3f16918.tar.gz
Improve Danger files after first review
Signed-off-by: Rémy Coutable <remy@rymai.me>
Diffstat (limited to 'danger')
-rw-r--r--danger/changelog/Dangerfile53
-rw-r--r--danger/changes_size/Dangerfile20
-rw-r--r--danger/database/Dangerfile41
-rw-r--r--danger/gemfile/Dangerfile27
-rw-r--r--danger/metadata/Dangerfile4
-rw-r--r--danger/specs/Dangerfile13
6 files changed, 111 insertions, 47 deletions
diff --git a/danger/changelog/Dangerfile b/danger/changelog/Dangerfile
index 773ffa15a0f..b4a2f7b964c 100644
--- a/danger/changelog/Dangerfile
+++ b/danger/changelog/Dangerfile
@@ -2,23 +2,55 @@
require 'yaml'
+NO_CHANGELOG_LABELS = %w[backstage QA test]
+SEE_DOC = "See [the documentation](https://docs.gitlab.com/ce/development/changelog.html)."
+MISSING_CHANGELOG_MESSAGE = <<~MSG
+**[CHANGELOG missing](https://docs.gitlab.com/ce/development/changelog.html).**
+
+You can create one with:
+
+```
+bin/changelog -m %<mr_iid>s
+```
+
+If your merge request doesn't warrant a CHANGELOG entry,
+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)
+end
+
def check_changelog(path)
yaml = YAML.safe_load(File.read(path))
- fail "`title` should be set, in #{gitlab.html_link(path)}." if yaml["title"].nil?
- fail "`type` should be set, in #{gitlab.html_link(path)}." if yaml["type"].nil?
+ fail "`title` should be set, in #{gitlab.html_link(path)}! #{SEE_DOC}" if yaml["title"].nil?
+ fail "`type` should be set, in #{gitlab.html_link(path)}! #{SEE_DOC}" if yaml["type"].nil?
if yaml["merge_request"].nil?
- message "Consider setting `merge_request` to #{gitlab.mr_json["iid"]} in #{gitlab.html_link(path)}."
- elsif yaml["merge_request"] != gitlab.mr_json["iid"]
- fail "Merge request IID was not set to #{gitlab.mr_json["iid"]}!"
+ message "Consider setting `merge_request` to #{gitlab.mr_json["iid"]} in #{gitlab.html_link(path)}. #{SEE_DOC}"
+ elsif yaml["merge_request"] != gitlab.mr_json["iid"] && !ce_port_changelog?(changelog_path)
+ fail "Merge request ID was not set to #{gitlab.mr_json["iid"]}! #{SEE_DOC}"
end
rescue StandardError
# YAML could not be parsed, fail the build.
- fail "#{gitlab.html_link(path)} isn't valid YAML!"
+ fail "#{gitlab.html_link(path)} isn't valid YAML! #{SEE_DOC}"
end
-changelog_needed = !gitlab.mr_labels.include?("backstage")
+def presented_no_changelog_labels
+ NO_CHANGELOG_LABELS.map { |label| "~#{label}" }.join(', ')
+end
+
+changelog_needed = (gitlab.mr_labels & NO_CHANGELOG_LABELS).empty?
changelog_found = git.added_files.find { |path| path =~ %r{\A(ee/)?(changelogs/unreleased)(-ee)?/} }
if git.modified_files.include?("CHANGELOG.md")
@@ -29,11 +61,6 @@ if changelog_needed
if changelog_found
check_changelog(path)
else
- msg = [
- "This merge request is missing a CHANGELOG entry, you can create one with `bin/changelog -m #{gitlab.mr_json["iid"]}`.",
- "If your merge request doesn't warrant a CHANGELOG entry, consider adding the ~backstage label."
- ]
-
- warn msg.join(" ")
+ warn format(MISSING_CHANGELOG_MESSAGE, mr_iid: gitlab.mr_json["iid"], labels: presented_no_changelog_labels)
end
end
diff --git a/danger/changes_size/Dangerfile b/danger/changes_size/Dangerfile
index 4471e1926c9..597073e6029 100644
--- a/danger/changes_size/Dangerfile
+++ b/danger/changes_size/Dangerfile
@@ -1,9 +1,19 @@
# rubocop:disable Style/SignalException
-if git.lines_of_code > 500
- warn "This merge request is quite big, please consider splitting it into multiple merge requests."
-end
+# FIXME: git.info_for_file raises the following error
+# /usr/local/bundle/gems/git-1.4.0/lib/git/lib.rb:956:in `command': (Danger::DSLError)
+# [!] Invalid `Dangerfile` file:
+# [!] Invalid `Dangerfile` file: git '--git-dir=/builds/gitlab-org/gitlab-ce/.git' '--work-tree=/builds/gitlab-org/gitlab-ce' cat-file '-t' '' 2>&1:fatal: Not a valid object name
+# This seems to be the same as https://github.com/danger/danger/issues/535.
+
+# locale_files_updated = git.modified_files.select { |path| path.start_with?('locale') }
+# locale_files_updated.each do |locale_file_updated|
+# git_stats = git.info_for_file(locale_file_updated)
+# message "Git stats for #{locale_file_updated}: #{git_stats[:insertions]} insertions, #{git_stats[:deletions]} insertions"
+# end
-if git.lines_of_code > 5_000
- fail "This merge request is definitely too big, please split it into multiple merge requests."
+if git.lines_of_code > 2_000
+ warn "This merge request is definitely too big (more than #{git.lines_of_code} lines changed), please split it into multiple merge requests."
+elsif git.lines_of_code > 500
+ warn "This merge request is quite big (more than #{git.lines_of_code} lines changed), please consider splitting it into multiple merge requests."
end
diff --git a/danger/database/Dangerfile b/danger/database/Dangerfile
index 136dcef7972..4759ceb7eb4 100644
--- a/danger/database/Dangerfile
+++ b/danger/database/Dangerfile
@@ -1,16 +1,31 @@
# rubocop:disable Style/SignalException
-db_schema_updated = !git.modified_files.grep(%r{\A(ee/)?(db/(geo/)?(post_)?migrate)/}).empty?
-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?
-
-if (migration_created || geo_migration_created) && !db_schema_updated
- msg = ["New migrations were added but #{gitlab.html_link("db/schema.rb")}"]
- msg << "(nor #{gitlab.html_link("ee/db/geo/schema.rb")})" if geo_migration_created
- msg << "wasn't. Usually, when adding new migrations, #{gitlab.html_link("db/schema.rb")}"
- msg << "(and #{gitlab.html_link("ee/db/geo/schema.rb")})" if geo_migration_created
- msg << "should be updated too (unless your migrations are data migrations and your"
- msg << "migration isn't the most recent one)."
-
- warn msg.join(" ")
+ANY_MIGRATIONS_REGEX = %r{\A(ee/)?(db/(geo/)?(post_)?migrate)/}
+SCHEMA_NOT_UPDATED_MESSAGE = <<~MSG
+**New %<migrations>s added but %<schema>s wasn't updated.**
+
+Usually, when adding new %<migrations>s, %<schema>s should be
+updated too (unless the migration isn't changing the DB schema
+and isn't the most recent one).
+MSG
+
+non_geo_db_schema_updated = !git.modified_files.grep(%r{\Adb/schema\.rb/}).empty?
+geo_db_schema_updated = !git.modified_files.grep(%r{\Aee/db/geo/schema\.rb/}).empty?
+
+any_migration_created = !git.added_files.grep(ANY_MIGRATIONS_REGEX).empty?
+any_migration_updated = !git.modified_files.grep(ANY_MIGRATIONS_REGEX).empty?
+
+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?
+
+if any_migration_created || any_migration_updated || non_geo_db_schema_updated || geo_db_schema_updated
+ message "Please make sure to ask a Database engineer for a review."
+end
+
+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"))
+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"))
end
diff --git a/danger/gemfile/Dangerfile b/danger/gemfile/Dangerfile
index 3d38959a9d0..b9f59d95815 100644
--- a/danger/gemfile/Dangerfile
+++ b/danger/gemfile/Dangerfile
@@ -1,15 +1,26 @@
# rubocop:disable Style/SignalException
+GEMFILE_LOCK_NOT_UPDATED_MESSAGE = <<~MSG
+**%<gemfile>s was updated but %<gemfile_lock>s wasn't updated.**
+
+Usually, when %<gemfile>s is updated, you should run
+```
+bundle install && \
+ BUNDLE_GEMFILE=Gemfile.rails5 bundle install
+```
+
+or
+
+```
+bundle update <the-added-or-updated-gem>
+```
+
+and commit the %<gemfile_lock>s changes.
+MSG
+
gemfile_modified = git.modified_files.include?("Gemfile")
gemfile_lock_modified = git.modified_files.include?("Gemfile.lock")
if gemfile_modified && !gemfile_lock_modified
- msg = [
- "#{gitlab.html_link("Gemfile")} was edited but #{gitlab.html_link("Gemfile.lock")} wasn't.",
- "Usually, when #{gitlab.html_link("Gemfile")} is updated, you should run",
- "`bundle install && BUNDLE_GEMFILE=Gemfile.rails5 bundle install` or",
- "`bundle update <the-added-or-updated-gem>` and commit the #{gitlab.html_link("Gemfile.lock")} changes."
- ]
-
- warn msg.join(" ")
+ warn format(GEMFILE_LOCK_NOT_UPDATED_MESSAGE, gemfile: gitlab.html_link("Gemfile"), gemfile_lock: gitlab.html_link("Gemfile.lock"))
end
diff --git a/danger/metadata/Dangerfile b/danger/metadata/Dangerfile
index 033a84d6ecd..3cfaa04e01b 100644
--- a/danger/metadata/Dangerfile
+++ b/danger/metadata/Dangerfile
@@ -1,7 +1,7 @@
# rubocop:disable Style/SignalException
if gitlab.mr_body.size < 5
- fail "Please provide a merge request description."
+ fail "Please provide a proper merge request description."
end
if gitlab.mr_labels.empty?
@@ -21,5 +21,5 @@ end
has_pick_into_stable_label = gitlab.mr_labels.find { |label| label.start_with?('Pick into') }
if gitlab.branch_for_base != "master" && !has_pick_into_stable_label
- fail "All merge requests should target `master`. Otherwise, please set the relevant `Pick into X.Y` label."
+ warn "Most of the time, all merge requests should target `master`. Otherwise, please set the relevant `Pick into X.Y` label."
end
diff --git a/danger/specs/Dangerfile b/danger/specs/Dangerfile
index 88e64c57a4b..0ed5235ec02 100644
--- a/danger/specs/Dangerfile
+++ b/danger/specs/Dangerfile
@@ -1,13 +1,14 @@
# rubocop:disable Style/SignalException
+NO_NEW_SPEC_MESSAGE = <<~MSG
+You've made some app changes, but didn't add any tests.
+That's OK as long as you're refactoring existing code,
+but please consider adding the ~backstage label in that case.
+MSG
+
has_app_changes = !git.modified_files.grep(%r{\A(ee/)?(app|lib|db/(geo/)?(post_)?migrate)/}).empty?
has_spec_changes = !git.modified_files.grep(/spec/).empty?
if has_app_changes && !has_spec_changes
- msg = [
- "You've made some app changes, but didn't add any tests.",
- "That's OK as long as you're refactoring existing code (please consider adding the ~backstage label in that case)."
- ]
-
- warn msg.join(" "), sticky: false
+ warn NO_NEW_SPEC_MESSAGE, sticky: false
end