Trigger note-webhook on comment edit
Problem
Currently, GitLab's note-webhooks are only triggered when comments are created. This limits the ability of external applications to react to and process comment updates.
Proposed Solution
Extend the existing note-webhook functionality to also trigger when comments within merge requests (or issues/snippets) are edited. This will provide a consistent mechanism for external systems to stay synchronized with the latest comment content.
Potential Use Cases
Workflow Automation:
Systems that rely on specific content or keywords within comments could trigger automated actions when a comment is updated.
This is the use case that I and RedHat are interested in.
External Integrations:
3rd party tools can remain tightly synchronized with GitLab discussions, even when comments undergo changes. This is the usecase @hussam789 and PRAgent are interested in.
Signed-off-by: Lucas Zampieri lzampier@redhat.com
Merge request reports
Activity
Hey @lzampier!
Thank you for your contribution to GitLab. Please refer to the contribution documentation for an overview of the process.
Did you know about our community forks? Working from there will make your contribution process easier. Please check it out!
When you're ready for a first review, post
@gitlab-bot ready
. If you know a relevant reviewer(s) (for example, someone that was involved in a related issue), you can also assign them directly with@gitlab-bot ready @user1 @user2
.At any time, if you need help, feel free to post
@gitlab-bot help
or initiate a mentor session on Discord. Read more on how to get help.You can comment
@gitlab-bot label <label1> <label2>
to add labels to your MR. Please see the list of allowed labels in thelabel
command documentation.This message was generated automatically. You're welcome to improve it.
added Community contribution workflowin dev labels
assigned to @lzampier
2 Warnings featureaddition and featureenhancement merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the Technical Writer counterpart.
For more information, see:
- The Handbook page on merge request types.
- The definition of done documentation.
This merge request does not refer to an existing milestone. Reviewer roulette
Category Reviewer Maintainer backend @jwanjohi
(UTC+0)
@harsimarsandhu
(UTC+5.5)
groupimport and integrate (backend) @SamWord
(UTC-4)
Maintainer review is optional for groupimport and integrate (backend) Please check reviewer's status!
Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
DangerEdited by Lucas Zampierimentioned in issue gitlab-org/quality/triage-reports#13335 (closed)
added backend featureenhancement groupproject management typefeature labels
added devopsplan sectiondev labels
Setting label(s) Category:Team Planning based on groupproject management.
added Category:Team Planning label
added 870 commits
-
85d62687...b534caf9 - 869 commits from branch
gitlab-org:master
- 13a675e3 - Trigger note-webhook on comment edit
-
85d62687...b534caf9 - 869 commits from branch
added 3 commits
-
13a675e3...6d5fe525 - 2 commits from branch
gitlab-org:master
- f28df728 - Trigger note-webhook on comment edit
-
13a675e3...6d5fe525 - 2 commits from branch
@lzampier, it seems we're waiting on an action from you for approximately two weeks.
- Do you still have capacity to work on this? If not, you might want to close this MR and/or ask someone to take over.
- Do you need help in getting it ready? At any time, you can:
- If you're actually ready for a review, you can post
@gitlab-bot ready
.
This message was generated automatically. You're welcome to improve it.
added automation:author-reminded label
added 23459 commits
-
f28df728...4b836b80 - 23458 commits from branch
gitlab-org:master
- ad7bb68c - Trigger note-webhook on comment edit
-
f28df728...4b836b80 - 23458 commits from branch
added 4354 commits
-
ad7bb68c...55ee4f63 - 4353 commits from branch
gitlab-org:master
- 240ccf96 - Trigger note-webhook on comment edit
-
ad7bb68c...55ee4f63 - 4353 commits from branch
- Resolved by Carla Drago
Hi @lzampier, We are from PRAgent.
We are interested in getting a webhook when a Merge request comment (note) is edited or a checkbox is clicked, for implementing interactive MR actions (see similar ability in Github https://www.codium.ai/images/pr_agent/pr-actions.mp4).
- Is this kind of webhook already implemented? we tried, and we didn't receive an event when editing a comment.
- Will this MR enable it? Is there a timeline for merging it?
Thanks a lot Codium Team
Edited by hussam lawen
added 6684 commits
-
240ccf96...21aba482 - 6683 commits from branch
gitlab-org:master
- dae0f6be - Trigger note-webhook on comment edit
-
240ccf96...21aba482 - 6683 commits from branch
added 257 commits
-
dc0531dd...87c086b3 - 256 commits from branch
gitlab-org:master
- dd4f6365 - Trigger note-webhook on comment edit
-
dc0531dd...87c086b3 - 256 commits from branch
@gitlab-bot ready
added workflowready for review label and removed workflowin dev label
Hi Coach
@oksanakohuch-ext
, this Community contribution is ready for review or needs your coaching.- Do you have capacity and domain expertise to review this? If not, find one or more reviewers and assign to them.
- If you've reviewed it, add the workflowin dev label if these changes need more work before the next review.
This message was generated automatically. You're welcome to improve it.
removed automation:author-reminded label
requested review from @oksanakohuch-ext
added 115 commits
-
dd4f6365...3ec753ba - 114 commits from branch
gitlab-org:master
- b7a02841 - Trigger note-webhook on comment edit
-
dd4f6365...3ec753ba - 114 commits from branch
- Resolved by Oksana Kohuch
Hi, @lzampier
Thanks for working on this! I left one small question otherwise looks good to me
- Resolved by Oksana Kohuch
added pipeline:mr-approved label
- Resolved by Carla Drago
@oksanakohuch-ext
, thanks for approving this merge request.This is the first time the merge request has been approved. To ensure we don't only run predictive pipelines, and we don't break
master
, please start a new pipeline before merging.Please wait for the pipeline to start before resolving this discussion and set auto-merge for the new pipeline. See merging a merge request for more details.
Hi @jnutt
Could you do a groupimport and integrate review?Hey @lzampier! Thank you so much for this contribution.
I think I've found an issue, which appears to be a result of your being misled by the existing test suite. I've expanded on in an inline comment.
Please let me know if I can assist in any way.
@carlad-gl, do you have capacity to perform the backend maintainer review?
Particularly, what do you think about the functional aspect of this change? While the webhook payload contains a unique ID that consumers can track, I'm concerned that there's nothing obvious in the hook payload that says "this is a new" vs. "this is an update". Are there existing workflows that might be broken if we start sending hooks for updates? Would we benefit from including a more explicit new vs. update field?
Otherwise, LGTM.
@lzampier thanks for the work here. A small nitpick is that it's always good to add context to the description of the MR: What changes have been made, why the changes are required. Reading through the comments I can see the comment from PRAgent. This info would be great to have in the description as it's easy to miss in the comment chain.
Picking up on @jnutt 's concern, I'm not super familiar with the webhooks domain yet so would have to dig into any potential workflow breakages. I gather that PrAgent want something similar to GitHub's webhook for edited issue/pr comment? And I note in the GitHub docs that they include an
action
key in the payload that has a valueedited
.@jnutt suggests we could add something to the event payload. We handle updates in our [Merge Request webhook](Merge Requests: https://docs.gitlab.com/ee/user/project/integrations/webhook_events.html#merge-request-events) with an
object_attributes.action
key. It specifies the type of event (egupdate
).We could start by adding an
action
key toobject_attributes
and see if anything breaks in the specs, though we'd only likely see workflow repercussions in the end-to-end tests.@bmarjanovic is OOO at the moment, back next week. He's done more work in the webhook space. Do you have any thiought here @bmarjanovic ?
@carlad-gl yeah, sorry I was being lazy, let me fix the description/commits.
@jnutt @carlad-gl I think the 'action' type field is a valuable addition, but it might be best handled separately. Can we focus on fixing the immediate issue with note editions for PRagent and RedHat in this MR, and schedule the field implementation for a follow-up? I'm willing to contribute the 'action' field, if you agree.
@lzampier I've had a chat internally with some other engineers. It should be fine to add another field to the payload, so if you're up for adding that please go ahead in a follow-up MR.
@jnutt pointed out that webhook consumers are prompted to be ready for duplicate events
So I'll approve this. Thanks for the patience.
Edited by Carla Drago@carlad-gl Thank you, I've opened !147660 (closed) to work on the action field.
@lzampier Thank you! I'm happy to add this property. I've opened an MR to add the
object_attributes.action
property here: !147856 (merged).
requested review from @jnutt
- Resolved by Carla Drago
requested review from @carlad-gl and removed review request for @jnutt
changed milestone to %16.11
1 Warning featureaddition and featureenhancement merge requests normally have a documentation change. Consider adding a documentation update or confirming the documentation plan with the Technical Writer counterpart.
For more information, see:
- The Handbook page on merge request types.
- The definition of done documentation.
Reviewer roulette
Category Reviewer Maintainer backend @praba.m7n
(UTC-7)
@vyaklushin
(UTC+1)
groupimport and integrate (backend) @SamWord
(UTC-4)
Maintainer review is optional for groupimport and integrate (backend) Please check reviewer's status!
Please refer to documentation page for guidance on how you can benefit from the Reviewer Roulette, or use the GitLab Review Workload Dashboard to find other available reviewers.
If needed, you can retry the
danger-review
job that generated this comment.Generated by
Dangerenabled an automatic merge when the pipeline for b82197ca succeeds
E2E Test Result Summary
allure-report-publisher
generated test report!e2e-test-on-gdk:
test report for b0b318e3expand test summary
+------------------------------------------------------------------+ | suites summary | +-------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +-------------+--------+--------+---------+-------+-------+--------+ | Create | 8 | 0 | 3 | 0 | 11 | ✅ | | Plan | 51 | 0 | 2 | 0 | 53 | ✅ | | Package | 0 | 0 | 1 | 0 | 1 | ➖ | | Data Stores | 2 | 0 | 0 | 0 | 2 | ✅ | | Govern | 3 | 0 | 0 | 0 | 3 | ✅ | | Monitor | 4 | 0 | 0 | 0 | 4 | ✅ | +-------------+--------+--------+---------+-------+-------+--------+ | Total | 68 | 0 | 6 | 0 | 74 | ✅ | +-------------+--------+--------+---------+-------+-------+--------+
e2e-package-and-test:
test report for b0b318e3expand test summary
+------------------------------------------------------------------+ | suites summary | +-------------+--------+--------+---------+-------+-------+--------+ | | passed | failed | skipped | flaky | total | result | +-------------+--------+--------+---------+-------+-------+--------+ | Plan | 245 | 0 | 19 | 0 | 264 | ✅ | | Create | 145 | 0 | 17 | 0 | 162 | ✅ | | Govern | 3 | 0 | 0 | 0 | 3 | ✅ | | Monitor | 4 | 0 | 0 | 0 | 4 | ✅ | | Data Stores | 2 | 0 | 0 | 0 | 2 | ✅ | | Package | 0 | 0 | 1 | 0 | 1 | ➖ | +-------------+--------+--------+---------+-------+-------+--------+ | Total | 399 | 0 | 37 | 0 | 436 | ✅ | +-------------+--------+--------+---------+-------+-------+--------+
mentioned in merge request !147660 (closed)
@lzampier, how was your code review experience with this merge request? Please tell us how we can continue to iterate and improve:
- React with a
or a on this comment to describe your experience. - Create a new comment starting with
@gitlab-bot feedback
below, and leave any additional feedback you have for us in the comment.
Subscribe to the GitLab Community Newsletter for contributor-focused content and opportunities to level up.
Thanks for your help!
This message was generated automatically. You're welcome to improve it.
- React with a
mentioned in commit 5a4fdbc9
added workflowstaging-canary label and removed workflowready for review label
added workflowcanary label and removed workflowstaging-canary label
added workflowstaging label and removed workflowcanary label
added workflowproduction label and removed workflowstaging label
added workflowpost-deploy-db-production label and removed workflowproduction label
mentioned in commit a8bec6d6
mentioned in merge request !147856 (merged)
mentioned in commit 7ca2704a
mentioned in commit 1d42562e
mentioned in commit 20694519
mentioned in commit 3d39d22e
mentioned in merge request gitlab-com/www-gitlab-com!133613 (merged)
mentioned in commit abc1b67b
added releasedcandidate label
mentioned in issue cki-project/kernel-workflow#787