search

andrew-templeton / cfn-lambda

CloudFormation custom resource helper for Lambda Node.js runtime
MIT License
82 stars 22 forks source link

sendResponse now needs parsedUrl.query for the PUT request #39

Closed nealeu closed 5 years ago

nealeu commented 5 years ago

I'll admit to not having tested, but your code looks to have the same issue as cfn-response at https://github.com/andrew-templeton/cfn-lambda/blob/development/index.js#L235, in that you leave out parsedUrl.query from the PUT request URL.

Have you tried against AWS recently?

andrew-templeton commented 5 years ago

Pull request? No I have not, have a day job + no updates to this in about 6 months. I do not know what new requirement you are referring to. Do you have a link to any of the updated documentation, which I could read to understand the new requirement? Do you know what exactly would need to be changed?

andrew-templeton commented 5 years ago

@nealeu - Checked what I think you're talking about. I accounted for this in my original design. I think you might misunderstand how the url.parse function works. Please see the attached image:

Screen Shot 2019-06-08 at 1 05 11 AM

Assuming you are referring to this portion: https://github.com/andrew-templeton/cfn-lambda/blob/development/index.js#L290

nealeu commented 5 years ago

@andrew-templeton Sorry if your first reply was a reflection of some frustration with me. Just trying to pass information on. It is easy for things to get out of date and my frustration is with Amazon not maintaining cfn-response themselves.

It's bizarre. I was having all sorts of problems and spotted a later implementation using all of parsed URL. Adding query did the trick.

FWIW, we're on Node 10 for our Lambdas, in case anyone else hits this issue. I will see if I get chance to do a regression check on Node 8 to see if that change is the problem.

andrew-templeton commented 5 years ago

Interesting. It was frustration, but unfair! I had a bad day at work. I owe you the apology. Thank you for checking it out. Are you still having issues? I did update it to use the newer version by default with a parametrized runtime parameter. If it's still an issue I'll definitely get on it - I was not able to reproduce though?

On Sat, Jun 8, 2019 at 2:44 PM Neale Upstone notifications@github.com wrote:

@andrew-templeton https://github.com/andrew-templeton Sorry if your first reply was a reflection of some frustration with me. Just trying to pass information on. It is easy for things to get out of date and my frustration is with Amazon not maintaining cfn-response themselves.

It's bizarre. I was having all sorts of problems and spotted a later implementation using all of parsed URL. Adding query did the trick.

FWIW, we're on Node 10 for our Lambdas, in case anyone else hits this issue. I will see if I get chance to do a regression check on Node 8 to see if that change is the problem.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/andrew-templeton/cfn-lambda/issues/39?email_source=notifications&email_token=AASUPK4MAKGDWHWL5UWFBITPZQD2TA5CNFSM4HVBRDPKYY3PNVWWK3TUL52HS4DFVREXG43VMVBW63LNMVXHJKTDN5WW2ZLOORPWSZGODXH4KCI#issuecomment-500155657, or mute the thread https://github.com/notifications/unsubscribe-auth/AASUPKZK5OWB5MF5Z5LKIHDPZQD2TANCNFSM4HVBRDPA .

nealeu commented 5 years ago

No apology needed. :) Those days do happen.

I'll retrofit what I did to a sample stack and give it a go on Monday, so I don't brick the dev environment for several hours.