From 5679837cd412c2fb7911dcb33c19e89d8a787db0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Mon, 21 May 2018 18:41:21 +0200 Subject: [PATCH 01/14] Start to use Danger for automating MR reviews MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- .gitattributes | 1 + .gitlab-ci.yml | 14 ++++++++++++ Dangerfile | 6 ++++++ danger/changelog/Dangerfile | 39 ++++++++++++++++++++++++++++++++++ danger/changes_size/Dangerfile | 9 ++++++++ danger/database/Dangerfile | 16 ++++++++++++++ danger/gemfile/Dangerfile | 15 +++++++++++++ danger/metadata/Dangerfile | 25 ++++++++++++++++++++++ danger/specs/Dangerfile | 13 ++++++++++++ 9 files changed, 138 insertions(+) create mode 100644 .gitattributes create mode 100644 Dangerfile create mode 100644 danger/changelog/Dangerfile create mode 100644 danger/changes_size/Dangerfile create mode 100644 danger/database/Dangerfile create mode 100644 danger/gemfile/Dangerfile create mode 100644 danger/metadata/Dangerfile create mode 100644 danger/specs/Dangerfile diff --git a/.gitattributes b/.gitattributes new file mode 100644 index 00000000000..f1c41c9bb76 --- /dev/null +++ b/.gitattributes @@ -0,0 +1 @@ +Dangerfile gitlab-language=ruby diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 610a5ecba6d..576a96f66f4 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -348,6 +348,20 @@ retrieve-tests-metadata: - wget -O $FLAKY_RSPEC_SUITE_REPORT_PATH http://${TESTS_METADATA_S3_BUCKET}.s3.amazonaws.com/$FLAKY_RSPEC_SUITE_REPORT_PATH || rm $FLAKY_RSPEC_SUITE_REPORT_PATH - '[[ -f $FLAKY_RSPEC_SUITE_REPORT_PATH ]] || echo "{}" > ${FLAKY_RSPEC_SUITE_REPORT_PATH}' +danger-review: + image: registry.gitlab.com/gitlab-org/gitaly/dangercontainer:latest + stage: prepare + before_script: + - source scripts/utils.sh + - retry gem install danger --no-ri --no-rdoc + cache: {} + except: + - tags + - master + script: + - git version + - danger --fail-on-errors=true + update-tests-metadata: <<: *tests-metadata-state <<: *only-canonical-masters diff --git a/Dangerfile b/Dangerfile new file mode 100644 index 00000000000..84b72673c50 --- /dev/null +++ b/Dangerfile @@ -0,0 +1,6 @@ +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') diff --git a/danger/changelog/Dangerfile b/danger/changelog/Dangerfile new file mode 100644 index 00000000000..773ffa15a0f --- /dev/null +++ b/danger/changelog/Dangerfile @@ -0,0 +1,39 @@ +# rubocop:disable Style/SignalException + +require 'yaml' + +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? + + 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"]}!" + end +rescue StandardError + # YAML could not be parsed, fail the build. + fail "#{gitlab.html_link(path)} isn't valid YAML!" +end + +changelog_needed = !gitlab.mr_labels.include?("backstage") +changelog_found = git.added_files.find { |path| path =~ %r{\A(ee/)?(changelogs/unreleased)(-ee)?/} } + +if git.modified_files.include?("CHANGELOG.md") + fail "CHANGELOG.md was edited. Please remove the additions and create an entry with `bin/changelog -m #{gitlab.mr_json["iid"]}` instead." +end + +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(" ") + end +end diff --git a/danger/changes_size/Dangerfile b/danger/changes_size/Dangerfile new file mode 100644 index 00000000000..4471e1926c9 --- /dev/null +++ b/danger/changes_size/Dangerfile @@ -0,0 +1,9 @@ +# 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 + +if git.lines_of_code > 5_000 + fail "This merge request is definitely too big, please split it into multiple merge requests." +end diff --git a/danger/database/Dangerfile b/danger/database/Dangerfile new file mode 100644 index 00000000000..136dcef7972 --- /dev/null +++ b/danger/database/Dangerfile @@ -0,0 +1,16 @@ +# 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(" ") +end diff --git a/danger/gemfile/Dangerfile b/danger/gemfile/Dangerfile new file mode 100644 index 00000000000..3d38959a9d0 --- /dev/null +++ b/danger/gemfile/Dangerfile @@ -0,0 +1,15 @@ +# rubocop:disable Style/SignalException + +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 ` and commit the #{gitlab.html_link("Gemfile.lock")} changes." + ] + + warn msg.join(" ") +end diff --git a/danger/metadata/Dangerfile b/danger/metadata/Dangerfile new file mode 100644 index 00000000000..033a84d6ecd --- /dev/null +++ b/danger/metadata/Dangerfile @@ -0,0 +1,25 @@ +# rubocop:disable Style/SignalException + +if gitlab.mr_body.size < 5 + fail "Please provide a merge request description." +end + +if gitlab.mr_labels.empty? + fail "Please add labels to this merge request." +end + +unless gitlab.mr_json["assignee"] + warn "This merge request does not have any assignee yet. Setting an assignee clarifies who needs to take action on the merge request at any given time." +end + +has_milestone = !gitlab.mr_json["milestone"].nil? + +unless has_milestone + warn "This merge request does not refer to an existing milestone.", sticky: false +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." +end diff --git a/danger/specs/Dangerfile b/danger/specs/Dangerfile new file mode 100644 index 00000000000..88e64c57a4b --- /dev/null +++ b/danger/specs/Dangerfile @@ -0,0 +1,13 @@ +# rubocop:disable Style/SignalException + +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 +end From eff378ca1a317877f168b76658d81b38d4d9f02e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Mon, 21 May 2018 18:42:12 +0200 Subject: [PATCH 02/14] Add a new fake migration file MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- db/migrate/my_new_migration.rb | 0 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 db/migrate/my_new_migration.rb diff --git a/db/migrate/my_new_migration.rb b/db/migrate/my_new_migration.rb new file mode 100644 index 00000000000..e69de29bb2d From 867018aab4fe2a305ebc58193da5c19e72827ddf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Mon, 21 May 2018 18:42:59 +0200 Subject: [PATCH 03/14] Add a fake change to Gemfile MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- Gemfile | 2 ++ 1 file changed, 2 insertions(+) diff --git a/Gemfile b/Gemfile index d575568adaa..f608937ecd4 100644 --- a/Gemfile +++ b/Gemfile @@ -437,3 +437,5 @@ gem 'grape_logging', '~> 1.7' # Asset synchronization gem 'asset_sync', '~> 2.4' + +# foo From 1915c9f0cf1ec3e836c02ba57c97e5a9a61d487b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 10 Jul 2018 11:30:16 +0200 Subject: [PATCH 04/14] Moving 400 lines of code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- app/models/project.rb | 301 ++++++++++++++++++++++++++++++++++++++++++ app/models/user.rb | 27 ++-- 2 files changed, 315 insertions(+), 13 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 770262f6193..2cc0c83d064 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -2182,5 +2182,306 @@ class Project < ActiveRecord::Base else check_access.call end + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + + end end diff --git a/app/models/user.rb b/app/models/user.rb index 1c5d39db118..4aeaaedfe4d 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -441,6 +441,20 @@ class User < ActiveRecord::Base [:ghost] end + def full_website_url + return "http://#{website_url}" if website_url !~ %r{\Ahttps?://} + + website_url + end + + def short_website_url + website_url.sub(%r{\Ahttps?://}, '') + end + + def all_ssh_keys + keys.map(&:publishable_key) + end + def internal? self.class.internal_attributes.any? { |a| self[a] } end @@ -845,19 +859,6 @@ class User < ActiveRecord::Base project.project_member(self) end - def full_website_url - return "http://#{website_url}" if website_url !~ %r{\Ahttps?://} - - website_url - end - - def short_website_url - website_url.sub(%r{\Ahttps?://}, '') - end - - def all_ssh_keys - keys.map(&:publishable_key) - end def temp_oauth_email? email.start_with?('temp-email-for-oauth') From dc629bb6b8146477fdbf9fcd11d10ebedc785029 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 10 Jul 2018 11:49:17 +0200 Subject: [PATCH 05/14] Modify locale/gitlab.pot for debugging purpose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- locale/gitlab.pot | 21 ++++++++------------- 1 file changed, 8 insertions(+), 13 deletions(-) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 83ff735580e..984753cffe5 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -16,6 +16,11 @@ msgstr "" "Content-Transfer-Encoding: 8bit\n" "Plural-Forms: nplurals=INTEGER; plural=EXPRESSION;\n" +msgid "Adding 4 lines" +msgid_plural "%d changed files" +msgstr[0] "" +msgstr[1] "" + msgid "%d changed file" msgid_plural "%d changed files" msgstr[0] "" @@ -31,21 +36,11 @@ msgid_plural "%d commits behind" msgstr[0] "" msgstr[1] "" -msgid "%d exporter" -msgid_plural "%d exporters" -msgstr[0] "" -msgstr[1] "" - msgid "%d issue" msgid_plural "%d issues" msgstr[0] "" msgstr[1] "" -msgid "%d layer" -msgid_plural "%d layers" -msgstr[0] "" -msgstr[1] "" - msgid "%d merge request" msgid_plural "%d merge requests" msgstr[0] "" @@ -1434,14 +1429,14 @@ msgstr "" msgid "Comment & unresolve discussion" msgstr "" -msgid "Comments" -msgstr "" - msgid "Commit" msgid_plural "Commits" msgstr[0] "" msgstr[1] "" +msgid "Comments" +msgstr "" + msgid "Commit (%{commit_count})" msgid_plural "Commits (%{commit_count})" msgstr[0] "" From ab87e7bab1d5cc20c7b69644843bfcb1f3f16918 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Tue, 10 Jul 2018 12:10:54 +0200 Subject: [PATCH 06/14] Improve Danger files after first review MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- .gitlab-ci.yml | 8 +++-- danger/changelog/Dangerfile | 53 +++++++++++++++++++++++++--------- danger/changes_size/Dangerfile | 20 +++++++++---- danger/database/Dangerfile | 37 +++++++++++++++++------- danger/gemfile/Dangerfile | 27 ++++++++++++----- danger/metadata/Dangerfile | 4 +-- danger/specs/Dangerfile | 13 +++++---- 7 files changed, 115 insertions(+), 47 deletions(-) diff --git a/.gitlab-ci.yml b/.gitlab-ci.yml index 576a96f66f4..137c26d7dae 100644 --- a/.gitlab-ci.yml +++ b/.gitlab-ci.yml @@ -355,9 +355,13 @@ danger-review: - source scripts/utils.sh - retry gem install danger --no-ri --no-rdoc cache: {} + only: + refs: + - branches@gitlab-org/gitlab-ce + - branches@gitlab-org/gitlab-ee except: - - tags - - master + variables: + - $CI_COMMIT_REF_NAME =~ /^ce-to-ee-.*/ script: - git version - danger --fail-on-errors=true 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 %s +``` + +If your merge request doesn't warrant a CHANGELOG entry, +consider adding any of the %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. -if git.lines_of_code > 5_000 - fail "This merge request is definitely too big, please split it into multiple merge requests." +# 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 > 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? +ANY_MIGRATIONS_REGEX = %r{\A(ee/)?(db/(geo/)?(post_)?migrate)/} +SCHEMA_NOT_UPDATED_MESSAGE = <<~MSG +**New %s added but %s wasn't updated.** -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)." +Usually, when adding new %s, %s should be +updated too (unless the migration isn't changing the DB schema +and isn't the most recent one). +MSG - warn msg.join(" ") +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 +**%s was updated but %s wasn't updated.** + +Usually, when %s is updated, you should run +``` +bundle install && \ + BUNDLE_GEMFILE=Gemfile.rails5 bundle install +``` + +or + +``` +bundle update +``` + +and commit the %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 ` 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 From ced15dd53169d54f1d6c32addf8b0a331227ed09 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Wed, 11 Jul 2018 14:48:31 +0200 Subject: [PATCH 07/14] Tweak the Danger settings for DB changes Instead of only checking for migrations, we now check for a variety of files and directories that require a database review. We also include some steps on how to make sure changes are reviewed. --- danger/database/Dangerfile | 69 +++++++++++++++++++++++++++++++++----- 1 file changed, 61 insertions(+), 8 deletions(-) diff --git a/danger/database/Dangerfile b/danger/database/Dangerfile index 4759ceb7eb4..eea84fcdaab 100644 --- a/danger/database/Dangerfile +++ b/danger/database/Dangerfile @@ -1,6 +1,23 @@ +# frozen_string_literal: true # rubocop:disable Style/SignalException -ANY_MIGRATIONS_REGEX = %r{\A(ee/)?(db/(geo/)?(post_)?migrate)/} +# All the files/directories that should be reviewed by the DB team. +DB_FILES = [ + 'db/', + 'app/models/project_authorization.rb', + 'app/services/users/refresh_authorized_projects_service.rb', + 'lib/gitlab/background_migration.rb', + 'lib/gitlab/background_migration/', + 'lib/gitlab/database.rb', + 'lib/gitlab/database/', + 'lib/gitlab/github_import.rb', + 'lib/gitlab/github_import/', + 'lib/gitlab/sql/', + 'rubocop/cop/migration', + 'ee/db/', + 'ee/lib/gitlab/database/' +].freeze + SCHEMA_NOT_UPDATED_MESSAGE = <<~MSG **New %s added but %s wasn't updated.** @@ -9,19 +26,28 @@ updated too (unless the migration isn't changing the DB schema and isn't the most recent one). MSG +def database_paths_requiring_review(files) + to_review = [] + + files.each do |file| + review = DB_FILES.any? do |pattern| + file.start_with?(pattern) + end + + to_review << file if review + end + + to_review +end + +all_files = git.added_files + git.modified_files + 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 @@ -29,3 +55,30 @@ 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 + +db_paths_to_review = database_paths_requiring_review(all_files) + +unless db_paths_to_review.empty? + message 'This merge request adds or changes files that require a ' \ + 'review from the Database team.' + + markdown(<<~MARKDOWN.strip) +## Database Review + +The following files require a review from the Database Team: + +* #{db_paths_to_review.join("\n* ")} + +To make sure these changes are reviewed, take the following steps: + +1. Edit your merge request, and add `gl-database` to the list of Group + approvers. +1. Mention `@gl-database` in a separate comment, and explain what needs to be + reviewed by the team. Please don't mention the team until your changes are + ready for review. + MARKDOWN + + unless gitlab.mr_labels.include?('database') + warn 'This merge request is missing the ~database label.' + end +end From b851fe8635d8c9603ce827dac23decccdf8b6508 Mon Sep 17 00:00:00 2001 From: Yorick Peterse Date: Wed, 11 Jul 2018 14:51:48 +0200 Subject: [PATCH 08/14] Fix "Database Team" typo for Danger --- danger/database/Dangerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/danger/database/Dangerfile b/danger/database/Dangerfile index eea84fcdaab..5d9b53fe4f0 100644 --- a/danger/database/Dangerfile +++ b/danger/database/Dangerfile @@ -65,7 +65,7 @@ unless db_paths_to_review.empty? markdown(<<~MARKDOWN.strip) ## Database Review -The following files require a review from the Database Team: +The following files require a review from the Database team: * #{db_paths_to_review.join("\n* ")} From e1578effd9363d20395f1104c4f7471249b854d4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 11 Jul 2018 18:53:45 +0200 Subject: [PATCH 09/14] Revert "Add a new fake migration file" This reverts commit eff378ca1a317877f168b76658d81b38d4d9f02e. --- db/migrate/my_new_migration.rb | 0 1 file changed, 0 insertions(+), 0 deletions(-) delete mode 100644 db/migrate/my_new_migration.rb diff --git a/db/migrate/my_new_migration.rb b/db/migrate/my_new_migration.rb deleted file mode 100644 index e69de29bb2d..00000000000 From ffba5ce93646eb8e42ceddcec94b6797b3efb471 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 11 Jul 2018 18:53:52 +0200 Subject: [PATCH 10/14] Revert "Add a fake change to Gemfile" This reverts commit 867018aab4fe2a305ebc58193da5c19e72827ddf. --- Gemfile | 2 -- 1 file changed, 2 deletions(-) diff --git a/Gemfile b/Gemfile index f608937ecd4..d575568adaa 100644 --- a/Gemfile +++ b/Gemfile @@ -437,5 +437,3 @@ gem 'grape_logging', '~> 1.7' # Asset synchronization gem 'asset_sync', '~> 2.4' - -# foo From 644135b9c4348dd7d45eaa5684d2c67a8b015b02 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 11 Jul 2018 18:53:59 +0200 Subject: [PATCH 11/14] Revert "Moving 400 lines of code" This reverts commit 1915c9f0cf1ec3e836c02ba57c97e5a9a61d487b. --- app/models/project.rb | 301 ------------------------------------------ app/models/user.rb | 27 ++-- 2 files changed, 13 insertions(+), 315 deletions(-) diff --git a/app/models/project.rb b/app/models/project.rb index 2cc0c83d064..770262f6193 100644 --- a/app/models/project.rb +++ b/app/models/project.rb @@ -2182,306 +2182,5 @@ class Project < ActiveRecord::Base else check_access.call end - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - - end end diff --git a/app/models/user.rb b/app/models/user.rb index 4aeaaedfe4d..1c5d39db118 100644 --- a/app/models/user.rb +++ b/app/models/user.rb @@ -441,20 +441,6 @@ class User < ActiveRecord::Base [:ghost] end - def full_website_url - return "http://#{website_url}" if website_url !~ %r{\Ahttps?://} - - website_url - end - - def short_website_url - website_url.sub(%r{\Ahttps?://}, '') - end - - def all_ssh_keys - keys.map(&:publishable_key) - end - def internal? self.class.internal_attributes.any? { |a| self[a] } end @@ -859,6 +845,19 @@ class User < ActiveRecord::Base project.project_member(self) end + def full_website_url + return "http://#{website_url}" if website_url !~ %r{\Ahttps?://} + + website_url + end + + def short_website_url + website_url.sub(%r{\Ahttps?://}, '') + end + + def all_ssh_keys + keys.map(&:publishable_key) + end def temp_oauth_email? email.start_with?('temp-email-for-oauth') From f5b64bee94f741ef8b1608567b774870278c7ac5 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 11 Jul 2018 18:54:06 +0200 Subject: [PATCH 12/14] Revert "Modify locale/gitlab.pot for debugging purpose" This reverts commit dc629bb6b8146477fdbf9fcd11d10ebedc785029. --- locale/gitlab.pot | 21 +++++++++++++-------- 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/locale/gitlab.pot b/locale/gitlab.pot index 984753cffe5..83ff735580e 100644 --- a/locale/gitlab.pot +++ b/locale/gitlab.pot @@ -16,11 +16,6 @@ msgstr "" "Content-Transfer-Encoding: 8bit\n" "Plural-Forms: nplurals=INTEGER; plural=EXPRESSION;\n" -msgid "Adding 4 lines" -msgid_plural "%d changed files" -msgstr[0] "" -msgstr[1] "" - msgid "%d changed file" msgid_plural "%d changed files" msgstr[0] "" @@ -36,11 +31,21 @@ msgid_plural "%d commits behind" msgstr[0] "" msgstr[1] "" +msgid "%d exporter" +msgid_plural "%d exporters" +msgstr[0] "" +msgstr[1] "" + msgid "%d issue" msgid_plural "%d issues" msgstr[0] "" msgstr[1] "" +msgid "%d layer" +msgid_plural "%d layers" +msgstr[0] "" +msgstr[1] "" + msgid "%d merge request" msgid_plural "%d merge requests" msgstr[0] "" @@ -1429,14 +1434,14 @@ msgstr "" msgid "Comment & unresolve discussion" msgstr "" +msgid "Comments" +msgstr "" + msgid "Commit" msgid_plural "Commits" msgstr[0] "" msgstr[1] "" -msgid "Comments" -msgstr "" - msgid "Commit (%{commit_count})" msgid_plural "Commits (%{commit_count})" msgstr[0] "" From 545c961dbccad9eefbdc44c68b6c82b4e4699661 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?R=C3=A9my=20Coutable?= Date: Wed, 11 Jul 2018 19:12:44 +0200 Subject: [PATCH 13/14] Wrap DB paths in backticks in the Danger check MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Signed-off-by: Rémy Coutable --- danger/database/Dangerfile | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/danger/database/Dangerfile b/danger/database/Dangerfile index 5d9b53fe4f0..2328a6c1733 100644 --- a/danger/database/Dangerfile +++ b/danger/database/Dangerfile @@ -67,7 +67,7 @@ unless db_paths_to_review.empty? The following files require a review from the Database team: -* #{db_paths_to_review.join("\n* ")} +* #{db_paths_to_review.map { |path| "`#{path}`" }.join("\n* ")} To make sure these changes are reviewed, take the following steps: From 27843cd2e7b290dd79483e3d11b1bb1bd8a6d78e Mon Sep 17 00:00:00 2001 From: Robert Speicher Date: Wed, 11 Jul 2018 14:44:35 -0500 Subject: [PATCH 14/14] Autocorrect RuboCop violations in danger/**/Dangerfile --- danger/changelog/Dangerfile | 6 +++--- danger/changes_size/Dangerfile | 2 -- danger/database/Dangerfile | 1 - danger/gemfile/Dangerfile | 4 +--- danger/specs/Dangerfile | 4 +--- 5 files changed, 5 insertions(+), 12 deletions(-) diff --git a/danger/changelog/Dangerfile b/danger/changelog/Dangerfile index b4a2f7b964c..76ebe57cf8d 100644 --- a/danger/changelog/Dangerfile +++ b/danger/changelog/Dangerfile @@ -2,9 +2,9 @@ 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 +NO_CHANGELOG_LABELS = %w[backstage QA test].freeze +SEE_DOC = "See [the documentation](https://docs.gitlab.com/ce/development/changelog.html).".freeze +MISSING_CHANGELOG_MESSAGE = <<~MSG.freeze **[CHANGELOG missing](https://docs.gitlab.com/ce/development/changelog.html).** You can create one with: diff --git a/danger/changes_size/Dangerfile b/danger/changes_size/Dangerfile index 597073e6029..8251d0d5cbf 100644 --- a/danger/changes_size/Dangerfile +++ b/danger/changes_size/Dangerfile @@ -1,5 +1,3 @@ -# rubocop:disable Style/SignalException - # 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: diff --git a/danger/database/Dangerfile b/danger/database/Dangerfile index 2328a6c1733..6f48994945a 100644 --- a/danger/database/Dangerfile +++ b/danger/database/Dangerfile @@ -1,5 +1,4 @@ # frozen_string_literal: true -# rubocop:disable Style/SignalException # All the files/directories that should be reviewed by the DB team. DB_FILES = [ diff --git a/danger/gemfile/Dangerfile b/danger/gemfile/Dangerfile index b9f59d95815..8ef4a464fe4 100644 --- a/danger/gemfile/Dangerfile +++ b/danger/gemfile/Dangerfile @@ -1,6 +1,4 @@ -# rubocop:disable Style/SignalException - -GEMFILE_LOCK_NOT_UPDATED_MESSAGE = <<~MSG +GEMFILE_LOCK_NOT_UPDATED_MESSAGE = <<~MSG.freeze **%s was updated but %s wasn't updated.** Usually, when %s is updated, you should run diff --git a/danger/specs/Dangerfile b/danger/specs/Dangerfile index 0ed5235ec02..934ea0beadb 100644 --- a/danger/specs/Dangerfile +++ b/danger/specs/Dangerfile @@ -1,6 +1,4 @@ -# rubocop:disable Style/SignalException - -NO_NEW_SPEC_MESSAGE = <<~MSG +NO_NEW_SPEC_MESSAGE = <<~MSG.freeze 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.