Fix EventSource Schedule passes Enabled to State #1666
Conversation
In the documentation it is stated that Enabled is passed directly to State, and also that intrinsic functions are supported in all of the Schedule EventSource properties. Added the expected "passing" of the value, while maintaining the previous behavior of `True -> ENABLED` and `False -> DISABLED`.
Codecov Report
@@ Coverage Diff @@
## develop #1666 +/- ##
========================================
Coverage 94.08% 94.09%
========================================
Files 86 86
Lines 5414 5416 +2
Branches 1082 1083 +1
========================================
+ Hits 5094 5096 +2
Misses 148 148
Partials 172 172
Continue to review full report at Codecov.
|
|
I couldn't verify this change on a real template, how should I do this? I tried running the script under How do I translate a template that I have locally? |
|
Got it to work. A fully working template renders and deploys successfully. |
|
Any comments? Something? :) |
|
I would like to see this PR merged. We currently have no workaround other than to look at other solutions like CDK. |
| if isinstance(self.Enabled, bool): | ||
| events_rule.State = "ENABLED" if self.Enabled else "DISABLED" | ||
| else: | ||
| events_rule.State = self.Enabled |
There was a problem hiding this comment.
Can self.Enabled be an instrinsic function at this point?
If so, is there a way to evaluate it?
There was a problem hiding this comment.
I think that if it's passed directly to CloudFormation, it will apply intrinsic functions (if applicable at CF level), isn't it so?
|
I'd also love to see this PR merged. Ran into this today and had to explode a couple schedule event sources into constituent rules, IAM roles, and Lambda permissions. |
|
Anyone? 🥺 |
|
Ping as a plus one for this to go in; as it is quite unexpected behavior and not ergonomic now with SAM if we're talking about 100s of events that need to be toggled via parameters. 👍 |
sriram-mv
added
the
stage/pm-review
jfuss
requested review from
a team,
mingkun2020 and
mildaniel
and removed request for
a team and
mingkun2020
| if self.Enabled is not None: | ||
| events_rule.State = "ENABLED" if self.Enabled else "DISABLED" | ||
| if isinstance(self.Enabled, bool): |
There was a problem hiding this comment.
If we are introducing a new property to get the behavior right with intrinsic support, why don't we just leave this as it was before?
There was a problem hiding this comment.
I meant to revert this back but looks like it got lost in me putting this task up and down. Will update.
| @@ -122,8 +123,18 @@ def to_cloudformation(self, **kwargs): | |||
| resources.append(events_rule) | |||
|
|
|||
| events_rule.ScheduleExpression = self.Schedule | |||
|
|
|||
| if self.State and self.Enabled: | |||
There was a problem hiding this comment.
Seems like we should explicitly check self.Enabled is not None. Consider:
Enabled: false # bool
State: ENABLED
There was a problem hiding this comment.
Good catch. Will make sure I add a test for this case too.
Issue
I stumbled upon this but noticed there's an open issue: #1271
Description of changes:
In the documentation, it is stated that Enabled is passed directly to State,
and also that intrinsic functions are supported in all of the Schedule
EventSource properties.
Added the expected "passing" of the value, while maintaining the previous
behavior of
True -> ENABLEDandFalse -> DISABLED.Description of how you validated changes:
Wrote missing tests for
Scheduleevent source, and for this specific test.Checklist:
make prpassesExamples?
Please reach out in the comments, if you want to add an example. Examples will be
added to
sam initthrough https://github.com/awslabs/aws-sam-cli-app-templates/By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.