Fix CloudFront origin error #514
Conversation
When using CloudFront without an origin access identity, we were failing to inlclude the "empty" property S3OriginConfig - this fixes that. Also adds an integ test and a validate that verifies this. Should fix aws#508
mindstorms6
force-pushed
the
508-cloudfront-origin
branch
from
90b0c24
to
f142583
Compare
| @@ -558,6 +567,16 @@ export class CloudFrontWebDistribution extends cdk.Construct { | |||
|
|
|||
| } | |||
|
|
|||
| public validate(): string[] { | |||
There was a problem hiding this comment.
Is it possible to add origins after the construct is created (might be a useful feature)? If not, then why not just validate this in the constructor?
There was a problem hiding this comment.
I don't mind validating in the constructor - I just thought that was the point of the validate method.
Is the idea that validate is only used if you have post-constructor modifications? (That makes sense to me, just trying to get a feel for how you see it)
There was a problem hiding this comment.
Yap. It's a hook that allows you to perform validations just before synthesis. But the sooner you can perform a validation, the better, because the error will include the most context for the user to be able to understand what they did wrong.
There was a problem hiding this comment.
Understood. I'll drop the validate for the time being - I don't currently think adding a origin post creation is valuable. (In my mind - it's not.)
If someone opens a pull request later - it's easy enough to add that.
When using CloudFront without an origin access identity, we were failing to include the "empty" property S3OriginConfig - this fixes that.
Also adds an integ test and a validate that verifies this.
Should fix #508
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license.