getlift / lift

Expanding Serverless Framework beyond functions using the AWS CDK
MIT License
916 stars 113 forks source link

Add in method setting for webhooks #341

Closed SludgeGirl closed 1 year ago

SludgeGirl commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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

mnapoli commented 1 year ago

@fredericbarthelet WDYT?

SludgeGirl commented 1 year ago

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

mnapoli commented 1 year ago

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.

SludgeGirl commented 1 year ago

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

fredericbarthelet commented 1 year ago

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