-
Notifications
You must be signed in to change notification settings - Fork 4k
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(scheduler-targets-alpha): incorrect validation of maximumEventAge #32284
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #32284 +/- ##
=======================================
Coverage 77.46% 77.46%
=======================================
Files 105 105
Lines 7168 7168
Branches 1314 1314
=======================================
Hits 5553 5553
Misses 1433 1433
Partials 182 182
Flags with carried forward coverage won't be shown. Click here to find out more.
|
AWS CodeBuild CI Report
Powered by github-codebuild-logs, available on the AWS Serverless Application Repository |
@@ -6,8 +6,12 @@ exports.handler = async (event: any, _context: any) => { | |||
// assets index.ts file copied to the cdk - integ.out directory | |||
const client = new LambdaClient(); | |||
|
|||
// Skip tagging when passed tag value is missing (3rd case) | |||
const tagValue = event.tagValue; | |||
if (!tagValue) return; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Previously, the 3rd case occurs a validation error at lambda:TagResource
API call. Because, if schedule target input is absent, the event
argument is an EventBridge event record and the length exceeds the limit of tag value. This change can skip the API call when an assertion is not needed.
new scheduler.Schedule(scheduleStack, 'ScheduleWithStaticDate', { | ||
schedule: scheduler.ScheduleExpression.at(new Date('2000-01-01T00:00:00Z')), | ||
target: new LambdaInvoke(sameStackFunc, { retryAttempts: 0 }), | ||
target: new LambdaInvoke(sameStackFunc, { maxEventAge: cdk.Duration.minutes(1), retryAttempts: 1 }), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
When retryAttempts
is 0, maxEventAge
is ignored in managed console. Therefore changed to 1.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM. Thank you for making the change.
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). |
Comments on closed issues and PRs are hard for our team to see. |
Issue # (if applicable)
N/A
Reason for this change
The minimum value of
MaximumEventAgeInSeconds
of RetryPolicy is 60 seconds.https://docs.aws.amazon.com/ja_jp/AWSCloudFormation/latest/UserGuide/aws-properties-scheduler-schedule-retrypolicy.html
renderRetryPolicy()
currently throws an error whenmaxEventAge
is less than 15 minutes.Description of changes
Fixed the minimum value of
maxEventAge
to 1 minute. JSDoc is already correct.Description of how you validated changes
Fixed unit tests for maxEventAge.
Fixed a lambda integ test to specify maxEventAge to 1 minute.
Checklist
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license