-
Notifications
You must be signed in to change notification settings - Fork 403
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
feat(parser): support for CloudFormation Custom Resources #2335
Conversation
@heitorlessa @leandrodamascena pr is ready. let me know what you think #2335 |
@heitorlessa @leandrodamascena fixed the code for older python support |
Codecov ReportPatch coverage:
Additional details and impacted files@@ Coverage Diff @@
## develop #2335 +/- ##
========================================
Coverage 97.16% 97.16%
========================================
Files 154 155 +1
Lines 7047 7067 +20
Branches 515 515
========================================
+ Hits 6847 6867 +20
Misses 157 157
Partials 43 43
☔ View full report in Codecov by Sentry. |
Hi @ran-isenberg! Thanks for sending the PR and sorry for the late reply, I was attending a meetup in another country. I'll look at it tomorrow; if all goes well, we can include it in the next release. |
Assigning to Leandro as he shared he's taking it @leandrodamascena |
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.
Hi @ran-isenberg! We need to make some updates before we merge this.
aws_lambda_powertools/utilities/parser/models/cloudformation_custom_resource.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/parser/models/cloudformation_custom_resource.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/parser/models/cloudformation_custom_resource.py
Outdated
Show resolved
Hide resolved
aws_lambda_powertools/utilities/parser/models/cloudformation_custom_resource.py
Outdated
Show resolved
Hide resolved
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.
Hi @ran-isenberg! We need to make some updates before we merge this.
…ustom_resource.py Co-authored-by: Leandro Damascena <leandro.damascena@gmail.com> Signed-off-by: Ran Isenberg <60175085+ran-isenberg@users.noreply.github.com>
…ustom_resource.py Co-authored-by: Leandro Damascena <leandro.damascena@gmail.com> Signed-off-by: Ran Isenberg <60175085+ran-isenberg@users.noreply.github.com>
@leandrodamascena I've pushed your suggestions |
hey @ran-isenberg could you update the PR body with the new name so we don't forget? Having a look at the rest now |
It seems older python code doesnt like the '|' instead of Union. what do you suggest? |
You can import from future: |
Yes, I added that import, still no go. In other models we use Optional and Union which I think are more readable than the pipes '|'. |
…ertools-python into custom
let me have a look ;) just finishing my last meeting of the day \o/ |
I see what's happening, you've just hit a bug in the grammar, and they fixed it in 3.10 only :/ https://bugs.python.org/issue42233 I'll revert to Union and make any necessary changes to get this merged. |
oh wow, that's a nasty one. |
Ready to merge - these are the changes I've made. Waiting for CI to complete then merge.
|
Leandro is on PTO, heitorlessa is taking over.
Thanks a lot @ran-isenberg for the contribution, much appreciated! The major change I've made that's worth pointing out is to refactor those tests as unit tests - here's the difference in a nutshell. BEFORE @event_parser(model=CloudFormationCustomResourceDeleteModel)
def handle_delete_custom_resource(event: CloudFormationCustomResourceDeleteModel, _: LambdaContext):
assert event.request_type == "Delete"
assert event.request_id == "xxxxx-d2a0-4dfb-ab1f-xxxxxx"
assert event.service_token == "arn:aws:lambda:us-east-1:xxx:function:xxxx-CrbuiltinfunctionidProvi-2vKAalSppmKe"
assert (
str(event.response_url)
== "https://cloudformation-custom-resource-response-useast1.s3.amazonaws.com/7F%7Cb1f50fdfc25f3b"
)
assert event.stack_id == "arn:aws:cloudformation:us-east-1:xxxx:stack/xxxx/271845b0-f2e8-11ed-90ac-0eeb25b8ae21"
assert event.logical_resource_id == "xxxxxxxxx"
assert event.resource_type == "Custom::MyType"
assert event.resource_properties == {
"ServiceToken": "arn:aws:lambda:us-east-1:xxxxx:function:xxxxx",
"MyProps": "ss",
}
def test_delete_trigger_event():
event_dict = load_event("cloudformationCustomResourceDelete.json")
handle_delete_custom_resource(event_dict, LambdaContext()) AFTER def test_cloudformation_custom_resource_delete_event():
raw_event = load_event("cloudformationCustomResourceDelete.json")
model = CloudFormationCustomResourceDeleteModel(**raw_event)
assert model.request_type == raw_event["RequestType"]
assert model.request_id == raw_event["RequestId"]
assert model.service_token == raw_event["ServiceToken"]
assert str(model.response_url) == raw_event["ResponseURL"]
assert model.stack_id == raw_event["StackId"]
assert model.logical_resource_id == raw_event["LogicalResourceId"]
assert model.resource_type == raw_event["ResourceType"]
assert model.resource_properties == raw_event["ResourceProperties"] This now focuses on the Model validation per se, no hard code data allowing us to have a schedule routine to generate real event to spot regressions in the future, and reduce boilerplate. If we use the same idea and try to test the Create test with a Custom model, we now only have to test the model per se: def test_cloudformation_custom_resource_create_event_custom_model():
class MyModel(BaseModel):
MyProps: str
class MyCustomResource(CloudFormationCustomResourceCreateModel):
resource_properties: MyModel = Field(..., alias="ResourceProperties")
raw_event = load_event("cloudformationCustomResourceCreate.json")
model = MyCustomResource(**raw_event)
assert model.resource_properties.MyProps == raw_event["ResourceProperties"].get("MyProps") |
@heitorlessa nice one, makes more sense as a unit test |
Co-authored-by: Leandro Damascena <leandro.damascena@gmail.com> Co-authored-by: Ran Isenberg <ran.isenberg@ranthebuilder.cloud> Co-authored-by: heitorlessa <lessa@amazon.co.uk>
…ools#2335) Co-authored-by: Leandro Damascena <leandro.damascena@gmail.com> Co-authored-by: Ran Isenberg <ran.isenberg@ranthebuilder.cloud> Co-authored-by: heitorlessa <lessa@amazon.co.uk>
Issue number: #2295
Summary
Changes
User experience
Using extended model:
Checklist
If your change doesn't seem to apply, please leave them unchecked.
Is this a breaking change?
No. **RFC issue number**:Checklist:
Acknowledgment
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.
Disclaimer: We value your time and bandwidth. As such, any pull requests created on non-triaged issues might not be successful.