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

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

Closed
MadnessIRL opened this issue Nov 14, 2024 · 5 comments · Fixed by #1194
Closed

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

MadnessIRL opened this issue Nov 14, 2024 · 5 comments · Fixed by #1194

Comments

@MadnessIRL
Copy link
Contributor

MadnessIRL commented Nov 14, 2024

As described in the documentation:
image

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

  • createdAt
  • internal
  • merge_request_diff_head_sha

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 MadnessIRL changed the title Creating a new merge request note does not allow for merge_request_diff_head_sha Creating a new merge request note does not allow for optional parameters Nov 14, 2024
@MadnessIRL
Copy link
Contributor Author

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
Copy link
Collaborator

jmini commented Nov 18, 2024

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
Copy link
Contributor Author

@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

MadnessIRL pushed a commit to MadnessIRL/gitlab4j-api that referenced this issue Nov 18, 2024
…ussionApi merge requests (gitlab4j#1193)

- Replace unmappable characters in javadocs
@jmini
Copy link
Collaborator

jmini commented Nov 18, 2024

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
Copy link
Contributor Author

MadnessIRL commented Nov 18, 2024

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.

MadnessIRL pushed a commit to MadnessIRL/gitlab4j-api that referenced this issue Nov 20, 2024
…ussionApi merge requests (gitlab4j#1193)

- Replace unmappable characters in javadocs
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

Successfully merging a pull request may close this issue.

2 participants