gitlab4j / gitlab4j-api

GitLab4J API (gitlab4j-api) provides a full featured Java client library for working with GitLab repositories via the GitLab REST API
MIT License
1.07k stars 463 forks source link

Creating a new merge request note does not allow for optional parameters #1193

Open MadnessIRL opened 1 week ago

MadnessIRL commented 1 week ago

As described in the documentation: image

it should be possible for example to create a note with a commit reference. Sadly, the values:

cannot be set with the current note api:

image

This is a huge blocker for us, I attempted writing my own CURL request with no success.

MadnessIRL commented 1 week ago

Update: we changed the approach by utilising the getDiscussionsApi().createCommitDiscussion()

Once again, the code does not respect the contract of the documentation. image but the API forces position to exist and requires base, head, start Sha and positionType. Once those are set, the line_code will also be required, effectively preventing users to create general commit level discussions. image

With a modified null check, the comment is created successfully and the commit id stored.

jmini commented 4 days ago

Once again, the code does not respect the contract of the documentation.

It is just outdated because the contract has changed. Feel free to provide a pull-request.

MadnessIRL commented 4 days ago

@jmini I can provide a merge request to add createdAt & internal parameters but I have no idea how to push or retrieve merge_request_diff_head_sha

jmini commented 4 days ago

The merge_request_diff_head_sha should be a parameter in the method used to create the note. https://docs.gitlab.com/ee/api/notes.html#create-new-merge-request-note

Required for the /merge quick action. The SHA of the head commit, which ensures the merge request wasn’t updated after the API request was sent.

I would say that when you read a merge request https://docs.gitlab.com/ee/api/merge_requests.html#get-single-mr It corresponds to org.gitlab4j.api.models.MergeRequest#getSha()

But if I understand the docs, it is not required if you are not using the /merge quick action in your comment.

And to trigger a merge on a MR there are other API methods.

So I do not really see a use-case for this merge_request_diff_head_sha parameter.

What do you think?

MadnessIRL commented 4 days ago

I am aware that my case is quite unique and you bring up a valid point.

With the current code, there is no way to obtain the commit sha from an existing comment. My pull request fixes it by allowing general commit comments to store the commitId, but is it the best way?

My use case was to create a merge request thread and then retrieve it based on the commit it was made on. It helped validating that a specific merge request was already reviewed in that specific commit.