Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add in method setting for webhooks #341

Merged

Conversation

SludgeGirl
Copy link
Contributor

Some webhooks utilise PUT instead of strictly POST, in order to support them I've added in the ability to specify which method the webhook should allow in order to allow for these cases

Let me know if you want any changes to this, it's my first time touching the project so suggestions are more than welcome

@SludgeGirl SludgeGirl force-pushed the add-method-setting-for-webhooks branch from 9b72595 to da78f58 Compare June 14, 2023 20:18
@SludgeGirl
Copy link
Contributor Author

Sorry for the extra force, after a bit more time to think on it I wasn't quite happy with my handling of getHttpMethod, I'm still quite, getEndpointPath only returns the first instance of the output it finds in the logs, this leads to only finding the first method set.

Due to the promise required outputs() from AwsConstruct I can't just return this.methods.join(', ') nor can I use the stack output due to it only returning the first instance

I'd very happily get your opinion on hows best to handle this

@mnapoli
Copy link
Member

mnapoli commented Jun 16, 2023

Can the method be a single option, instead of an array? I think that would simplify the PR a lot, and would cover most use cases?

@SludgeGirl SludgeGirl force-pushed the add-method-setting-for-webhooks branch from da78f58 to 9aad291 Compare June 16, 2023 09:31
@SludgeGirl
Copy link
Contributor Author

Yep that'd make a lot of sense, I've just got those changes pushed up

@mnapoli
Copy link
Member

mnapoli commented Jun 16, 2023

@fredericbarthelet WDYT?

@SludgeGirl
Copy link
Contributor Author

@mnapoli Sorry for the nudge but is there anything else I need to do to get this one merged in?

@mnapoli
Copy link
Member

mnapoli commented Aug 6, 2023

I'd like to get @fredericbarthelet's opinion on this. I think this PR makes sense.

In any case we'd need tests to cover the new feature.

@@ -28,12 +28,14 @@ const WEBHOOK_DEFINITION = {
insecure: { type: "boolean" },
path: { type: "string" },
eventType: { type: "string" },
method: { type: "string" },
Copy link
Collaborator

Choose a reason for hiding this comment

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

I'd be a bit more precise here, in order to avoid HTTP verbs without body - API Gateway would not throw an error for a GET method, but integration with Eventbridge would crash as no body payload would be accessible :

Suggested change
method: { type: "string" },
method: { enum: ["POST", "PUT", "PATCH"] },

Copy link
Contributor Author

Choose a reason for hiding this comment

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

My latest changes have switched over to that!

Copy link
Collaborator

@fredericbarthelet fredericbarthelet 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 @SludgeGirl !
Could you update the documentation at

## Configuration reference
and include corresponding tests in test/unit/webhooks.test.ts ?

Once added, we'll be merging this feature !

@SludgeGirl SludgeGirl force-pushed the add-method-setting-for-webhooks branch from 9aad291 to aa7931e Compare August 24, 2023 12:55
@SludgeGirl
Copy link
Contributor Author

I've rebased everything, added in the changes to the docs and unit tests. Let me know if you want anything further done!

Copy link
Member

@mnapoli mnapoli left a comment

Choose a reason for hiding this comment

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

I'll let @fredericbarthelet review and approve

@fredericbarthelet fredericbarthelet merged commit fc3cfc3 into getlift:master Sep 19, 2023
14 checks passed
@fredericbarthelet
Copy link
Collaborator

Thanks for your contribution @SludgeGirl :) ! Will be releasing a new Lift version right away for you to be able tu use this feature.

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.

None yet

3 participants