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(events-targets): cannot set retry policy to 0 retry attempts #21900

Merged
merged 15 commits into from Sep 3, 2022

Conversation

VarunWachaspati
Copy link
Contributor

@VarunWachaspati VarunWachaspati commented Sep 2, 2022

Currently, it is not possible to set 0 retry attempts for any of the EventBridge targets that support a retry policy as 0 is falsy value and hence the following conditional doesn't evaluate to true -

retryPolicy: retryAttempts || maxEventAge
? {
maximumRetryAttempts: retryAttempts,
maximumEventAgeInSeconds: maxEventAge?.toSeconds({ integral: true }),
}
: undefined,

Changed the conditional logic to allow 0 retry attempts for all supported targets along with unit tests.

fixes #21864


All Submissions:

Adding new Unconventional Dependencies:

  • This PR adds new unconventional dependencies following the process described here

New Features

  • Have you added the new feature to an integration test?
    • Did you use yarn integ to deploy the infrastructure and generate the snapshot (i.e. yarn integ without --dry-run)?

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

@gitpod-io
Copy link

gitpod-io bot commented Sep 2, 2022

@TheRealAmazonKendra
Copy link
Contributor

I just set a bad example here and did what I ask people not to do. So, just in case you have a reason to update the branch with a merge, please use @Mergifyio update instead of what I did.

Copy link
Contributor

@TheRealAmazonKendra TheRealAmazonKendra left a comment

Choose a reason for hiding this comment

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

This bug is a great find. Thanks for submitting the fix! It looks great and nearly ready to go as is. We'll just need an integration test for this somewhere. Feel free to use an existing one and update it to have retryAttempts = 0.

Additionally, please make sure that your PR title confirms to the conventional commit standard (fix, feat, chore) and that it is written in a style that will reflect correctly in the change log (See Contributing Guide, Pull Requests (https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md#step-4-pull-request)).

Specifically from the guide fix: describe the bug (not the solution) and Title should be lowercase.

@VarunWachaspati VarunWachaspati changed the title fix(events-targets): allow 0 retry attempts for AWS EventBridge targets fix(events-targets): setting 0 retry attempts for eventbridge targets Sep 3, 2022
@mergify mergify bot dismissed TheRealAmazonKendra’s stale review September 3, 2022 12:20

Pull request has been modified.

@VarunWachaspati
Copy link
Contributor Author

Thanks for your feedback. I have updated the PR title to the best of my understanding as per the conventional commit standard. In case it is still not satisfactory, please do let me know.

I have changed the existing lambda integration test to test for the 0 retry attempt case, and ran the integration test successfully on my local machine as well.

yarn integ lambda/integ.events.js
yarn run v1.22.19
$ integ-runner lambda/integ.events.js

Verifying integration test snapshots...

  UNCHANGED  lambda/integ.events 2.449s

Snapshot Results:

Tests:    1 passed, 1 total
✨  Done in 2.91s.

Hope this is sufficient.

@TheRealAmazonKendra TheRealAmazonKendra changed the title fix(events-targets): setting 0 retry attempts for eventbridge targets fix(events-targets): cannot set 0 retry attempts for eventbridge targets Sep 3, 2022
@TheRealAmazonKendra TheRealAmazonKendra changed the title fix(events-targets): cannot set 0 retry attempts for eventbridge targets fix(events-targets): cannot set retry policy to 0 retry attempts Sep 3, 2022
@mergify
Copy link
Contributor

mergify bot commented Sep 3, 2022

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
Copy link
Contributor

mergify bot commented Sep 3, 2022

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
Copy link
Collaborator

AWS CodeBuild CI Report

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

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

@mergify mergify bot merged commit 5549f16 into aws:main Sep 3, 2022
@mergify
Copy link
Contributor

mergify bot commented Sep 3, 2022

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).

hacker65536 pushed a commit to hacker65536/aws-cdk that referenced this pull request Sep 30, 2022
…#21900)

Currently, it is not possible to set 0 retry attempts for any of the EventBridge targets that support a retry policy as 0 is falsy value and hence the following conditional doesn't evaluate to true - 
https://github.com/aws/aws-cdk/blob/c607ca51be1042f091b4e4419f20bec75863055c/packages/%40aws-cdk/aws-events-targets/lib/util.ts#L54-L59
Changed the conditional logic to allow 0 retry attempts for all supported targets along with unit tests.

fixes [aws#21864 ](aws#21864)

----

### All Submissions:

* [x] Have you followed the guidelines in our [Contributing guide?](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md)

### Adding new Unconventional Dependencies:

* [ ] This PR adds new unconventional dependencies following the process described [here](https://github.com/aws/aws-cdk/blob/main/CONTRIBUTING.md/#adding-new-unconventional-dependencies)

### New Features

* [x] Have you added the new feature to an [integration test](https://github.com/aws/aws-cdk/blob/main/INTEGRATION_TESTS.md)?
	* [x] Did you use `yarn integ` to deploy the infrastructure and generate the snapshot (i.e. `yarn integ` without `--dry-run`)?

*By submitting this pull request, I confirm that my contribution is made under the terms of the Apache-2.0 license*
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

aws-events-targets: retryAttempts = 0 does not work as intended
3 participants