awslabs / goformation

GoFormation is a Go library for working with CloudFormation templates.
Apache License 2.0
841 stars 197 forks source link

feat(intrinsics): add support for FindInMap default #546

Closed xrn closed 1 year ago

xrn commented 1 year ago

Adding support for FindInMap default value:

Description of changes: This probably would be breaking change because of additional parameter in a function

By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.

xrn commented 1 year ago

@rubenfonseca any chance that you will take a look?

rubenfonseca commented 1 year ago

Hi @xrn this is great work! However, I'm worried that this is a breaking change that will require us to release a new major (v8) version. Have you considered making the argument list variable?

func FindInMapPtr(mapName, topLevelKey, secondLevelKey interface{}, additional ...interface{}) *string {
  ...
}

This way we can support both use cases at the same time. What do you think?

xrn commented 1 year ago

Both works for me but tbh when trying that way some tests from intrinsics_test.go are failing and not sure how to solve correctly (I am not senior Go dev but trying my best :D)

Overall would be awesome to have support for that default, will simplify templates a lot

rubenfonseca commented 1 year ago

Hi @xrn do you want to try to change the function signature as suggested?

codecov-commenter commented 1 year ago

Codecov Report

Patch coverage: 52.00% and project coverage change: -0.34 :warning:

Comparison is base (5ab72b1) 6.13% compared to head (61941fc) 5.80%.

Additional details and impacted files ```diff @@ Coverage Diff @@ ## master #546 +/- ## ========================================= - Coverage 6.13% 5.80% -0.34% ========================================= Files 30 30 Lines 16519 17566 +1047 ========================================= + Hits 1014 1020 +6 - Misses 15480 16518 +1038 - Partials 25 28 +3 ``` | [Impacted Files](https://app.codecov.io/gh/awslabs/goformation/pull/546?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=awslabs) | Coverage Δ | | |---|---|---| | [cloudformation/intrinsics.go](https://app.codecov.io/gh/awslabs/goformation/pull/546?src=pr&el=tree&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=awslabs#diff-Y2xvdWRmb3JtYXRpb24vaW50cmluc2ljcy5nbw==) | `44.34% <52.00%> (-2.60%)` | :arrow_down: | ... and [1 file with indirect coverage changes](https://app.codecov.io/gh/awslabs/goformation/pull/546/indirect-changes?src=pr&el=tree-more&utm_medium=referral&utm_source=github&utm_content=comment&utm_campaign=pr+comments&utm_term=awslabs)

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Do you have feedback about the report comment? Let us know in this issue.

xrn commented 1 year ago

@rubenfonseca can you take a look, sth like that works for you?

xrn commented 1 year ago

@rubenfonseca pls take a look - it would be very useful to have it

rubenfonseca commented 1 year ago

Thanks @xrn I'll look at this tomorrow!

rubenfonseca commented 1 year ago

Looking at this now

github-actions[bot] commented 1 year ago

:tada: This PR is included in version 7.8.0 :tada:

The release is available on GitHub release

Your semantic-release bot :package::rocket: