Skip to content

chore: update gitlab dependencies to reflect latest webhook payload#568

Merged
knative-prow[bot] merged 20 commits into
knative-extensions:mainfrom
xNok:main
Sep 18, 2025
Merged

chore: update gitlab dependencies to reflect latest webhook payload#568
knative-prow[bot] merged 20 commits into
knative-extensions:mainfrom
xNok:main

Conversation

@xNok
Copy link
Copy Markdown
Contributor

@xNok xNok commented May 25, 2025

🧽 Update or clean up current behavior

The issue mostly lies in using the go-playground/webhooks module, as the event parsing is never up-to-date. In fact, I just realised that I last contributed to it, but I no longer intend to do so because it is much better to use the Gitlab-maintained go-gitlab module.

The new approach should make the event Source compatible with all new event types (it was missing quite a few), and update the go-gitlab modules periodically is all you need to do to support the latest Gitlab development.

I didn't apply it, but it would be nice if the events contained an indication of what version of Gitlab or the go-gitlab module it is using as source, as only getting the benefit if they are up-to-date.

Proposed Changes

  • Upgrade go-gitlab (it was relocated to Gitlab)
  • Update the Adapter logic to use the go-gitlab module

Release Note

GitlabSource now support all Gitlab Webhook Event Types to their latest version. 

* [EventTypes](https://gitlab.com/gitlab-org/api/client-go/-/blob/main/types.go?ref_type=heads#L693)
* [EventPayloads](https://gitlab.com/gitlab-org/api/client-go/-/blob/main/event_webhook_types.go?ref_type=heads)

You need to update your event Sink dependency to see the results, which might include migrating from `go-playground/webhooks` to `gitlab.com/gitlab-org/api/client-go`. As `go-playground/webhooks` is not keeping up with Gitlab updates.

Docs

@knative-prow knative-prow Bot requested review from aslom and pierDipi May 25, 2025 10:05
@linux-foundation-easycla
Copy link
Copy Markdown

linux-foundation-easycla Bot commented May 25, 2025

CLA Signed

The committers listed above are authorized under a signed CLA.

@knative-prow
Copy link
Copy Markdown

knative-prow Bot commented May 25, 2025

Welcome @xNok! It looks like this is your first PR to knative-extensions/eventing-gitlab 🎉

@knative-prow knative-prow Bot added the size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files. label May 25, 2025
@PabloPie
Copy link
Copy Markdown

PabloPie commented Jun 25, 2025

Hi @aslom @pierDipi! Do you think you could have a look at this PR? Thank you!

@Cali0707 Cali0707 self-requested a review June 30, 2025 19:48
@codecov
Copy link
Copy Markdown

codecov Bot commented Jul 2, 2025

Codecov Report

❌ Patch coverage is 59.18367% with 20 lines in your changes missing coverage. Please review.
✅ Project coverage is 36.96%. Comparing base (3208f7f) to head (1beb0cc).
⚠️ Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
pkg/adapter/webhook.go 57.44% 14 Missing and 6 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #568      +/-   ##
==========================================
- Coverage   37.09%   36.96%   -0.13%     
==========================================
  Files          19       20       +1     
  Lines         736      752      +16     
==========================================
+ Hits          273      278       +5     
- Misses        454      461       +7     
- Partials        9       13       +4     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Copy Markdown
Member

@Cali0707 Cali0707 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for working on updating this @xNok !

Would you be able to fix the linter errors re: the license boilerplate in the new webhook.go file?

Overall, this PR looks good to me otherwise. Have you managed to test it in a k8s cluster and see if events show up from the webhook?

@xNok
Copy link
Copy Markdown
Contributor Author

xNok commented Jul 9, 2025

Thanks for having a look, I will get back to it on Friday for further testing and wrap this up

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 11, 2025
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 14, 2025
@xNok
Copy link
Copy Markdown
Contributor Author

xNok commented Jul 14, 2025

@Cali0707 I have updated the license in the webhook.go and, more importantly, implemented e2e tests for the webhook reciever part.

I am running the e2e test against the knative quickstart kind cluster, I didn't hook it to the `presubmit-tests.sh

@xNok xNok requested a review from Cali0707 July 14, 2025 08:54
Copy link
Copy Markdown
Member

@Cali0707 Cali0707 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Overall this looks good to me, with a few small nits

/lgtm
/cc @creydr

Comment thread test/e2e_test.go Outdated
Comment thread test/event_tracker.go Outdated
@knative-prow knative-prow Bot requested a review from creydr July 21, 2025 15:08
@knative-prow knative-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Jul 21, 2025
@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Jul 21, 2025
Comment thread pkg/adapter/webhook.go Outdated
@knative-prow knative-prow Bot removed the lgtm Indicates that a PR is ready to be merged. label Jul 22, 2025
Copilot AI review requested due to automatic review settings August 13, 2025 18:28
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 13, 2025
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR modernizes GitLab webhook integration by migrating from the outdated go-playground/webhooks module to the officially maintained gitlab.com/gitlab-org/api/client-go module, ensuring support for all current GitLab webhook event types.

  • Replaces go-playground/webhooks dependency with the official GitLab Go client
  • Updates webhook parsing logic to use GitLab's official event types and payloads
  • Adds comprehensive end-to-end tests with real webhook payload examples

Reviewed Changes

Copilot reviewed 19 out of 267 changed files in this pull request and generated 5 comments.

Show a summary per file
File Description
go.mod Updates dependencies to use gitlab.com/gitlab-org/api/client-go v0.129.0
pkg/adapter/webhook.go New webhook handler implementation using official GitLab client
pkg/adapter/receive_adapter.go Simplified adapter logic using new webhook handler
pkg/adapter/receive_adapter_test.go Updated tests for new event types and error handling
pkg/client/gitlab/webhook_client.go Updated GitLab client usage with new API patterns
pkg/reconciler/source/gitlabsource.go Updated import to use official GitLab client
config/300-gitlabsource.yaml Added support for new event types (feature_flag, job, release, resource_access_token)
test/ New end-to-end integration tests with webhook payload examples

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

Comment thread pkg/adapter/webhook.go Outdated
Comment thread pkg/adapter/webhook.go Outdated
Comment thread test/e2e_test.go
Comment thread config/300-gitlabsource.yaml Outdated
Comment thread config/300-gitlabsource.yaml Outdated
Comment thread pkg/adapter/webhook.go Outdated
@xNok
Copy link
Copy Markdown
Contributor Author

xNok commented Aug 13, 2025

@Cali0707, it took me some time to get back to it, I rebased this MR on main

@knative-prow-robot knative-prow-robot added the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Aug 20, 2025
xNok and others added 19 commits September 18, 2025 10:49
Signed-off-by: Alexandre Couedelo <nokwebspace@gmail.com>
Signed-off-by: Alexandre Couedelo <nokwebspace@gmail.com>
Signed-off-by: Alexandre Couedelo <nokwebspace@gmail.com>
Signed-off-by: Alexandre Couedelo <nokwebspace@gmail.com>
Signed-off-by: Alexandre Couedelo <nokwebspace@gmail.com>
Signed-off-by: Alexandre Couedelo <nokwebspace@gmail.com>
Co-authored-by: Calum Murray <cmurray@redhat.com>
Co-authored-by: Calum Murray <cmurray@redhat.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <175728472+Copilot@users.noreply.github.com>
@knative-prow-robot knative-prow-robot removed the needs-rebase Indicates a PR cannot be merged because it has merge conflicts with HEAD. label Sep 18, 2025
@dprotaso
Copy link
Copy Markdown
Contributor

I rebased the PR

Copy link
Copy Markdown
Member

@Cali0707 Cali0707 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm

Thanks for the PR @xNok

@knative-prow knative-prow Bot added the lgtm Indicates that a PR is ready to be merged. label Sep 18, 2025
@knative-prow
Copy link
Copy Markdown

knative-prow Bot commented Sep 18, 2025

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: Cali0707, xNok

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Details Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@knative-prow knative-prow Bot added the approved Indicates a PR has been approved by an approver from all required OWNERS files. label Sep 18, 2025
@knative-prow knative-prow Bot merged commit 5a40aba into knative-extensions:main Sep 18, 2025
20 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

approved Indicates a PR has been approved by an approver from all required OWNERS files. lgtm Indicates that a PR is ready to be merged. size/XXL Denotes a PR that changes 1000+ lines, ignoring generated files.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants