Ensure changeResourceRecordSet() fails silently #8
Conversation
…domain is not managed via Route53.
|
Regarding the CI failure, I'll let you bump the version if you accept. |
|
Hey @mscifo, thanks for the contribution! I'm not sure if failing silently here is the right behavior. How about an optional config option instead for disabling the Route53 update? This way users still using Route53 will be notified when there is a failure. I'm imagining something like this. Let me know what you think custom:
customDomain:
...
createRoute53Record: falseThe option could default to |
|
I'm ok with that approach. The reason I did it the way I did was because I won't always know whether or not the domain will be managed by Route53 (I generate my |
|
Hey @mscifo, are you going to try to change this PR to use a config option? |
|
Yes, I just haven't gotten to it yet. Gonna try next week. |
…ecord optional (enabled by default).
| @@ -249,7 +255,16 @@ class ServerlessCustomDomain { | |||
| throw new Error(`${action} is not a valid action. action must be either CREATE or DELETE`); | |||
| } | |||
|
|
|||
| if (this.serverless.service.custom.customDomain.createRoute53Record !== undefined | |||
There was a problem hiding this comment.
Is the this.serverless.service.custom.customDomain.createRoute53Record !== undefined check needed if its also doing this.serverless.service.custom.customDomain.createRoute53Record === false?
There was a problem hiding this comment.
Just defensive programming. The createRoute53Record property won't exist in this.serverless.service.custom.customDomain if not defined in the serverless.yml and therefore won't strict equal false.
index.js
Outdated
| const endPos = hostedZoneId.length; | ||
| return hostedZoneId.substring(startPos, endPos); | ||
| } | ||
| return null; |
There was a problem hiding this comment.
I think its better to throw an exception instead of returning null if its unable to find the hosted zone. With the createRoute53Record argument, this code will only be called if a hosted zone is expected to exist so its an error if it doesn't.
Ensure
changeResourceRecordSet()fails silently in case domain is not managed via Route53.Also remove duplicate and unused
getHostedZoneId()call.