Skip to content

Fix CFN Events create rule without targets #6061

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

Merged

Conversation

pinzon
Copy link
Member

@pinzon pinzon commented May 13, 2022

This PR addresses issue #6033, where is shown that LocalStack CFN throws an error when trying to create a Rule that was defined without Targets. When testing against AWS, shows that it should work without problems.

Changes:

  • CFN Events Rule put_targets function is optional now.
  • Test to validate changes.
  • Template of a Rule without Targets.

Copy link
Member

@dominikschubert dominikschubert left a comment

Choose a reason for hiding this comment

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

Thanks for the fix and for validation this!
Please update the test to use the newer fixture and native CloudFormation parameters instead. I've already started refactoring a lot of existing tests for this as well 👍

@pinzon pinzon force-pushed the fix-cfn-events-rule-wout-targets branch 3 times, most recently from 9de47df to f91c276 Compare May 21, 2022 19:24
@pinzon pinzon force-pushed the fix-cfn-events-rule-wout-targets branch from f91c276 to 45e948d Compare May 23, 2022 18:03
@whummer whummer requested a review from dominikschubert May 30, 2022 21:21
Copy link
Member

@dominikschubert dominikschubert left a comment

Choose a reason for hiding this comment

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

LGTM. Thanks for fixing the nits 👍

@dominikschubert dominikschubert merged commit e3f26a2 into localstack:master May 31, 2022
@github-actions github-actions bot locked and limited conversation to collaborators May 31, 2022
@pinzon pinzon deleted the fix-cfn-events-rule-wout-targets branch July 13, 2022 22:24
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants