Skip to content

CloudFormation: add types and restructure #8240

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
Merged
merged 8 commits into from
May 4, 2023
Merged

CloudFormation: add types and restructure #8240

merged 8 commits into from
May 4, 2023

Conversation

simonrw
Copy link
Contributor

@simonrw simonrw commented May 2, 2023 •
edited
Loading

Gain understanding by refactoring internals and add static types.

In particular, the func_details type was difficult to nail down.

Remove configure_resource_via_sdk since it had two purposes. Instead, inline the two behaviours into

  • a function that resolves the function properties, and
  • code that deploys the resource via the SDK.

@simonrw simonrw self-assigned this May 2, 2023
@simonrw simonrw requested a review from dominikschubert May 2, 2023 21:05
@coveralls
Copy link

coveralls commented May 2, 2023 •
edited
Loading

Coverage Status

Coverage: 82.11% (+0.01%) from 82.095% when pulling a93883d on refactor-cfn-stack-usage into b4bc83f on master.

@github-actions
Copy link

github-actions bot commented May 2, 2023 •
edited
Loading

LocalStack Community integration with Pro

1 998 tests   1 734 ✔️  1h 17m 5s ⏱️
       2 suites     264 💤
       2 files           0 ❌

Results for commit a93883d.

♻️ This comment has been updated with latest results.

@simonrw simonrw marked this pull request as ready for review May 3, 2023 10:10
dominikschubert
dominikschubert reviewed May 3, 2023
View reviewed changes
localstack/services/cloudformation/engine/template_deployer.py Outdated


# Type definition for func_details supplied to invoke_function and configure_resource_via_sdk
FuncDetails = list[FuncDetailsValue] | FuncDetailsValue | Any
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't that be a dict[str, Union[FuncDetailsValue, Callable]] ? 🤔

str could even be a union of the literals "create", "delete" for now, but I don't think that's necessary

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks, I've added another type that's the result of get_deploy_templates.

dominikschubert
dominikschubert approved these changes May 3, 2023
View reviewed changes
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 🚀

@simonrw simonrw added the semver: patch Non-breaking changes which can be included in patch releases label May 3, 2023
@simonrw simonrw force-pushed the refactor-cfn-stack-usage branch from b28fd21 to 5206df7 Compare May 4, 2023 15:14
@simonrw simonrw force-pushed the refactor-cfn-stack-usage branch from 5206df7 to a93883d Compare May 4, 2023 16:21
@simonrw simonrw merged commit 54eef43 into master May 4, 2023
@alexrashed alexrashed deleted the refactor-cfn-stack-usage branch November 28, 2023 13:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@dominikschubert dominikschubert dominikschubert approved these changes

Assignees

@simonrw simonrw

Labels
semver: patch Non-breaking changes which can be included in patch releases
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@simonrw @coveralls @dominikschubert