fix(s3): fail fast for s3 lifecycle configuration when ExpiredObjectDeleteMarker specified with ExpirationInDays, ExpirationDate, or TagFilters. #25841

Zishanwang1992 · 2023-06-03T00:18:27Z

By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license

gitpod-io · 2023-06-03T00:18:38Z

aws-cdk-automation

The pull request linter has failed. See the aws-cdk-automation comment below for failure reasons. If you believe this pull request should receive an exemption, please comment and provide a justification.

A comment requesting an exemption should contain the text Exemption Request. Additionally, if clarification is needed add Clarification Request to a comment.

Zishanwang1992 · 2023-06-03T01:03:33Z

Exemption Request: This fix is to fail fast. The integ test has no difference compared to unit test. The unit test has been added for the scenario.

abelmokadem · 2023-06-12T08:52:53Z

Just a random thought. Why not fix this in the type definition of Lifecycle Rule?

corymhall

Thanks for the PR! Just 2 minor comments

corymhall · 2023-06-13T10:24:02Z

packages/aws-cdk-lib/aws-s3/test/rules.test.ts

Can you add tests for all the combinations?

Will add different combinations.

@corymhall Could you help review this PR?

corymhall · 2023-06-13T10:25:45Z

packages/aws-cdk-lib/aws-s3/lib/bucket.ts

+      if ((rule.expiredObjectDeleteMarker != null && rule.expiredObjectDeleteMarker != undefined)
+      && (rule.expiration || rule.expirationDate || self.parseTagFilters(rule.tagFilters))) {
+        // ExpiredObjectDeleteMarker cannot be specified with ExpirationInDays, ExpirationDate, or TagFilters.
+        throw new Error('ExpiredObjectDeleteMarker cannot be specified with expiration, ExpirationDate, or TagFilters.');
+      }


Suggested change

if ((rule.expiredObjectDeleteMarker != null && rule.expiredObjectDeleteMarker != undefined)

&& (rule.expiration || rule.expirationDate || self.parseTagFilters(rule.tagFilters))) {

// ExpiredObjectDeleteMarker cannot be specified with ExpirationInDays, ExpirationDate, or TagFilters.

throw new Error('ExpiredObjectDeleteMarker cannot be specified with expiration, ExpirationDate, or TagFilters.');

}

if ((rule.expiredObjectDeleteMarker)

&& (rule.expiration || rule.expirationDate || this.parseTagFilters(rule.tagFilters))) {

// ExpiredObjectDeleteMarker cannot be specified with ExpirationInDays, ExpirationDate, or TagFilters.

throw new Error('ExpiredObjectDeleteMarker cannot be specified with expiration, ExpirationDate, or TagFilters.');

}

The reason I do not use the "if ((rule.expiredObjectDeleteMarker))" is the type of "expiredObjectDeleteMarker" is boolean. If its value is false, the validation here will pass without error(not work). I assume "false" could also not be specified with ExpirationInDays, ExpirationDate, or TagFilter according to public doc.

See if it makes sense.

Btw, the "this.parseTagFilters(rule.tagFilters)" will not compile, and there is same "self.parseTagFilters(rule.tagFilters)" usage at line 2112/2117. My change is to stay consistent with existing code.

See if it makes sense.

I have update the code for "if ((rule.expiredObjectDeleteMarker))" as suggested. Based on local integration test, the "false" of expiredObjectDeleteMarker can be specified together with ExpirationInDays, ExpirationDate, or TagFilter.

✅ Updated pull request passes all PRLinter validations. Dismissing previous PRLinter review.

…eleteMarker specified with ExpirationInDays, ExpirationDate, or TagFilters.

Pull request has been modified.

Zishanwang1992 · 2023-06-13T17:37:03Z

Just a random thought. Why not fix this in the type definition of Lifecycle Rule?

I am not sure if we can do this in the type definition of Lifecycle Rule, because there is complex validaiton logic. We can easily do this when Lifecycle rule gets initialized and the current code shall be the only place to initialize the Lifecycle rule.

See if it makes sense to you.

packages/aws-cdk-lib/aws-s3/test/rules.test.ts

mergify · 2023-06-14T11:06:19Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

mergify · 2023-06-14T11:09:59Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

aws-cdk-automation · 2023-06-14T11:57:57Z

AWS CodeBuild CI Report

CodeBuild project: AutoBuildv2Project1C6BFA3F-wQm2hXv2jqQv
Commit ID: e565f68
Result: SUCCEEDED
Build Logs (available for 30 days)

Powered by github-codebuild-logs, available on the AWS Serverless Application Repository

mergify · 2023-06-14T11:58:19Z

Thank you for contributing! Your pull request will be updated from main and then merged automatically (do not update manually, and be sure to allow changes to be pushed to your fork).

github-actions bot added bug This issue is a bug. effort/small Small work item – less than a day of effort p2 repeat-contributor [Pilot] contributed between 3-5 PRs to the CDK labels Jun 3, 2023

aws-cdk-automation requested a review from a team June 3, 2023 00:18

aws-cdk-automation previously requested changes Jun 3, 2023

View reviewed changes

Zishanwang1992 force-pushed the S3LifecycleRule branch from 8c8a858 to 9c62a47 Compare June 3, 2023 00:57

aws-cdk-automation added pr-linter/exemption-requested The contributor has requested an exemption to the PR Linter feedback. pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Jun 3, 2023

corymhall previously requested changes Jun 13, 2023

View reviewed changes

corymhall self-assigned this Jun 13, 2023

corymhall added pr-linter/exempt-integ-test The PR linter will not require integ test changes and removed pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. labels Jun 13, 2023

fix(s3): fail fast for s3 lifecycle configuration when ExpiredObjectD…

a43967a

…eleteMarker specified with ExpirationInDays, ExpirationDate, or TagFilters.

Zishanwang1992 force-pushed the S3LifecycleRule branch from 9c62a47 to a43967a Compare June 13, 2023 17:31

aws-cdk-automation added the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 13, 2023

corymhall reviewed Jun 14, 2023

View reviewed changes

packages/aws-cdk-lib/aws-s3/test/rules.test.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-s3/test/rules.test.ts Outdated Show resolved Hide resolved

packages/aws-cdk-lib/aws-s3/test/rules.test.ts Outdated Show resolved Hide resolved

Apply suggestions from code review

0ec4c28

corymhall approved these changes Jun 14, 2023

View reviewed changes

aws-cdk-automation removed the pr/needs-community-review This PR needs a review from a Trusted Community Member or Core Team Member. label Jun 14, 2023

Merge branch 'main' into S3LifecycleRule

e565f68

mergify bot merged commit 1a82d85 into aws:main Jun 14, 2023

filletofish mentioned this pull request Jul 2, 2023

feat(scheduler): ScheduleGroup #26196

Merged

github-actions bot mentioned this pull request Jul 26, 2023

Monthly PRs metrics report - June 2023 #26516

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix(s3): fail fast for s3 lifecycle configuration when ExpiredObjectDeleteMarker specified with ExpirationInDays, ExpirationDate, or TagFilters. #25841

fix(s3): fail fast for s3 lifecycle configuration when ExpiredObjectDeleteMarker specified with ExpirationInDays, ExpirationDate, or TagFilters. #25841

Zishanwang1992 commented Jun 3, 2023 •

edited by corymhall

Loading

gitpod-io bot commented Jun 3, 2023

aws-cdk-automation left a comment

Zishanwang1992 commented Jun 3, 2023

abelmokadem commented Jun 12, 2023

corymhall left a comment

corymhall Jun 13, 2023

Zishanwang1992 Jun 13, 2023

Zishanwang1992 Jun 13, 2023

corymhall Jun 13, 2023

Zishanwang1992 Jun 13, 2023 •

edited

Loading

Zishanwang1992 Jun 13, 2023 •

edited

Loading

Zishanwang1992 Jun 13, 2023

Zishanwang1992 commented Jun 13, 2023 •

edited

Loading

mergify bot commented Jun 14, 2023

mergify bot commented Jun 14, 2023

aws-cdk-automation commented Jun 14, 2023

mergify bot commented Jun 14, 2023

fix(s3): fail fast for s3 lifecycle configuration when ExpiredObjectDeleteMarker specified with ExpirationInDays, ExpirationDate, or TagFilters. #25841

fix(s3): fail fast for s3 lifecycle configuration when ExpiredObjectDeleteMarker specified with ExpirationInDays, ExpirationDate, or TagFilters. #25841

Conversation

Zishanwang1992 commented Jun 3, 2023 • edited by corymhall Loading

gitpod-io bot commented Jun 3, 2023

aws-cdk-automation left a comment

Choose a reason for hiding this comment

Zishanwang1992 commented Jun 3, 2023

abelmokadem commented Jun 12, 2023

corymhall left a comment

Choose a reason for hiding this comment

corymhall Jun 13, 2023

Choose a reason for hiding this comment

Zishanwang1992 Jun 13, 2023

Choose a reason for hiding this comment

Zishanwang1992 Jun 13, 2023

Choose a reason for hiding this comment

corymhall Jun 13, 2023

Choose a reason for hiding this comment

Zishanwang1992 Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

Zishanwang1992 Jun 13, 2023 • edited Loading

Choose a reason for hiding this comment

Zishanwang1992 Jun 13, 2023

Choose a reason for hiding this comment

Zishanwang1992 commented Jun 13, 2023 • edited Loading

mergify bot commented Jun 14, 2023

mergify bot commented Jun 14, 2023

aws-cdk-automation commented Jun 14, 2023

AWS CodeBuild CI Report

mergify bot commented Jun 14, 2023

Zishanwang1992 commented Jun 3, 2023 •

edited by corymhall

Loading

Zishanwang1992 Jun 13, 2023 •

edited

Loading

Zishanwang1992 Jun 13, 2023 •

edited

Loading

Zishanwang1992 commented Jun 13, 2023 •

edited

Loading