feat(deploy): add env resources import/adjust#1204
Conversation
faf27ef to
85d87fb
Compare
bvtujo
left a comment
There was a problem hiding this comment.
This is super slick! Thank you so much for writing out the CF. Just to confirm there will be test cases for all these branching logic paths, right?
| e.AdjustVpcConfig.PrivateSubnetCIDRs = aws.StringSlice(strings.Split(defaultPrivateSubnetCIDRS, ",")) | ||
| } | ||
|
|
||
| content, err := e.parser.Parse(EnvTemplatePath, struct { |
There was a problem hiding this comment.
Is there any reason we might want this to be its own defined struct? for reusability or ease of modifying the code?
There was a problem hiding this comment.
I think it is for reusability and ease of extending the struct (by categorizing fields), since other than vpc resources we'll have more options available and if flattening those two it would be very long.
The main logic changes are in CFN template which will be included in e2e tests! |
f8e2498 to
06c277d
Compare
| // ParseEnv parses an environment's CloudFormation template with the specified data object and returns its content. | ||
| func (t *Template) ParseEnv(data interface{}, options ...ParseOption) (*Content, error) { |
There was a problem hiding this comment.
If you look at these Parse* methods, we decided on purpose to take a specific struct so that we are guaranteed not to run into a runtime issue by forcing the data passed in to always render a valid template. For example:
I think I made a mistake with having these defined as methods, instead what do you think of defining them as functions in this package. Here is a sample interface: https://gist.github.com/efekarakus/3edbd280587c8433b0811307c2027b6a with an explanation at the bottom.
There was a problem hiding this comment.
Let me know your thoughts with the API above.
If you like it, we can keep the current PR as is but do have the next PR be a refactor of the template pkg.
There was a problem hiding this comment.
yeah since this pr is already big enough. I'll do the refactoring in the next one!
1d1947d to
d5d1cae
Compare
kohidave
left a comment
There was a problem hiding this comment.
Looks good! A few comments. Did you run the integ tests, by chance?
| ImportVpc *ImportVpcOpts | ||
| VpcConfig *AdjustVpcOpts |
There was a problem hiding this comment.
Would you do me a favor and separate this into two structs
type EnvImport struct {
VPC *ImportVPC
}
type EnvConfig struct {
VPC *ConfigVPC
}I just want to make it way easier in the future to add new config/import options.
There was a problem hiding this comment.
(We can even consider nesting them so we don't have to worry about namespacing them.
There was a problem hiding this comment.
discussed offline and keep it as it is for now
part of #1192.
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.