Skip to content
Snippets Groups Projects

Trigger note-webhook on comment edit

Merged Lucas Zampieri requested to merge lzampier/gitlab:webhooks-on-comment-edit into master
1 unresolved thread

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

Edited by Lucas Zampieri

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
  • added 1 commit

    • a1faaf62 - Trigger note-webhook on comment edit

    Compare with previous version

  • Oksana Kohuch approved this merge request

    approved this merge request

    • Hey @lzampier! Thank you so much for this contribution. :slight_smile:

      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. :slight_smile:

    • @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 value edited.

      @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 (eg update).

      We could start by adding an action key to object_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 ?

    • Author Contributor

      @carlad-gl yeah, sorry I was being lazy, let me fix the description/commits.

    • Author Contributor

      @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 CleanShot_2024-03-22_at_11.00.05_2x So I'll approve this. Thanks for the patience.

      Edited by Carla Drago
    • Author Contributor

      @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).

    • Please register or sign in to reply
  • Oksana Kohuch requested review from @jnutt

    requested review from @jnutt

  • James Nutt
  • added 1 commit

    Compare with previous version

  • James Nutt approved this merge request

    approved this merge request

  • James Nutt requested review from @carlad-gl and removed review request for @jnutt

    requested review from @carlad-gl and removed review request for @jnutt

  • added 1 commit

    • b0b318e3 - Trigger note-webhook on comment edit

    Compare with previous version

  • Lucas Zampieri changed the description

    changed the description

  • Carla Drago approved this merge request

    approved this merge request

  • Carla Drago changed milestone to %16.11

    changed milestone to %16.11

  • Carla Drago resolved all threads

    resolved all threads

  • 1 Warning
    :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:

    Reviewer roulette

    Category Reviewer Maintainer
    backend @praba.m7n profile link current availability (UTC-7) @vyaklushin profile link current availability (UTC+1)
    groupimport and integrate (backend) @SamWord profile link current availability (UTC-4) Maintainer review is optional for groupimport and integrate (backend)

    Please check reviewer's status!

    • available Reviewer is available!
    • unavailable Reviewer is unavailable!

    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 :repeat: danger-review job that generated this comment.

    Generated by :no_entry_sign: Danger

  • Carla Drago enabled an automatic merge when the pipeline for b82197ca succeeds

    enabled an automatic merge when the pipeline for b82197ca succeeds

  • E2E Test Result Summary

    allure-report-publisher generated test report!

    e2e-test-on-gdk: :white_check_mark: test report for b0b318e3

    expand 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: :white_check_mark: test report for b0b318e3

    expand 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   | ✅     |
    +-------------+--------+--------+---------+-------+-------+--------+
  • Lucas Zampieri mentioned in merge request !147660 (closed)

    mentioned in merge request !147660 (closed)

  • merged

  • @lzampier, how was your code review experience with this merge request? Please tell us how we can continue to iterate and improve:

    1. React with a :thumbsup: or a :thumbsdown: on this comment to describe your experience.
    2. 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! :heart:

    This message was generated automatically. You're welcome to improve it.

  • Carla Drago mentioned in commit 5a4fdbc9

    mentioned in commit 5a4fdbc9

  • added workflowstaging label and removed workflowcanary label

  • Luke Duncalfe mentioned in commit a8bec6d6

    mentioned in commit a8bec6d6

  • Luke Duncalfe mentioned in merge request !147856 (merged)

    mentioned in merge request !147856 (merged)

  • Luke Duncalfe mentioned in commit 7ca2704a

    mentioned in commit 7ca2704a

  • Luke Duncalfe mentioned in commit 1d42562e

    mentioned in commit 1d42562e

  • Luke Duncalfe mentioned in commit 20694519

    mentioned in commit 20694519

  • Luke Duncalfe mentioned in commit 3d39d22e

    mentioned in commit 3d39d22e

  • Luke Duncalfe mentioned in commit abc1b67b

    mentioned in commit abc1b67b

  • Please register or sign in to reply
    Loading