fix(storage): Use template functions for LogicalID and Env Var names #1035
Conversation
* Move all functions used in go templates to the template package. * Move functions only used in one file from deploy/cloudformation/ stack/template_functions.go to deploy/cloudformation/stack/svc.go * Rename LogicalIDSafe used in mft/cf to ReplaceDashes (as the behavior is quite different from the LogicalIDSafe function used for storage environment variables) modified: internal/pkg/deploy/cloudformation/stack/app.go
internal/pkg/addon/storage.go
Outdated
| @@ -106,3 +104,8 @@ func NewS3(input *S3Props) *S3 { | |||
| parser: template.New(), | |||
| } | |||
| } | |||
|
|
|||
| var storageTemplateFunctions = map[string]interface{}{ | |||
There was a problem hiding this comment.
nit: would you mind moving the global variable to the top of the file? I don't know if there is official guidance on it but I think they tend to be written that way
| // sanitizes it by removing "-" characters (not allowed) | ||
| // and replacing them with "DASH" (allowed by CloudFormation but | ||
| // not permitted in ecs-cli generated resource names). | ||
| func ReplaceDashes(logicalID string) string { |
There was a problem hiding this comment.
What do you think of ending all of these function names with Func
so that it reads template.ReplaceDashesFunc to denote that they are template functions vs. regular functions?
| var nonAlphaNum = regexp.MustCompile("[^a-zA-Z0-9]+") | ||
|
|
||
| // StorageLogicalIDSafe strips non-alphanumeric characters from an input string. | ||
| func StorageLogicalIDSafe(s string) string { |
There was a problem hiding this comment.
Should we name this just:
| func StorageLogicalIDSafe(s string) string { | |
| func LogicalIDSafe(s string) string { |
|
|
||
| // EnvVarName converts an input resource name to LogicalIDSafe, then appends | ||
| // "Name" to the end. When generating environment variables, this string | ||
| // is then passed through the "toSnakeCase" method |
There was a problem hiding this comment.
Should we remove that last sentence? Technically users of this function don't have to pass it to ToSnakeCase
When generating environment variables, this string is then passed through the "toSnakeCase" method
Also addresses feedback: moves templateFuncs to top of file, renames StorageLogicalIDSafe to LogicalIDSafe
|
|
||
| // LogicalIDSafeFunc strips non-alphanumeric characters from an input string. | ||
| func LogicalIDSafeFunc(s string) string { | ||
| return nonAlphaNum.ReplaceAllString(s, "") |
There was a problem hiding this comment.
Do we need to call ReplaceDashesFunc to make a string logical id safe?
There was a problem hiding this comment.
ReplaceDashesFunc is used when writing the service cloudformation and has slightly different behavior than the LogicalIDSafeFunc, which is used when writing addons templates.
Originally, ReplaceDashesFunc was the only LogicalIDSafe method we needed, as we only needed to turn service names into CF logical IDs. Service names can only include lowercase letters, numbers, and dashes, so for example it would turn my-cool-frontend into myDASHcoolDASHfrontend for the service CF logical IDs which related to that service.
We also needed to be able to recover the name of the service from the logical ID in methods in internal/pkg/deploy/cloudformation/stack used in calls to pipeline update (this method is buried deep inside a call to GetRegionalAppResources and part of that is extracting original IDs from logical IDs for ECR repo URL generation).
With the addition of storage init, though, we needed to be able to sanitize more complex strings into logical IDs, as DDB table names can include uppercase, lowercase, numbers, and ._- characters. This is why I added LogicalIDSafeFunc, to handle this more complex parsing, as it simply removes non-alphanumeric characters from the string and results in simpler env var names injected into the container.
Does that make sense?
efekarakus
added
the
do-not-merge
bvtujo
removed
the
do-not-merge
This PR adds template functions to the storage resource CF templates. We now
only need to specify a resource name to the DynamoDB and S3 types, and
template functions StorageLogicalIDSafe and EnvVarName will properly construct
CF resource names and Env Vars.
This PR also reorganizes and renames some common go template functions that
previously lived in the "deploy/cloudformation/stack" package, moving them to the
"template" package.
Finally, it changes the implementation of ToSnakeCase to better match StringsWithUPPERCase
(now renders as "STRINGS_WITH_UPPER_CASE" instead of "STRINGS_WITH_U_P_P_E_R_CASE")
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.