-
Notifications
You must be signed in to change notification settings - Fork 17
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
Add utility functions for supporting CloudFormation CustomResources #1808
Conversation
This change is part of the following stack: Change managed by git-spice. |
Does the PR have any schema changes?Looking good! No breaking changes found. |
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #1808 +/- ##
==========================================
+ Coverage 46.51% 46.78% +0.27%
==========================================
Files 42 42
Lines 6127 6164 +37
==========================================
+ Hits 2850 2884 +34
- Misses 3050 3052 +2
- Partials 227 228 +1 ☔ View full report in Codecov by Sentry. |
provider/pkg/naming/convert.go
Outdated
switch reflect.TypeOf(value).Kind() { | ||
case reflect.Map: | ||
valueMap, ok := value.(map[string]interface{}) | ||
if !ok { |
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.
Under what circumstance would this happen (!ok
)? Should we throw an error here instead?
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.
In this case it shouldn't happen because CloudFormation only supports strings as keys.
Throwing sounds more apt in this case. Will change it accordingly!
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.
I actually ended up changing the implementation to allow arbitrary map keys. That way the method is cleaner and we don't need to worry about somebody using it in the future for another usecase and tripping over it only supporting string-indexed maps
provider/pkg/naming/convert.go
Outdated
if !ok { | ||
return value | ||
} | ||
result := map[string]interface{}{} |
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.
Could we just call ToStringifiedMap
instead to avoid duplicating the code?
60b5047
to
4cd4ddb
Compare
This change adds utility methods needed for supporting CloudFormation custom resources.
The main part of this change is a method for deeply converting all primitive values in a
map[string]interface{}
to strings. This is needed because CloudFormation has this behavior: aws-cloudformation/cloudformation-coverage-roadmap#1037