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.

Sign up for GitHub

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

Jump to bottom

Condition handling (for issue #142) #653

Merged
merged 15 commits into from
Dec 5, 2018
Merged

Condition handling (for issue #142) #653

merged 15 commits into from
Dec 5, 2018

Conversation

Jacco
Copy link
Contributor

@Jacco Jacco commented Nov 7, 2018

*Issue #142 *

Description of changes:

  • added "Condition" to the _supported_resource_attributes of Resource
  • SamFunction now passes attributes to constructor of LambdaFunction in _construct_lambda_function with the accepted attribute values of the source
  • added test case in test_translator.py (plus one input and three outputs) that checks "Condition" being created

This is my first PR, I know there is still work to do:

  • the passing of the attributes should also work for other SamResourceMacro's
  • The "Condition" attribute should also be put on other resources created on behalf of this one

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

Jacco Kulman and others added 2 commits November 7, 2018 18:20
brettstack
brettstack suggested changes Nov 9, 2018
View reviewed changes
Copy link
Contributor

@brettstack brettstack left a comment

Choose a reason for hiding this comment

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

This is great! Looking forward to the additions you mentioned.

General
abd805f 
- Added get_passthrough_resource_attributes to Resource currently
  only returns Condition
- Added make_conditional to intrinsics surrounds data with Fn:If and
  returns AWS::NoValue if the condition is false

SamFunction
- _construct_role of SamFunction now passes condition to created role
- _construct_version of SamFunction now adds condition to version
- _construct_alias of SamFunction now passes Condition to alias

PushEvents
- All events: _construct_permission passes passthrough attributes
- CloudWatchEvent gives event_rule same condition as function
- S3 gives event_rule same condition as function
- S3 Conditional DependsOn for the bucket (undocumented used!!)
  (see comments)
- S3 conditional LambdaConfigurations added

Tests
- Added test case s3_with_condition (+updated function_with_condition)
- test_cloudwatchlogs_event_source.py mock fixed

Still not done...
This is harder than I though, some help would be appreciated :-)
@Jacco
Copy link
Contributor Author

Jacco commented Nov 11, 2018 •
edited
Loading

Discussion:
In the S3 push events a DependsOn is used. Since conditional DependsOn are not supported (AWS CloudFormation requires strings only), I used an undocumented way to introduce implicit dependencies. Its is descibed in this stackoverflow post:

Conditional DependsOn

In the code I add a Tag:

Key value: sam:ConditionalDependsOn:logical-id (logical-id of the permission in this case)
Value: Fn:If [TheCondition, Fn:GetAtt [logical-id, Arn], 'no dependency']

Regards Jacco

DynamoDB table support for conditionals
aea4332 
Push event conditionals
@keetonian
Copy link
Contributor

@Jacco Thank you for submitting this PR! Are there any other outstanding items that you are going to address, or is this PR ready for review?

@Jacco
Copy link
Contributor Author

Jacco commented Nov 13, 2018

@keetonian I think it is ready for review. In the post above I point out a possible issue about conditional DependsOns.

Another issue might be the lack of tests, to test all possible scenario's with conditionals. But since its currently unsupported (Condition properties are essentially ignored). I think it could be released as a beta feature to get some bug reports/improvement requests (maybe with a command line parameter to activate).

Personally I do not have enough experience to oversee all the consequences.

@bottemav @brettstack
Feature/sns filter policy (#648)
8de5e86 
* document new property FilterPolicy in the SNS event source

* add unit test for sns event source

* implement new property FilterPolicy for SNS event source

* reorder the filter elements in the translated CloudFormation templates to align with the order in the original SAM template.

* add example lambda_sns_filter_policy

* add newline at the end of the yaml file
@keetonian keetonian self-assigned this Nov 13, 2018
@brianium
Copy link

@brettstack is this blocked by those outstanding items mentioned by @Jacco ? Sounds like he is waiting on review? I know I'm new here, but is there anything I can do to help either of you?

@jlhood
Copy link
Contributor

jlhood commented Nov 21, 2018

@Jacco Apologies for the delays in getting to this. We are still on the hook to review. @brianium, really appreciate the offer. Please feel free to take a look and comment on the PR if you see issues. Thanks!

brettstack
brettstack suggested changes Nov 23, 2018
View reviewed changes
Copy link
Contributor

@brettstack brettstack left a comment

Choose a reason for hiding this comment

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

Looking good. Couple of minor comments.

samtranslator/model/eventsources/push.py Show resolved Hide resolved
samtranslator/model/eventsources/push.py Outdated Show resolved Hide resolved
Jacco Kulman and others added 10 commits November 24, 2018 14:40
Fixes for review
6063b0e
Type in test outputs
90cbfc0
Condition handling (for issue #142)
02460f0
@Jacco
Update __init__.py
3627324 
Whitespce correction
General
f1f4c01 
- Added get_passthrough_resource_attributes to Resource currently
  only returns Condition
- Added make_conditional to intrinsics surrounds data with Fn:If and
  returns AWS::NoValue if the condition is false

SamFunction
- _construct_role of SamFunction now passes condition to created role
- _construct_version of SamFunction now adds condition to version
- _construct_alias of SamFunction now passes Condition to alias

PushEvents
- All events: _construct_permission passes passthrough attributes
- CloudWatchEvent gives event_rule same condition as function
- S3 gives event_rule same condition as function
- S3 Conditional DependsOn for the bucket (undocumented used!!)
  (see comments)
- S3 conditional LambdaConfigurations added

Tests
- Added test case s3_with_condition (+updated function_with_condition)
- test_cloudwatchlogs_event_source.py mock fixed

Still not done...
This is harder than I though, some help would be appreciated :-)
DynamoDB table support for conditionals
1b9e5b8 
Push event conditionals
Fixes for review
499b2e7
Type in test outputs
c88d36f
Fix mocks
03b4e1c
Fix rebase conflicts
2db6941
keetonian
keetonian approved these changes Nov 27, 2018
View reviewed changes
Copy link
Contributor

@keetonian keetonian left a comment

Choose a reason for hiding this comment

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

Excited for this feature!

@jlhood jlhood merged commit 16f512f into aws:develop Dec 5, 2018
@keetonian keetonian mentioned this pull request Jan 2, 2019
Merged
@keetonian
Copy link
Contributor

@Jacco If you have a moment, could you take a look at #755? It's a continuation of your work here. Looking through it, the way to add a Conditional depends_on to an S3 bucket that you implemented is rather neat.

@keetonian keetonian mentioned this pull request Jan 10, 2019
Closed
@Jacco
Copy link
Contributor Author

Jacco commented Jan 15, 2019

@keetonian I will have a look. Am a bit busy ATM so it might take some time.

@keetonian
Copy link
Contributor

Ok, no worries then. Thanks again for your work on this!

@brettstack brettstack mentioned this pull request Feb 6, 2019
Closed
keetonian added a commit to keetonian/serverless-application-model that referenced this pull request Feb 14, 2019
@keetonian
Revert "feat: add support for Condition (for issue aws#142) (aws#653)"
2202b72 
This reverts commit 16f512f.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@brettstack brettstack brettstack approved these changes

@keetonian keetonian keetonian approved these changes

Assignees

@keetonian keetonian

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@Jacco @keetonian @brianium @jlhood @brettstack @bottemav