Skip to content
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

fix(AWS CloudFront): Fix check for deprecated CacheBehavior properties #8768

Merged
merged 1 commit into from
Jan 15, 2021
Merged

fix(AWS CloudFront): Fix check for deprecated CacheBehavior properties #8768

merged 1 commit into from
Jan 15, 2021

Conversation

DASPRiD
Copy link
Contributor

@DASPRiD DASPRiD commented Jan 15, 2021

While trying to add a lambda@edge function with a cloudfront event, I ran into an issue. I assume that this is an edge case, which is why it wasn't caught so far:

My cloudFront event is specifying a cachePolicy and at the same time a behavior with AllowedMethods and CachedMethods. Independently they work as expected, but when combining them, a broken check triggers.

That check tests for deprecated properties, but instead of checking for the values being undefined, it checks if they are null, which is not a valid value in regards to CloudFormation.

This PR fixes that check and introduces a unit test to make sure it won't break again in the future.

@codecov
Copy link

codecov bot commented Jan 15, 2021

Codecov Report

Merging #8768 (a4065d3) into master (ffd7751) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #8768   +/-   ##
=======================================
  Coverage   87.60%   87.60%           
=======================================
  Files         260      260           
  Lines        9664     9664           
=======================================
  Hits         8466     8466           
  Misses       1198     1198           
Impacted Files Coverage Δ
...b/plugins/aws/package/compile/events/cloudFront.js 98.90% <ø> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ffd7751...a4065d3. Read the comment docs.

Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hello @DASPRiD, thanks for submitting this patch 🙇 It looks good, I have just one minor comment, please take a look at let me know what do you think

behaviorConfig.MinTTL !== null ||
behaviorConfig.MaxTTL !== null ||
behaviorConfig.DefaultTTL !== null
behaviorConfig.MinTTL !== undefined ||
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What do you think about instead of checking for undefined, we'll check for != null, which should cover both null and undefined values properly in that check?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

null should not be allowed at all – CloudFormation will complain that null is not a valid value.

Copy link
Contributor

@pgrzesik pgrzesik left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

@pgrzesik pgrzesik changed the title Fix check for deprecated CacheBehavior properties fix(AWS CloudFront): Fix check for deprecated CacheBehavior properties Jan 15, 2021
@pgrzesik pgrzesik merged commit c3a61e2 into serverless:master Jan 15, 2021
@DASPRiD
Copy link
Contributor Author

DASPRiD commented Jan 15, 2021

Thanks for the quick processing – considering the last release was only 6 hours ago, any chance that there'll be a hotfix release anytime soon? :)

@pgrzesik
Copy link
Contributor

@DASPRiD Most likely we will have a new release coming up sometime next week as we're wrapping up a few bigger features. Hope that works 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants