Support commenting on the diff
Closed this issue · 22 comments
Waiting for:
https://gitlab.com/gitlab-org/gitlab-ce/issues/14850
It seems now possible to start a discussion on a specific line in the diff with the position[new_line] and position[old_line] attributes. https://docs.gitlab.com/ce/api/discussions.html#create-new-merge-request-discussion
This plugin is using this library: https://github.com/timols/java-gitlab-api
This functionality needs to be added in that library.
I have added the functionality with timols/java-gitlab-api#313
It should be included in the next release of the library.
Nice!
Are you thinking one new "Discussion" per violation? I'll have to investigate this a bit. Or maby you want to implement the feature here as well?
Yes I think for each violation in the diff one new discussion should be created with the violation as a body. I will create a PR as soon as I can make it work.
Ok. It might be convenient to change the dependency, in the lib, on java-gitlab-api like this during development:
compile 'com.github.timols:java-gitlab-api:master'
To get it using Jitpack.
Some more notes:
shouldCreateSingleFileCommentneeds to be implemented with an attribute: https://github.com/tomasbjerre/violation-comments-to-gitlab-lib/blob/master/src/main/java/se/bjurr/violations/comments/gitlab/lib/GitLabCommentsProvider.java#L191createSingleFileCommentneeds to be re-implemented. It is just an old experiment now: https://github.com/tomasbjerre/violation-comments-to-gitlab-lib/blob/master/src/main/java/se/bjurr/violations/comments/gitlab/lib/GitLabCommentsProvider.java#L103- And to understand what this is, here is where it is invoked: https://github.com/tomasbjerre/violation-comments-lib/blob/master/src/main/java/se/bjurr/violations/comments/lib/CommentsCreator.java#L59
Did some adjustments in a feature branch: https://github.com/jenkinsci/violation-comments-to-gitlab-plugin/tree/feature/comment-on-diff
@bonzani I'm getting comments like this. Was that the intention?
I was expecting the comments to be on the diff, like this:
I found this: https://docs.gitlab.com/ee/api/discussions.html#create-new-merge-request-discussion
Perhaps part of the solution is supplying "type":"DiffNote", but my comments still show up as just ordinary comments.
I was expecting that too. I digged into the problem and found, that for the discussion to be of type DiffNote, the following arguments have to be supplied:
body, position[new_line], position[position_type], position[new_line], position[old_line], position[new_path], position[old_path], position[base_sha], position[start_sha], position[head_sha]
I think at the new_path, old_path, old_line parameters need to be set correctly.
I will create a PR as soon as I have some free time to spend on it.
I started fiddling with it with a script here: https://github.com/tomasbjerre/violation-comments-to-gitlab-lib/tree/master/sandbox
I made a few adjustments to your sandbox script in the PR such that it will work.
Great!
I can set old path and new path correctly now. But 2 things is yet to solve:
- Setting oldLine correctly.
- Posting it as query params.
I need to think about a solution for the oldLine problem, but I created timols/java-gitlab-api#323 to solve the query params issue.
Perhaps it is possible from parsing the diff. I did not have time to finish it. tomasbjerre/violation-comments-to-gitlab-lib@ecdb925
Nice, I will look into it next time I have some spare time.
Just that it does not get lost: When a new line is added, the oldLine parameter needs to be null (then it does not get added to the request).
I found a way to get oldLine from the diff:
https://github.com/tomasbjerre/violation-comments-to-gitlab-lib/blob/master/src/main/java/se/bjurr/violations/comments/gitlab/lib/GitLabCommentsProvider.java#L122
https://github.com/tomasbjerre/violation-comments-lib/blob/master/src/main/java/se/bjurr/violations/comments/lib/PatchParser.java#L43
I got it working now with the Jenkins plugin!
Very nice! I am happily looking forward to use it in the near future.
Released in 2.12 now.