-
Notifications
You must be signed in to change notification settings - Fork 29
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 support for Lambda #76
Conversation
} catch (error) { | ||
if (error.name !== 'AssertionError') { | ||
console.error(`FAIL [${description}]`, error.stack); | ||
check(error, {[description]: () => false}) | ||
} else { | ||
console.error(`FAIL [${description}]`, error.message); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding this bit to provide better error messages. While testing I noticed that we weren't really adding a failed check when the test failed without any failed assertions.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with this addition, albeit we might want to improve that generally eventually 👍🏻
Indeed asyncDescribe
is pretty much a workaround (to not say hack) at the moment, as using k6-chaijs in an async context is currently impacted by k6#2728, and group
not working correctly in an async context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @nickcaballero,
thanks for this new contribution! 🙇
We already have a similar PR #71 in the pipeline regarding the Lambda service. Our plan would be to merge #71 before then it would be great to have this PR rebased on top of it.
I see you have added additional details and a different proposal for the invoke
API that we would like to include in the project.
Does it sound like a reasonable plan for you?
Sounds good! Apologies, I didn't see the other PR 🤦. |
Hey @nickcaballero, we merged #71. You can now rebase on top of it. Thanks for the effort 🙇 |
a9445fc
to
0f8e8af
Compare
const error = response.json() as any; | ||
const message = error.Message || error.message || error.__type || 'An error occurred on the server side'; | ||
const code = response.headers['X-Amzn-Errortype'] || error.__type | ||
return new AWSError(message, code) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Pulled some of the error handling logic that @jakub-qg added into here to make it more reusable.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really neat 🙇🏻
Done! |
Consult the `LambdaClient` [dedicated k6 documentation page](https://k6.io/docs/javascript-api/jslib/aws/lambdaclient) for more details on its methods and how to use it. | ||
|
||
```javascript | ||
import { AWSConfig, LambdaClient } from 'https://jslib.k6.io/aws/0.11.0/lambda.js' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
import { AWSConfig, LambdaClient } from 'https://jslib.k6.io/aws/0.11.0/lambda.js' | |
import { AWSConfig, LambdaClient } from 'https://jslib.k6.io/aws/0.12.0/lambda.js' |
I expect we need to bump, right @oleiade?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codebien So far bumping version numbering has been part of our release process, and we haven't done it in feature PRs.
Although I realize we haven't added it to our CONTRIBUTING guidelines yet, will do so 👍🏻
testdata_folder="/etc/localstack/init/testdata/lambda" | ||
zip_dir=/tmp/lambda | ||
mkdir -p "$zip_dir" | ||
|
||
# Create a dummy lambda function responding with a static string "Hello World!" | ||
cat >index.js <<EOF | ||
exports.handler = async function(event, context) { | ||
return "Hello World!"; | ||
} | ||
EOF | ||
for file in "$testdata_folder"/*; do | ||
function_name=$(basename "$file") | ||
function_zip="$zip_dir/$function_name.zip" | ||
(cd "$file" || exit; zip "$function_zip" ./*) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@nickcaballero do we need to have them nested in folders? Could we have direct a list of files?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The file structure matches how the zips are created. We can flatten the folders, but it'll make the script a bit more complicated.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to have a simpler script, rather than a simpler folder structure 👍🏻
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👏🏻 🚀
Thank you for your contribution @nickcaballero, as always much appreciated 🙇🏻
} catch (error) { | ||
if (error.name !== 'AssertionError') { | ||
console.error(`FAIL [${description}]`, error.stack); | ||
check(error, {[description]: () => false}) | ||
} else { | ||
console.error(`FAIL [${description}]`, error.message); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm okay with this addition, albeit we might want to improve that generally eventually 👍🏻
Indeed asyncDescribe
is pretty much a workaround (to not say hack) at the moment, as using k6-chaijs in an async context is currently impacted by k6#2728, and group
not working correctly in an async context.
const error = response.json() as any; | ||
const message = error.Message || error.message || error.__type || 'An error occurred on the server side'; | ||
const code = response.headers['X-Amzn-Errortype'] || error.__type | ||
return new AWSError(message, code) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is really neat 🙇🏻
Consult the `LambdaClient` [dedicated k6 documentation page](https://k6.io/docs/javascript-api/jslib/aws/lambdaclient) for more details on its methods and how to use it. | ||
|
||
```javascript | ||
import { AWSConfig, LambdaClient } from 'https://jslib.k6.io/aws/0.11.0/lambda.js' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@codebien So far bumping version numbering has been part of our release process, and we haven't done it in feature PRs.
Although I realize we haven't added it to our CONTRIBUTING guidelines yet, will do so 👍🏻
This PR adds support for invoking Lambda functions.