fix: Correct variable optional argument defaults and container definition conditional logic #332
Conversation
| content = module.ecs_container_definition.container_definition_json | ||
| filename = "${path.module}/definition.json" | ||
| # Need the output pretty-printed and sorted for comparison | ||
| resource "null_resource" "container_definition_json" { |
There was a problem hiding this comment.
the local_file doesn't output in pretty-printed JSON so switched to this hack to ensure its formatted and sorted to be able to compare diffs
There was a problem hiding this comment.
Why do we need definition.json other than for debugging? Consider making it conditional, so users can turn it off if needed.
|
another round of bugfixes due to the lack of hashicorp/terraform-provider-aws#17988 |
| ipc_mode = optional(string) | ||
| memory = optional(number, 2048) | ||
| network_mode = optional(string, "awsvpc") | ||
| network_mode = optional(string) |
There was a problem hiding this comment.
I like that there are fewer default values in "optional()". It is easier to read, and hopefully it also works more predictably. 👍
| content = module.ecs_container_definition.container_definition_json | ||
| filename = "${path.module}/definition.json" | ||
| # Need the output pretty-printed and sorted for comparison | ||
| resource "null_resource" "container_definition_json" { |
There was a problem hiding this comment.
Why do we need definition.json other than for debugging? Consider making it conditional, so users can turn it off if needed.
|
added the variable to enable/disable writing the container definition to file in 2354bb2 and yes, its used just for debugging and checking diffs - not consumed by end users at all |
## [6.1.4](v6.1.3...v6.1.4) (2025-08-07) ### Bug Fixes * Correct variable optional argument defaults and container definition conditional logic ([#332](#332)) ([054ad89](054ad89))
|
This PR is included in version 6.1.4 🎉 |
|
I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues. If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further. |
2 participants
Description
nullable = falsein wrapped sub-modules so that default values are surfaced properly and to avoid scenarios wherenulldoes not make sense (create = null- no). This also simplifies the number of redundant defaults set in the higher modules, instead relying on the lower modules to provide them unless overridden by users. Not all variables that have defaults have been set tonullable = falsebecause there are scenarios where users should still be allowed tonullout the value. For examplecpuandmemorysettings can be set tonullif a users so chooses, so we do not specifynullable = falsein those scenarioslinuxParameterswas corrected to:enable_execute_commandto eithertrueorfalseinitProcessEnabledto the value of their choosing whenenable_execute_command = falseToDoto thecontainer_definition_jsonoutput value. In hindsight, missed the part about multiple definitions being passed to the Task Definition API, which means that output would need to be wrapped in an array. However, that also means you could only ever pass one container definition from the output into the Task Definition, which is not usually the case; ergo, it doesn't make sense to have this output when we havecontainer_definitionthat can be composed within an array of other container definitions and then wrapped in anjsonencode()Motivation and Context
linuxParametersconditional and defaults #330initProcessEnabledwhenenable_execute_commandistrue#319Breaking Changes
How Has This Been Tested?
examples/*to demonstrate and validate my change(s)examples/*projectsfargateandcompleteexamples as they are defined onmaster, then switched to this branch to verify changespre-commit run -aon my pull request