diff --git a/CHANGELOG.md b/CHANGELOG.md index b587cb8e7..ad4e05939 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -10,6 +10,8 @@ ## master +* Fixes GitLab inline comments when violations happened in files outside of the MR diff [@pbendersky](https://github.com/pbendersky), #1092 + ## 5.16.0 * Add support for Visual Studio App Center [@rishabhtayal](https://github.com/rishabhtayal) / [@cojoj](https://github.com/cojoj) diff --git a/lib/danger/request_sources/gitlab.rb b/lib/danger/request_sources/gitlab.rb index 4a7bec523..78da525ae 100644 --- a/lib/danger/request_sources/gitlab.rb +++ b/lib/danger/request_sources/gitlab.rb @@ -90,11 +90,26 @@ def mr_comments def mr_diff @mr_diff ||= begin - client.merge_request_changes(ci_source.repo_slug, ci_source.pull_request_id) + mr_changes .changes.map { |change| change["diff"] }.join("\n") end end + def mr_changed_paths + @mr_changed_paths ||= begin + mr_changes + .changes.map { |change| change["new_path"] } + end + + @mr_changed_paths + end + + def mr_changes + @mr_changes ||= begin + client.merge_request_changes(ci_source.repo_slug, ci_source.pull_request_id) + end + end + def setup_danger_branches base_branch = self.mr_json.source_branch head_branch = self.mr_json.target_branch @@ -359,10 +374,8 @@ def submit_inline_comments_for_kind!(kind, messages, diff_lines, danger_comments messages.reject do |m| next false unless m.file && m.line - # position = find_position_in_diff diff_lines, m, kind - - # Keep the change if it's line is not in the diff and not in dismiss mode - # next dismiss_out_of_range_messages_for(kind) if position.nil? + # Keep the change it's in a file changed in this diff + next if !mr_changed_paths.include?(m.file) # Once we know we're gonna submit it, we format it if is_markdown_content diff --git a/spec/fixtures/gitlab_api/merge_request_1_changes_response.json b/spec/fixtures/gitlab_api/merge_request_1_changes_response.json index 065d1fc88..e2ccce6cd 100644 --- a/spec/fixtures/gitlab_api/merge_request_1_changes_response.json +++ b/spec/fixtures/gitlab_api/merge_request_1_changes_response.json @@ -9,4 +9,4 @@ Vary: Origin X-Request-Id: S3VHro2VCE8 X-Runtime: 0.076161 -{"id":593728,"iid":1,"project_id":1342007,"title":"Add a","description":"The descriptions is here\r\n\r\n\u003e Danger: ignore \"Developer specific files shouldn't be changed\"\r\n\r\n\u003e Danger: ignore \"Testing\"","state":"opened","created_at":"2016-06-27T11:04:02.114Z","updated_at":"2016-09-19T20:25:31.077Z","merged_by":null,"merged_at":null,"closed_by":null,"closed_at":null,"target_branch":"master","source_branch":"mr-test","upvotes":0,"downvotes":0,"author":{"id":483414,"name":"Hugo Tunius","username":"k0nserv","state":"active","avatar_url":"https://secure.gravatar.com/avatar/7300342f996dcc9d4e22418cc9a70b14?s=80\u0026d=identicon","web_url":"https://gitlab.com/k0nserv"},"assignee":null,"source_project_id":1342007,"target_project_id":1342007,"labels":["test-label"],"work_in_progress":false,"milestone":null,"merge_when_pipeline_succeeds":false,"merge_status":"can_be_merged","sha":"04e58de1fa97502d7e28c1394d471bb8fb1fc4a8","merge_commit_sha":null,"user_notes_count":2,"discussion_locked":null,"should_remove_source_branch":null,"force_remove_source_branch":null,"web_url":"https://gitlab.com/k0nserv/danger-test/merge_requests/1","time_stats":{"time_estimate":0,"total_time_spent":0,"human_time_estimate":null,"human_total_time_spent":null},"squash":false,"subscribed":false,"changes_count":"3","latest_build_started_at":null,"latest_build_finished_at":null,"first_deployed_to_production_at":null,"pipeline":null,"diff_refs":{"base_sha":"0e4db308b6579f7cc733e5a354e026b272e1c076","head_sha":"04e58de1fa97502d7e28c1394d471bb8fb1fc4a8","start_sha":"0e4db308b6579f7cc733e5a354e026b272e1c076"},"merge_error":null,"changes":[{"old_path":"Dangerfile","new_path":"Dangerfile","a_mode":"100644","b_mode":"100644","new_file":false,"renamed_file":false,"deleted_file":false,"diff":"--- a/Dangerfile\n+++ b/Dangerfile\n@@ -1,13 +1,11 @@\n # Sometimes it's a README fix, or something like that - which isn't relevant for\n # including in a project's CHANGELOG for example\n-declared_trivial = pr_title.include? \"#trivial\"\n+declared_trivial = gitlab.pr_title.include? \"#trivial\"\n \n # Make it more obvious that a PR is a work in progress and shouldn't be merged yet\n-warn(\"PR is classed as Work in Progress\") if pr_title.include? \"[WIP]\"\n+warn(\"PR is classed as Work in Progress\") if gitlab.pr_title.include? \"[WIP]\"\n \n-# Warn when there is a big PR\n-warn(\"Big PR\") if lines_of_code \u003e 500\n+warn(\"Test warning\", sticky: false)\n+fail(\"Test error\")\n+message(\"Test message\", sticky: false)\n \n-# Don't let testing shortcuts get into master by accident\n-fail(\"fdescribe left in tests\") if `grep -r fdescribe specs/`.length \u003e 1\n-fail(\"fit left in tests\") if `grep -r \"fit specs/ `.length \u003e 1\n"},{"old_path":"a","new_path":"a","a_mode":"0","b_mode":"100644","new_file":true,"renamed_file":false,"deleted_file":false,"diff":"--- /dev/null\n+++ b/a\n@@ -0,0 +1 @@\n+Danger rocks!\n"},{"old_path":"b","new_path":"b","a_mode":"0","b_mode":"100644","new_file":true,"renamed_file":false,"deleted_file":false,"diff":"--- /dev/null\n+++ b/b\n@@ -0,0 +1 @@\n+Test message please ignore\n"}],"approvals_before_merge":null}XWing-467:danger Pablo$ +{"id":593728,"iid":1,"project_id":1342007,"title":"Add a","description":"The descriptions is here\r\n\r\n\u003e Danger: ignore \"Developer specific files shouldn't be changed\"\r\n\r\n\u003e Danger: ignore \"Testing\"","state":"opened","created_at":"2016-06-27T11:04:02.114Z","updated_at":"2016-09-19T20:25:31.077Z","merged_by":null,"merged_at":null,"closed_by":null,"closed_at":null,"target_branch":"master","source_branch":"mr-test","upvotes":0,"downvotes":0,"author":{"id":483414,"name":"Hugo Tunius","username":"k0nserv","state":"active","avatar_url":"https://secure.gravatar.com/avatar/7300342f996dcc9d4e22418cc9a70b14?s=80\u0026d=identicon","web_url":"https://gitlab.com/k0nserv"},"assignee":null,"source_project_id":1342007,"target_project_id":1342007,"labels":["test-label"],"work_in_progress":false,"milestone":null,"merge_when_pipeline_succeeds":false,"merge_status":"can_be_merged","sha":"04e58de1fa97502d7e28c1394d471bb8fb1fc4a8","merge_commit_sha":null,"user_notes_count":2,"discussion_locked":null,"should_remove_source_branch":null,"force_remove_source_branch":null,"web_url":"https://gitlab.com/k0nserv/danger-test/merge_requests/1","time_stats":{"time_estimate":0,"total_time_spent":0,"human_time_estimate":null,"human_total_time_spent":null},"squash":false,"subscribed":false,"changes_count":"3","latest_build_started_at":null,"latest_build_finished_at":null,"first_deployed_to_production_at":null,"pipeline":null,"diff_refs":{"base_sha":"0e4db308b6579f7cc733e5a354e026b272e1c076","head_sha":"04e58de1fa97502d7e28c1394d471bb8fb1fc4a8","start_sha":"0e4db308b6579f7cc733e5a354e026b272e1c076"},"merge_error":null,"changes":[{"old_path":"Dangerfile","new_path":"Dangerfile","a_mode":"100644","b_mode":"100644","new_file":false,"renamed_file":false,"deleted_file":false,"diff":"--- a/Dangerfile\n+++ b/Dangerfile\n@@ -1,13 +1,11 @@\n # Sometimes it's a README fix, or something like that - which isn't relevant for\n # including in a project's CHANGELOG for example\n-declared_trivial = pr_title.include? \"#trivial\"\n+declared_trivial = gitlab.pr_title.include? \"#trivial\"\n \n # Make it more obvious that a PR is a work in progress and shouldn't be merged yet\n-warn(\"PR is classed as Work in Progress\") if pr_title.include? \"[WIP]\"\n+warn(\"PR is classed as Work in Progress\") if gitlab.pr_title.include? \"[WIP]\"\n \n-# Warn when there is a big PR\n-warn(\"Big PR\") if lines_of_code \u003e 500\n+warn(\"Test warning\", sticky: false)\n+fail(\"Test error\")\n+message(\"Test message\", sticky: false)\n \n-# Don't let testing shortcuts get into master by accident\n-fail(\"fdescribe left in tests\") if `grep -r fdescribe specs/`.length \u003e 1\n-fail(\"fit left in tests\") if `grep -r \"fit specs/ `.length \u003e 1\n"},{"old_path":"a","new_path":"a","a_mode":"0","b_mode":"100644","new_file":true,"renamed_file":false,"deleted_file":false,"diff":"--- /dev/null\n+++ b/a\n@@ -0,0 +1 @@\n+Danger rocks!\n"},{"old_path":"b","new_path":"b","a_mode":"0","b_mode":"100644","new_file":true,"renamed_file":false,"deleted_file":false,"diff":"--- /dev/null\n+++ b/b\n@@ -0,0 +1 @@\n+Test message please ignore\n"}],"approvals_before_merge":null} diff --git a/spec/lib/danger/request_sources/gitlab_spec.rb b/spec/lib/danger/request_sources/gitlab_spec.rb index 27b7e55a5..16f7351bf 100644 --- a/spec/lib/danger/request_sources/gitlab_spec.rb +++ b/spec/lib/danger/request_sources/gitlab_spec.rb @@ -331,12 +331,18 @@ end it "adds new comments inline" do + stub_merge_request_changes( + "merge_request_1_changes_response", + "k0nserv\%2Fdanger-test", + 1 + ) + expect(subject.client).to receive(:create_merge_request_discussion) allow(subject.client).to receive(:delete_merge_request_comment) subject.fetch_details - v = Danger::Violation.new("Sure thing", true, "CHANGELOG.md", 4) + v = Danger::Violation.new("Sure thing", true, "a", 4) subject.update_pull_request!(warnings: [], errors: [], messages: [v]) end @@ -346,6 +352,11 @@ "k0nserv%2Fdanger-test", 1 ) + stub_merge_request_changes( + "merge_request_1_changes_response", + "k0nserv\%2Fdanger-test", + 1 + ) v = Danger::Violation.new("Updated danger comment", true, "a", 1) body = subject.generate_inline_comment_body("warning", subject.process_markdown(v, true), danger_id: "danger", template: "gitlab") @@ -391,6 +402,29 @@ v = Danger::Violation.new("Test error", true) subject.update_pull_request!(warnings: [], errors: [v], messages: []) end + + it "skips inline comments for files that are not part of the diff" do + stub_merge_request_discussions( + "merge_request_1_discussions_response", + "k0nserv%2Fdanger-test", + 1 + ) + stub_merge_request_changes( + "merge_request_1_changes_response", + "k0nserv\%2Fdanger-test", + 1 + ) + + allow(subject.client).to receive(:update_merge_request_discussion_note) + allow(subject.client).to receive(:delete_merge_request_comment) + allow(subject.client).to receive(:edit_merge_request_note) + + v = Danger::Violation.new("Error on not-on-diff file", true, "not-on-diff", 1) + + subject.fetch_details + + subject.update_pull_request!(warnings: [v], errors: [], messages: []) + end end end