Fail lambda if shouldFail flag provided #233
Conversation
docs/middlewares.md
Outdated
| @@ -487,6 +487,7 @@ For each secret, you also provide the name under which its value should be added | |||
| - `secrets` (object) : Map of secrets to fetch from Secrets Manager, where the key is the destination, and value is secret name in Secrets Manager. | |||
| Example: `{secrets: {RDS_LOGIN: 'dev/rds_login'}}` | |||
| - `awsSdkOptions` (object) (optional): Options to pass to AWS.SecretsManager class constructor. | |||
| - `shouldFail` (boolean) (optional): Defaults to `false`. Set it to `true` if you want your lambda to fail in case call to AWS Secrets Manager fails (secrets don't exist or internal error) | |||
There was a problem hiding this comment.
Maybe name it more explicitly to better convey the intent?
Like throwOnMissingParam or ignoreMissingParams
There was a problem hiding this comment.
Yeah I couldn't think of any good name, your proposal doesn't cover errors other than missing param in store - how about throwOnFailedCall?
There was a problem hiding this comment.
Hmm, agree, doesn't cover throttling errors etc, so another thing I can think of is to borrow bail name from jest
There was a problem hiding this comment.
Hmm, I think throw would be more descriptive here, bail is not so common
There was a problem hiding this comment.
LGTM, thanks for the valuable contribution @sdomagala! Might worth a look by @theburningmonk
|
Thanks @vladgolubev for quick review! |
src/middlewares/secretsManager.js
Outdated
| @@ -57,6 +58,9 @@ module.exports = opts => { | |||
| // if we already have a cached secrets, then reset the timestamp so we don't | |||
| // keep retrying on every invocation which can cause performance problems | |||
| // when there's temporary problems with Secrets Manager | |||
| if (options.throwOnFailedCall) { | |||
There was a problem hiding this comment.
I think we should only throw on the first request, if you have a cached value already you can continue with execution still. If we want to be smarter then we can build in some mechanism for the app developer to flush the cache - for example, by adding a function to the context object to flushCachedSecrets, but let's not do it in this PR.
There was a problem hiding this comment.
Hey @theburningmonk,
I didn't thought of it, it's fixed now, lambda will fail only if first call didn't succeed
Codecov Report
@@ Coverage Diff @@
## master #233 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 19 19
Lines 492 494 +2
Branches 100 101 +1
=====================================
+ Hits 492 494 +2
Continue to review full report at Codecov.
|
2 similar comments
Codecov Report
@@ Coverage Diff @@
## master #233 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 19 19
Lines 492 494 +2
Branches 100 101 +1
=====================================
+ Hits 492 494 +2
Continue to review full report at Codecov.
|
Codecov Report
@@ Coverage Diff @@
## master #233 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 19 19
Lines 492 494 +2
Branches 100 101 +1
=====================================
+ Hits 492 494 +2
Continue to review full report at Codecov.
|
|
@sdomagala thanks for making the change, LGTM! |
|
Hey Guys, sorry I have been a bit absent these days. Thanks for taking care of this and doing such a thorough review. I'll version bump and get this merged. |
|
@sdomagala, thanks for proposing this change. I tried to version bump (to version Thanks again :) |
|
Hey @lmammino, sure, on it! |
|
@lmammino, could you update package-lock yourself? I'm using yarn exclusively and get a lot of checksum errors when going the npm way, which then result in dependencies refresh. |
|
sure, I can update both. I already have the commit ready, i just can't push :P |
|
Looks like you have to accept being a collabolator, invite should be available here: https://github.com/sdomagala/middy/invitations |
Hello,
It seemed to me that checking if each secret is available at start of lambda function is a bit too much if you are in situation where missing secrets mean there is no point for lambda to run.