Skip to content

Conversation

@ManilDf
Copy link

@ManilDf ManilDf commented May 29, 2025

This PR implements the integration of notifying from Gitlab pipelines when triggering and when they finished.

@Half-Shot Half-Shot self-requested a review May 30, 2025 20:19
@Half-Shot
Copy link
Contributor

This is super exciting! Massive props to doing comprehensive tests for this too :)

Signed-off-by: madjidDer <[email protected]>
@Half-Shot
Copy link
Contributor

@ManilDf are you happy for this to be undrafted?

Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

This is really, really fantastic folks. Thank you for spending the time to do some proper tests for this and getting this to 99% of the way.

So I just have a few minor nits, mainly that we need to support the subtypes of pipeline so that users may filter on which kinds of pipeline event. E.g. We like to use .failed a lot at Element because we can detect failing automations.

Otherwise, keep it up and ping me when ready!

const { status, ref, duration, id: pipelineId } = event.object_attributes;

const statusUpper = status.toUpperCase();
if (!this.notifiedPipelines.has(pipelineId)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this likely to trigger multiple times?

Copy link
Author

Choose a reason for hiding this comment

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

Thank you for your comment. You're right — the handler is currently being called multiple times due to the various statuses within a single pipeline.

To address this, we’ll create separate handlers for each pipeline sub-type (e.g., pipeline.success, pipeline.triggered). This will allow us to remove the conditional check currently in place

| "release"
| "release.created";
| "release.created"
| "pipeline";
Copy link
Contributor

Choose a reason for hiding this comment

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

According to the docs we have:

  - pipeline.triggered \*
  - pipeline.canceled \*
  - pipeline.failed \*
  - pipeline.success \*

so we should include these here.

Copy link
Author

Choose a reason for hiding this comment

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

Alright, we’ll proceed with the update once the individual handlers have been created

}

public async onPipelineEvent(event: IGitLabWebhookPipelineEvent) {
if (this.hookFilter.shouldSkip("pipeline")) {
Copy link
Contributor

Choose a reason for hiding this comment

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

As per docs, we should properly filter on the subtypes of pipeline.*?

Copy link
Author

Choose a reason for hiding this comment

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

Yes, it will also be updated when handlers will be created

};
object_attributes: {
id: number;
status: string;
Copy link
Contributor

Choose a reason for hiding this comment

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

Would it be possible to enumerate all the possible values of status here? E.g.

Suggested change
status: string;
status: "triggered"|"success";

etc etc?


expect(intent.sentEvents[1].content.body).to.include("SUCCESS");
expect(intent.sentEvents[1].content.formatted_body).to.include(
'<font color="green"><b>SUCCESS</b></font>',
Copy link
Contributor

Choose a reason for hiding this comment

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

N.B. As per the Matrix spec we now prefer a <span> with data-mx-color https://spec.matrix.org/latest/client-server-api/#mroommessage-msgtypes

@Half-Shot Half-Shot marked this pull request as ready for review June 5, 2025 09:08
@Half-Shot Half-Shot requested a review from a team as a code owner June 5, 2025 09:08
@leguye
Copy link

leguye commented Jun 6, 2025

Hi @Half-Shot,

Thank you for your encouraging feedback and for un-drafting the PR. We are thrilled to see your positive comments.

@ManilDf and @madjidDer are currently occupied with other commitments but plan to address your suggestions next week.

We'll notify you once the updates are ready for review.

ManilDf and others added 9 commits June 6, 2025 20:53
Co-authored-by: Will Hunt <[email protected]>
Signed-off-by: Manil Diaf <[email protected]>
…x-org#1062)

* Add warning for failing to reach HS

* Update types

* fixup test

* tweaks

* lint
…ric hooks (matrix-org#1004)

* Add an option at the config and state level to disable hook bodies.

* add a test

* changelog

* no fake requried

* default to state, then config for data inclusion

* default to including webhook state

* allow undefined

* update new config

* update tests

* document
…/webhook` (matrix-org#1063)

* Refactor and migrate webhook paths forGitHub

* changelog

* begin moving Gitlab

* Test both paths on GitHub

* Add tests for GitLab.

* Move JIRA webhook handling to router

* Update docs with new webhook info.

* Use /webhook for path

* lint
Signed-off-by: Will Hunt <[email protected]>
@Hakim-Lakrout
Copy link

Hakim-Lakrout commented Jun 7, 2025

We have to update the codebase when resolving the conflict to use the new Router.ts in main branch to redirect to the Gitlab events.

@Half-Shot
Copy link
Contributor

The docker fails are #1023 and can be ignored. Sorry about the refactor from underneath you. When the linter passes I'll do another review and merge if all good.

@Half-Shot Half-Shot self-requested a review June 9, 2025 10:20
@Hakim-Lakrout
Copy link

The docker fails are #1023 and can be ignored. Sorry about the refactor from underneath you. When the linter passes I'll do another review and merge if all good.
Hi! Thanks for your reply. We still have a few adjustments to make, but it should be ready for review by the end of the day.

Copy link
Contributor

@Half-Shot Half-Shot left a comment

Choose a reason for hiding this comment

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

Handful of cleanup tasks


public shouldSkip(...hookName: T[]) {
// Should skip if all of the hook names are missing
//console.log("→ shouldSkip called with", hookName, "vs", this.enabledHooks);
Copy link
Contributor

Choose a reason for hiding this comment

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

comment :)

<EventHookCheckbox
enabledHooks={enabledHooks}
hookEventName="pipeline"
onChange={toggleEnabledHook}
Copy link
Contributor

Choose a reason for hiding this comment

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

N.B. needs to have the subcategories like "Merge requests" does above.

@Half-Shot Half-Shot self-requested a review June 12, 2025 14:37
@Hakim-Lakrout
Copy link

Hi @Half-Shot , just following up on this pull request, do you have any updates or feedback?

@Half-Shot
Copy link
Contributor

Hi @Half-Shot , just following up on this pull request, do you have any updates or feedback?

Hey sorry for the delay, I was off from work some of last week. Catching up now.

@leguye
Copy link

leguye commented Sep 1, 2025

Hey sorry for the delay, I was off from work some of last week. Catching up now.

Hi @Half-Shot, just wanted to check in - it’s been a little while since we rebased the PR, and I’d love to see this feature merged as my local users are really waiting for it. Let me know if there’s anything else you’d like me to adjust (happy to rebase again if needed, though hopefully for the last time 😊).

@leguye
Copy link

leguye commented Nov 18, 2025

Hi @Half-Shot 👋

I’ve just synced this PR with the latest main so everything is up to date.

GitHub is currently showing “5 workflows awaiting approval”...
When you have a moment, could you approve them so the checks can run?

Thanks again for all your previous feedback and your time reviewing this! 🙏

@leguye
Copy link

leguye commented Nov 19, 2025

Hi @Half-Shot!

Thank you for triggering the CI.

I’ve taken a look at the failures. As you mentioned a while ago, the Docker/login failure (related to #1023) is expected and can be ignored.

However, the CI is still failing overall, and the issues don’t seem to come from the PR’s code : everything passes on my local setup.

It looks like the remaining failures are caused by the CI environment itself rather than the changes in this PR. If there’s anything I can do to help narrow it down, please let me know.

Thanks!

@leguye
Copy link

leguye commented Nov 26, 2025

Hi @Half-Shot!
Just a gentle ping to see if you’ve had a chance to look at this again.
No rush at all — just keeping it on your radar. Let me know if you need anything on my side!

Thanks again for your time and reviews.

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 this pull request may close these issues.

5 participants