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

feat(AWS S3): Recognize ExpirationInDays property for s3 events #10083

Merged
merged 4 commits into from Oct 13, 2021
Merged

feat(AWS S3): Recognize ExpirationInDays property for s3 events #10083

merged 4 commits into from Oct 13, 2021

Conversation

ROSeaboyer
Copy link
Contributor

Closes: #10081

@ROSeaboyer
Copy link
Contributor Author

@medikoo I'm not sure how to assign a reviewer to this specifically, but since you added the comment to the initial issue, would you mind reviewing the PR for it?

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

Thanks @ROSeaboyer !

To be able to address similar issues easier in a future, It'll be great to create a reusable util for that. So we can use for such properties:

ExpirationInDays: cfValue({ type: 'integer', minimum: 0 })

I propose to create lib/utils/aws-schema-get-cf-value.js util, which will return something as:

anyOf: [
  value,
  { $ref: '#/definitions/awsCfFunction' },
  { $ref: '#/definitions/awsCfIf' },
]

@pgrzesik what do you think?

@ROSeaboyer
Copy link
Contributor Author

@medikoo I just made that change; can you let me know what you think?

Copy link
Contributor

@medikoo medikoo left a comment

Choose a reason for hiding this comment

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

@ROSeaboyer thanks for update. Looks good to me!

@pgrzesik what are your thoughts?

@pgrzesik
Copy link
Contributor

Thanks @ROSeaboyer and sorry @medikoo - I'm missed previous notifcation. I think it looks good, we will definitely have more similar cases and that util will come in handy 👍

@codecov
Copy link

codecov bot commented Oct 13, 2021

Codecov Report

Merging #10083 (757bef6) into master (c4cb0f3) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head 757bef6 differs from pull request most recent head 51694f8. Consider uploading reports for the commit 51694f8 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##           master   #10083   +/-   ##
=======================================
  Coverage   85.41%   85.41%           
=======================================
  Files         333      334    +1     
  Lines       13586    13589    +3     
=======================================
+ Hits        11604    11607    +3     
  Misses       1982     1982           
Impacted Files Coverage Δ
...gins/aws/package/compile/events/s3/configSchema.js 100.00% <100.00%> (ø)
lib/utils/aws-schema-get-cf-value.js 100.00% <100.00%> (ø)

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 c4cb0f3...51694f8. Read the comment docs.

@pgrzesik pgrzesik changed the title Allow for Cf If statements for ExpirationInDays property for s3 events feat(AWS S3): Recognize ExpirationInDays property for s3 events Oct 13, 2021
@pgrzesik pgrzesik merged commit 8e6dcd1 into serverless:master Oct 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

s3 lifecycleConfiguration ExpirationInDays field doesn't allow for a CloudFormation If statement
3 participants