Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

New comment on GitLab #1084

Closed
AlexDenisov opened this issue Feb 21, 2019 · 4 comments
Closed

New comment on GitLab #1084

AlexDenisov opened this issue Feb 21, 2019 · 4 comments

Comments

@AlexDenisov
Copy link
Contributor

Report

What did you do?

I ask Danger to always create new comments:

danger --new-comment --fail-on-errors=true --dangerfile=ci/danger/lint.rb

What did you expect to happen?

For each re-run of our CI expected to see a new comment, preserving the old one.
I.e., the merge request history should look like this:

commit: abc
danger: please fix typos
commit: def
danger: looks good to me

What happened instead?

The first comment' body is replaced with a new comment.
I.e., the merge request history looks like this:

commit: abc
danger: please fix typos

And after new commit:

commit: abc
danger: looks good to me
commit: def

Your Environment

  • Which CI are you running on?

    Jenkins

  • Are you running the latest version of Danger?

    5.14.0

  • What is your Dangerfile?

    require 'open3'
    
    def run(title, command)
      stdout, stderr, status = Open3.capture3(command)
      if status.success?
        message "#{title} passed"
      else
        failure #{title} failed
      end
    end
    
    run('sanity checks', 'make sanity_checks')

Additional info

The fix for my expected behaviour is trivial (see below), but the question is whether it is a bug or I misunderstand the purpose of the --new-comment flag?

diff --git a/lib/danger/request_sources/gitlab.rb b/lib/danger/request_sources/gitlab.rb
index 644bb9e1..014a8022 100644
--- a/lib/danger/request_sources/gitlab.rb
+++ b/lib/danger/request_sources/gitlab.rb
@@ -133,7 +133,7 @@ def update_pull_request!(warnings: [], errors: [], messages: [], markdowns: [],
                                  danger_id: danger_id,
                                   template: "gitlab")
 
-          if editable_comments.empty?
+          if editable_comments.empty? or should_create_new_comment
             client.create_merge_request_comment(
               ci_source.repo_slug, ci_source.pull_request_id, body
             )

Thank you for the great tool, it helps a lot!

@orta
Copy link
Member

orta commented Feb 21, 2019

Yep, I agree, I don't think it was probably added right to the GitLab side looking at it's usage in comparison to GitHub

@AlexDenisov
Copy link
Contributor Author

Shall I submit the patch then?

@orta
Copy link
Member

orta commented Feb 21, 2019

Yep, sure 👍

AlexDenisov added a commit to AlexDenisov/danger that referenced this issue Feb 22, 2019
Current implementation of GitLab request source does not create a new comment
when requested via the `--new-comment` CLI option, but rather updates the old
one.

This patch fixes the issue by checking whether new comment is requested
before updating Pull Request.

Preliminary discussion of this issue is here:
danger#1084
AlexDenisov added a commit to AlexDenisov/danger that referenced this issue Feb 22, 2019
Current implementation of GitLab request source does not create a new comment
when requested via the `--new-comment` CLI option, but rather updates the old
one.

This patch fixes the issue by checking whether new comment is requested
before updating Pull Request.

Preliminary discussion of this issue is here:
danger#1084
AlexDenisov added a commit to AlexDenisov/danger that referenced this issue Feb 22, 2019
Current implementation of GitLab request source does not create a new comment
when requested via the `--new-comment` CLI option, but rather updates the old
one.

This patch fixes the issue by checking whether new comment is requested
before updating Pull Request.

Preliminary discussion of this issue is here:
danger#1084
@AlexDenisov
Copy link
Contributor Author

Resolved via #1085

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants