feat: Gitlab integration #93
Conversation
cfraz89
force-pushed
the
feat/gitlab-integration
branch
from
b07338a
to
f272f51
Compare
| import { Gitlab, GitlabEvent } from "@eventual/integrations-gitlab"; | ||
| import { AWSSecret } from "@eventual/aws-runtime"; | ||
| import { workflow } from "@eventual/core"; | ||
| import { PipelineEvent } from "packages/@eventual/integrations-gitlab/src/index.js"; |
There was a problem hiding this comment.
This is wrong, should be @eventual/integrations-gitlab. One of these days i'm gonna dig into why VS code is fucking stupid about imports since we migrated to ESM
| const { path, events } = gitlab.webhook("repo-1-hook", { | ||
| validationToken: new AWSSecret({ secretId: process.env.REPO_1_HOOK_TOKEN! }), | ||
| }); |
There was a problem hiding this comment.
Looks great - but we'll want to add event-specific webhooks too
There was a problem hiding this comment.
So right now it doesn't configure the webhook on gitlabs end, what hooks it receives depends on how you set up the gitlab side. Is it the same with the slack integration right now? There's some manual setup involved?
There was a problem hiding this comment.
Oh I didn't mean to automatically configure hooks on gitlab. I meant as a router. Fine with this to start
There was a problem hiding this comment.
Ah yeah I was actually wondering if we should add filtering to the event class, do it's a bit like Rx. So rather than defining the hook as a certain event type, you filter those out.
webookEvents
.filter(e=>e.kind == "pipeline")
.on(e=>{...});There was a problem hiding this comment.
What does the filter achieve that can't be done with an if?
If we were going to add layers I'd expect this pattern, not RX
gitlab.pipeline(event => )There was a problem hiding this comment.
What does the filter achieve that can't be done with an if?
If we were going to add layers I'd expect this pattern, not RX
gitlab.pipeline(event => )
Yeah you can do anything with if, it's the same with with any abstraction though, we have list.filter, is not necessary, you could use ifs otherwise, but it's nice, convenient, and clean.
I don't think gitlab.pipeline makes sense in this context, since this side of the code just pulls down events that are configured on the opposite end. Syntax like github.pipeline seems weird when it's entirely possible to configure gitlab to eg only send commit events to that hook. So I think it's good to make it explicit that the hook could receive any event, and you'll need to filter down the events that are actually relevant. It'd be a different story if the code was also configuring the gitlab side, so that gitlab.pipeline means 'configure my repository to send pipeline notifications to this event bus'. Right now it would just mean ' set up an endpoint send I'll go point gitlab to this hook and make sure to configure it to send pipeline notifications and nothing else'
There was a problem hiding this comment.
The bottom level can be a generic hook but we can't infer any information from it and an if. An explicit hook per type of event gives us information we can infer from and it aids in documentation and discoverability.
There was a problem hiding this comment.
It doesn't have to infer to provide value either. People explore libraries my looking at methods.
There was a problem hiding this comment.
Ok I'll set it up that way. How about gitlab.webhook.pipeline? gitlab.pipeline sounds like it would define an actual pipeline.
There was a problem hiding this comment.
Well GitLab will be documented as a @eventual/integrations-gitlab so i'm not sure that will actually be true. Maybe we should call the class GitLabWebhook? I would usually lean towards minimal syntactic overhead.
| @@ -28,7 +28,9 @@ export default async function ( | |||
| : undefined; | |||
|
|
|||
| const request = new Request( | |||
| new URL(`http://localhost:3000${event.rawPath}?${event.rawQueryString}`), | |||
| new URL( | |||
| `${event.requestContext.http.protocol}://${event.requestContext.domainName}${event.rawPath}?${event.rawQueryString}` | |||
There was a problem hiding this comment.
I made this change also. http.protocol is wrong - it's something like HTTP/1. I think it needs to be retrieved from the x-forwarded-proto header instead. I just defaulted to https hard coded for now
For now, the beginnings of webhook integration. Allows you to define a webhook, which is mapped to an event. Will need to generate types for the other hook event types. And to see if this actually works.