New Resource: aws_ecs_daemon, aws_ecs_daemon_task_definition #47562
New Resource: aws_ecs_daemon, aws_ecs_daemon_task_definition #47562dloeafoe wants to merge 9 commits into
Conversation
Community GuidelinesThis comment is added to every new Pull Request to provide quick reference to how the Terraform AWS Provider is maintained. Please review the information below, and thank you for contributing to the community that keeps the provider thriving! 🚀 Voting for Prioritization
Pull Request Authors
|
|
✅ Thank you for correcting the previously detected issues! The maintainers appreciate your efforts to make the review process as smooth as possible. |
|
tagging @jar-b |
| "enable_ecs_managed_tags": schema.BoolAttribute{ | ||
| Optional: true, | ||
| Computed: true, | ||
| Default: booldefault.StaticBool(false), |
There was a problem hiding this comment.
Where possible, avoid provider-side default values in favor of optional/computed with a UseStateForUnknown plan modifier. More context here:
https://hashicorp.github.io/terraform-provider-aws/data-handling-and-conversion/#default-values
There was a problem hiding this comment.
These fields (enable_ecs_managed_tags and enable_execute_command) are write-only — they're accepted by CreateDaemon and UpdateDaemon but not returned by DescribeDaemon (DaemonDetail doesn't include them). With UseStateForUnknown(), the values remain unknown after Create when the user doesn't set them, causing Terraform to error with "Provider returned invalid result object after apply — still indicated an unknown value." The provider-side default is necessary here because there's no API response to populate these fields from. This matches the pattern used by wait_for_steady_state in express_gateway_service.go for the same reason.
There was a problem hiding this comment.
Perhaps these should instead be marked as WriteOnly then?
https://developer.hashicorp.com/terraform/plugin/framework/resources/write-only-arguments
| if len(plan.DeploymentConfiguration) > 0 { | ||
| input.DeploymentConfiguration = expandDaemonDeploymentConfigurationFromModel(plan.DeploymentConfiguration[0]) | ||
| } |
There was a problem hiding this comment.
As this is a fairly straightforward structure, AutoFlex should be able to handle this. Can you describe the issues you're observing that warrant the custom expand function?
There was a problem hiding this comment.
Investigated and confirmed AutoFlex can handle the full conversion. Updated model types to be AutoFlex-compatible: changed DeploymentConfiguration from []deploymentConfigurationModel with autoflex:"-" to fwtypes.ListNestedObjectValueOf[deploymentConfigurationModel], changed Alarms from []alarmConfigurationModel to fwtypes.ListNestedObjectValueOf[alarmConfigurationModel], and changed AlarmNames from types.Set to fwtypes.SetOfString. Removed the manual expand calls from both Create and Update in daemon.go and deleted the expandDaemonDeploymentConfigurationFromModel function entirely. AutoFlex now handles the full DeploymentConfiguration → DaemonDeploymentConfiguration conversion automatically.
|
Hi @jar-b. Thank you for the feedback. I've been working my way through the list of changes. I have completed most of the changes. I will finish up the rest of the changes and run the acceptance tests today. You can expect to see a new commit before the end of the day. I will put a summary of each change I made in the corresponding comment. |
|
Hi @jar-b. I've made the changes that you've requested. I've also detailed the changes I've made in each corresponding comment. Please let me know if you have any other feedback for me. |
| "enable_ecs_managed_tags": schema.BoolAttribute{ | ||
| Optional: true, | ||
| Computed: true, | ||
| Default: booldefault.StaticBool(false), |
There was a problem hiding this comment.
Perhaps these should instead be marked as WriteOnly then?
https://developer.hashicorp.com/terraform/plugin/framework/resources/write-only-arguments
|
Thanks @dloeafoe, I've left another round of comments and there are still some CI checks failing which will need to be resolved. Let me know if there is anything I can assist with. |
…ngs for daemon resources
|
Thanks again for the feedback @jar-b. I've gone ahead and addressed the issues you raised along with the issues from the failed CI checks. What I did in response to each issue is detailed in my response for each corresponding comment. Let me know if you have any questions about what I did. I did have one thing with which I needed some assistance. There were 3 CI checks that said I have a type mismatch error in
|
You will likely need to merge |
| } | ||
| } | ||
|
|
||
| func newNullObject(typ attr.Type) (obj basetypes.ObjectValue, diags diag.Diagnostics) { |
There was a problem hiding this comment.
This appears to be unused. Can it be removed?
| case err == nil: | ||
| // no-op, continue on to waiter | ||
| case errs.IsAErrorMessageContains[*awstypes.InvalidParameterException](err, "deployment deletion is ongoing"): | ||
| // no-op, continue on to waiter |
There was a problem hiding this comment.
Slight simplification.
| case err == nil: | |
| // no-op, continue on to waiter | |
| case errs.IsAErrorMessageContains[*awstypes.InvalidParameterException](err, "deployment deletion is ongoing"): | |
| // no-op, continue on to waiter | |
| case err == nil, errs.IsAErrorMessageContains[*awstypes.InvalidParameterException](err, "deployment deletion is ongoing"): | |
| // no-op, continue on to waiter |
There was a problem hiding this comment.
Combined two case clauses with identical // no-op, continue on to waiter bodies into one comma-separated case: case err == nil, errs.IsAErrorMessageContains[...]("deployment deletion is ongoing"):.
| return obj, diags | ||
| } | ||
|
|
||
| func daemonNameFromARN(arn string) types.String { |
There was a problem hiding this comment.
It would be good to have unit tests for this. Also, should if len(arnParts) match on exactly 3? Is there a valid form of the daemon ARN with more than 3 /-deliminted parts?
There was a problem hiding this comment.
Changed len(arnParts) >= 3 to len(arnParts) == 3 and arnParts[len(arnParts)-1] to arnParts[2] in daemonNameFromARN in daemon.go. Added DaemonNameFromARN = daemonNameFromARN export to exports_test.go following the existing pattern for exposing unexported functions to the test package. Added TestDaemonNameFromARN table-driven unit test to daemon_test.go . Test covers 5 cases: valid daemon ARN (returns name), too few parts (returns null), too many parts (returns null), empty string (returns null), no slashes (returns null).
There was a problem hiding this comment.
can we use arn.Parse() from aws-sdk instead
There was a problem hiding this comment.
Replaced manual strings.Split(arn, "/") with arn.Parse() from github.com/aws/aws-sdk-go-v2/aws/arn. Now validates full ARN structure before extracting the daemon name from parsed.Resource. Matches pattern in task_definition.go which also uses arn.Parse(). Added "github.com/aws/aws-sdk-go-v2/aws/arn" import.
| if !query.ClusterArn.IsNull() { | ||
| input.ClusterArn = query.ClusterArn.ValueStringPointer() | ||
| } |
There was a problem hiding this comment.
As a required argument, cluster_arn should never be null.
There was a problem hiding this comment.
Removed if !query.ClusterArn.IsNull() guard and inlined ClusterArn into the ListDaemonsInput struct initialization in daemon_list.go.
| var summaries []daemonTaskDefinitionSummaryModel | ||
| for _, summary := range results { | ||
| var s daemonTaskDefinitionSummaryModel | ||
| response.Diagnostics.Append(fwflex.Flatten(ctx, summary, &s)...) | ||
| summaries = append(summaries, s) | ||
| } | ||
|
|
||
| data.DaemonTaskDefinitions = fwtypes.NewListNestedObjectValueOfValueSliceMust(ctx, summaries) |
There was a problem hiding this comment.
AutoFlex should be able to handle this without manually iterating over each summary. If needed you can pass just the DaemonTaskDefinitions argument as the "target" argument (e.g. fwflex.Flatten(ctx, results, data.DaemonTaskDefinitions))
There was a problem hiding this comment.
Replaced 7-line manual loop with fwflex.Flatten(ctx, results, &data.DaemonTaskDefinitions) in daemon_task_definitions_data_source.go. Same pattern in daemons_data_source.go — replaced with fwflex.Flatten(ctx, summaries, &data.Daemons).
| Revision fwtypes.StringEnum[awstypes.DaemonTaskDefinitionRevisionFilter] `tfsdk:"revision"` | ||
| Sort fwtypes.StringEnum[awstypes.SortOrder] `tfsdk:"sort"` | ||
| Status fwtypes.StringEnum[awstypes.DaemonTaskDefinitionStatusFilter] `tfsdk:"status"` | ||
| DaemonTaskDefinitions fwtypes.ListNestedObjectValueOf[daemonTaskDefinitionSummaryModel] `tfsdk:"daemon_task_definitions"` |
| var results []daemonSummaryModel | ||
| for _, summary := range summaries { | ||
| s := daemonSummaryModel{ | ||
| DaemonArn: types.StringPointerValue(summary.DaemonArn), | ||
| Status: types.StringValue(string(summary.Status)), | ||
| } | ||
|
|
||
| if summary.CreatedAt != nil { | ||
| s.CreatedAt = timetypes.NewRFC3339TimePointerValue(summary.CreatedAt) | ||
| } | ||
|
|
||
| if summary.UpdatedAt != nil { | ||
| s.UpdatedAt = timetypes.NewRFC3339TimePointerValue(summary.UpdatedAt) | ||
| } | ||
|
|
||
| results = append(results, s) | ||
| } | ||
|
|
||
| data.Daemons = fwtypes.NewListNestedObjectValueOfValueSliceMust(ctx, results) |
There was a problem hiding this comment.
Replaced 14 lines of manual field-by-field mapping in daemons_data_source.go (types.StringPointerValue, types.StringValue(string(...)), timetypes.NewRFC3339TimePointerValue with nil checks) with a 4-line fwflex.Flatten loop.
| err := listDaemonsPages(ctx, conn, input, func(page *ecs.ListDaemonsOutput, lastPage bool) bool { | ||
| result = append(result, page.DaemonSummariesList...) | ||
| return !lastPage | ||
| }) |
There was a problem hiding this comment.
This needs a check to prevent potential nil pointer deferences when page is nil.
| err := listDaemonsPages(ctx, conn, input, func(page *ecs.ListDaemonsOutput, lastPage bool) bool { | |
| result = append(result, page.DaemonSummariesList...) | |
| return !lastPage | |
| }) | |
| err := listDaemonsPages(ctx, conn, input, func(page *ecs.ListDaemonsOutput, lastPage bool) bool { | |
| if page == nil { | |
| return !lastPage | |
| } | |
| result = append(result, page.DaemonSummariesList...) | |
| return !lastPage | |
| }) |
There was a problem hiding this comment.
Added if page == nil { return !lastPage } guard to paginator callbacks in 4 files: daemon.go (findDaemons), daemon_list.go (listDaemonSummaries), daemon_task_definition.go (findDaemonTaskDefinitions), and daemon_task_definition_list.go (listDaemonTaskDefinitionSummaries).
| err := listDaemonTaskDefinitionsPages(ctx, conn, input, func(page *ecs.ListDaemonTaskDefinitionsOutput, lastPage bool) bool { | ||
| result = append(result, page.DaemonTaskDefinitions...) | ||
| return !lastPage | ||
| }) |
There was a problem hiding this comment.
| err := listDaemonTaskDefinitionsPages(ctx, conn, input, func(page *ecs.ListDaemonTaskDefinitionsOutput, lastPage bool) bool { | |
| result = append(result, page.DaemonTaskDefinitions...) | |
| return !lastPage | |
| }) | |
| err := listDaemonTaskDefinitionsPages(ctx, conn, input, func(page *ecs.ListDaemonTaskDefinitionsOutput, lastPage bool) bool { | |
| if page == nil { | |
| return !lastPage | |
| } | |
| result = append(result, page.DaemonTaskDefinitions...) | |
| return !lastPage | |
| }) |
| func (r *daemonResource) Schema(ctx context.Context, request resource.SchemaRequest, response *resource.SchemaResponse) { | ||
| response.Schema = schema.Schema{ | ||
| Attributes: map[string]schema.Attribute{ | ||
| names.AttrARN: schema.StringAttribute{ |
There was a problem hiding this comment.
Added CustomType: fwtypes.ARNType to the schema, changed model field from types.String to fwtypes.ARN, and changed the Create assignment from types.StringValue(...) to fwtypes.ARNValue(...).
| names.AttrName: schema.StringAttribute{ | ||
| Required: true, | ||
| PlanModifiers: []planmodifier.String{ | ||
| stringplanmodifier.RequiresReplace(), |
There was a problem hiding this comment.
Added stringvalidator.LengthBetween(1, 255) and stringvalidator.RegexMatches(regexache.MustCompile("^[0-9A-Za-z_-]+$"), ...) to the name attribute. API docs state: "Up to 255 letters (uppercase and lowercase), numbers, underscores, and hyphens are allowed." Same pattern as family in daemon_task_definition.go. Added regexache and stringvalidator imports.
| }, | ||
| "daemon_task_definition": schema.StringAttribute{ | ||
| Required: true, | ||
| }, |
There was a problem hiding this comment.
Added deployment_arn as a Computed attribute with CustomType: fwtypes.ARNType. This field is the ARN of the most recent daemon deployment, returned by DaemonDetail.DeploymentArn in the Describe response. Added to documentation Attribute Reference and basic acceptance test (TestCheckResourceAttrSet).
| }, | ||
| }, | ||
| names.AttrPropagateTags: schema.StringAttribute{ | ||
| Optional: true, |
There was a problem hiding this comment.
Field is in CreateDaemonInput and UpdateDaemonInput but absent from DaemonDetail (Describe response). API never returns this value, making it write-only.
| names.AttrStatus: schema.StringAttribute{ | ||
| Computed: true, | ||
| PlanModifiers: []planmodifier.String{ | ||
| stringplanmodifier.UseStateForUnknown(), |
There was a problem hiding this comment.
UseStateForUnknown was removed since it was incorrect, it would hide drift. Added CustomType: fwtypes.StringEnumType[awstypes.DaemonStatus]() and changed model from types.String to fwtypes.StringEnum[awstypes.DaemonStatus] to match the SDK enum type.
| "bake_time_in_minutes": schema.Int64Attribute{ | ||
| Optional: true, | ||
| Validators: []validator.Int64{ | ||
| int64validator.Between(0, 1440), |
There was a problem hiding this comment.
Removed validator. Not in documentation.
Public API docs state "The default value is 0" for bakeTimeInMinutes. Added Computed: true and Default: int64default.StaticInt64(0) so Terraform fills in 0 when user omits the attribute. Computed is required alongside Default to allow Terraform to populate the value in state. Added int64default import.
https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_DaemonDeploymentConfiguration.html
| }, | ||
| }, | ||
| "firelens_configuration": schema.ListNestedBlock{ | ||
| CustomType: fwtypes.NewListNestedObjectTypeOf[firelensConfigurationModel](ctx), |
There was a problem hiding this comment.
Added listvalidator.SizeAtMost(1) — SDK field is FirelensConfiguration *FirelensConfiguration (pointer = single object). Prevents silent data loss from multiple blocks.
| }, | ||
| }, | ||
| names.AttrHealthCheck: schema.ListNestedBlock{ | ||
| CustomType: fwtypes.NewListNestedObjectTypeOf[healthCheckModel](ctx), |
There was a problem hiding this comment.
Added listvalidator.SizeAtMost(1) — SDK field is HealthCheck *HealthCheck (pointer = single object). Prevents silent data loss from multiple blocks.
| }, | ||
| }, | ||
| "linux_parameters": schema.ListNestedBlock{ | ||
| CustomType: fwtypes.NewListNestedObjectTypeOf[daemonLinuxParametersModel](ctx), |
There was a problem hiding this comment.
Added listvalidator.SizeAtMost(1) — SDK field is LinuxParameters *DaemonLinuxParameters (pointer = single object). Prevents silent data loss from multiple blocks.
| }, | ||
| Blocks: map[string]schema.Block{ | ||
| "capabilities": schema.ListNestedBlock{ | ||
| CustomType: fwtypes.NewListNestedObjectTypeOf[kernelCapabilitiesModel](ctx), |
There was a problem hiding this comment.
Added listvalidator.SizeAtMost(1) — SDK field is Capabilities *KernelCapabilities (pointer = single object). Prevents silent data loss from multiple blocks.
| }, | ||
| }, | ||
| "log_configuration": schema.ListNestedBlock{ | ||
| CustomType: fwtypes.NewListNestedObjectTypeOf[logConfigurationModel](ctx), |
There was a problem hiding this comment.
Added listvalidator.SizeAtMost(1) — SDK field is LogConfiguration *LogConfiguration (pointer = single object). Prevents silent data loss from multiple blocks.
| }, | ||
| }, | ||
| "repository_credentials": schema.ListNestedBlock{ | ||
| CustomType: fwtypes.NewListNestedObjectTypeOf[repositoryCredentialsModel](ctx), |
There was a problem hiding this comment.
Added listvalidator.SizeAtMost(1) — SDK field is RepositoryCredentials RepositoryCredentials` (pointer = single object). Prevents silent data loss from multiple blocks.
| }, | ||
| }, | ||
| "restart_policy": schema.ListNestedBlock{ | ||
| CustomType: fwtypes.NewListNestedObjectTypeOf[containerRestartPolicyModel](ctx), |
There was a problem hiding this comment.
Added listvalidator.SizeAtMost(1) — SDK field is RestartPolicy *ContainerRestartPolicy (pointer = single object). Prevents silent data loss from multiple blocks.
| input.EnableExecuteCommand = execCmd.ValueBool() | ||
| } | ||
|
|
||
| output, err := conn.CreateDaemon(ctx, &input) |
There was a problem hiding this comment.
Added defensive code to handle partitions that don't support tag-on-create. If CreateDaemon fails with tags and the error is a partition unsupported operation, retries without tags then applies tags separately via createTags. Pattern matches all taggable ECS resources have this.
Added retryDaemonCreate that wraps conn.CreateDaemon with tfresource.RetryWhen. Retries on ClusterNotFoundException (cluster just created in same apply) and InvalidParameterException containing "Unable to assume the service linked role" (IAM propagation delay). Total retry window: 4 minutes (propagationTimeout + 2min). Matches pattern in service.go (retryServiceCreate) and express_gateway_service.go (retryExpressGatewayServiceCreate). Replaced direct conn.CreateDaemon calls in Create function with retryDaemonCreate.
| return | ||
| } | ||
|
|
||
| daemon, err := findDaemonByARN(ctx, conn, plan.DaemonArn.ValueString()) |
There was a problem hiding this comment.
After findDaemonByARN in Create, added retry.NotFound(err) check that gracefully removes the resource from state if the daemon was deleted between creation and read-back. Matches the pattern in service.go which calls resourceServiceRead (which has not-found handling). Defends against the scenario of external deletion during the Create flow.
| response.Diagnostics.AddError(fmt.Sprintf("creating ECS Daemon (%s)", plan.DaemonName.ValueString()), err.Error()) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
Added defensive check if output == nil || output.DaemonArn == nil after the CreateDaemon API call. Prevents confusing downstream errors if the API returns a successful response with empty/nil fields. Matches express_gateway_service.go pattern which checks out == nil || out.Service == nil.
Added response.State.SetAttribute(ctx, path.Root("arn"), output.DaemonArn) immediately after creation, before waitDaemonActive. Also added github.com/hashicorp/terraform-plugin-framework/path import. Without this, if the waiter times out, the Create function returns an error and nothing is saved to state. The daemon exists in AWS but Terraform has no record of it and it is now an orphaned resource. On the next terraform apply, Terraform tries to create it again, fails with "already exists", and the user must manually terraform import to recover. With partial state save, even if the waiter times out, Terraform knows the resource exists (has its ARN) and can read/update/delete it on the next run without manual intervention. Matches express_gateway_service.go which does resp.State.SetAttribute(ctx, path.Root("service_arn"), out.Service.ServiceArn) before its waiter, and service.go which does d.SetId(arn) before its waiter.
| input.EnableExecuteCommand = execCmd.ValueBool() | ||
| } | ||
|
|
||
| _, err := conn.UpdateDaemon(ctx, &input) |
There was a problem hiding this comment.
Added retryDaemonUpdate that wraps conn.UpdateDaemon with tfresource.RetryWhen. Retries on InvalidParameterException containing "Unable to assume the service linked role" (IAM propagation delay when task definition references a newly created role). Total retry window: 4 minutes. Replaced direct conn.UpdateDaemon call in Update function. Matches pattern in service.go (inline RetryWhen in Update) and express_gateway_service.go (retryExpressGatewayServiceUpdate).
| input.EnableECSManagedTags = managedTags.ValueBool() | ||
| } | ||
| if !execCmd.IsNull() { | ||
| input.EnableExecuteCommand = execCmd.ValueBool() |
There was a problem hiding this comment.
Updated Create function to set input.PropagateTags = awstypes.DaemonPropagateTags(propagateTags.ValueString()) when not null. Previously, users setting propagate_tags = "DAEMON" had the value silently ignored.
| input.EnableECSManagedTags = managedTags.ValueBool() | ||
| } | ||
| if !execCmd.IsNull() { | ||
| input.EnableExecuteCommand = execCmd.ValueBool() |
There was a problem hiding this comment.
Updated Update function to set input.PropagateTags = awstypes.DaemonPropagateTags(propagateTags.ValueString()) when not null. Previously, users setting propagate_tags = "DAEMON" had the value silently ignored.
| Optional: true, | ||
| }, | ||
| names.AttrName: schema.StringAttribute{ | ||
| Optional: true, |
There was a problem hiding this comment.
In daemon_task_definition.go: Added LengthAtMost(255) and regex ^[0-9A-Za-z_-]+$ validators to name inside container_definition, per public API docs.
https://docs.aws.amazon.com/AmazonECS/latest/APIReference/API_DaemonContainerDefinition.html
| Required: true, | ||
| }, | ||
| names.AttrValue: schema.StringAttribute{ | ||
| Required: true, |
There was a problem hiding this comment.
Changed name and value in the environment block of container_definition from Required to Optional in daemon_task_definition.go.
| "retries": schema.Int64Attribute{ | ||
| Optional: true, | ||
| Validators: []validator.Int64{ | ||
| int64validator.Between(1, 10), |
There was a problem hiding this comment.
Added Computed: true and Default: int64default.StaticInt64(3) to retries in the health_check block of daemon_task_definition.go. Per public docs and SDK comments, the API server defaults retries to 3 when omitted.
| names.AttrTimeout: schema.Int64Attribute{ | ||
| Optional: true, | ||
| Validators: []validator.Int64{ | ||
| int64validator.Between(2, 60), |
There was a problem hiding this comment.
Added Computed: true and Default: int64default.StaticInt64(5) to timeout in the health_check block of daemon_task_definition.go.
| input.Volumes = expandDaemonVolumesFromModel(volumeSlice) | ||
| } | ||
|
|
||
| output, err := conn.RegisterDaemonTaskDefinition(ctx, &input) |
There was a problem hiding this comment.
Added retry logic that clears tags and retries RegisterDaemonTaskDefinition if the partition doesn't support tag-on-create (errs.IsUnsupportedOperationInPartitionError). Matches the pattern in `task_definition.go.
| return | ||
| } | ||
|
|
||
| plan.DaemonTaskDefinitionArn = fwtypes.ARNValue(aws.ToString(output.DaemonTaskDefinitionArn)) |
There was a problem hiding this comment.
Added createTags call after registration when input.Tags == nil && len(tags) > 0. This handles partitions that don't support tag-on-create by applying tags separately. Matches task_definition.go.
| response.Diagnostics.AddError(fmt.Sprintf("creating ECS Daemon Task Definition (%s)", plan.Family.ValueString()), err.Error()) | ||
| return | ||
| } | ||
|
|
There was a problem hiding this comment.
Added if output == nil || output.DaemonTaskDefinitionArn == nil check that returns "empty output from API" error. Prevents nil pointer panic if the API returns an unexpected empty response.
| DaemonTaskDefinition: state.DaemonTaskDefinitionArn.ValueStringPointer(), | ||
| }) | ||
|
|
||
| if errs.IsA[*awstypes.ClientException](err) { |
There was a problem hiding this comment.
Changed errs.IsA[*awstypes.ClientException](err) (swallows ALL ClientExceptions) to specific checks: DaemonTaskDefinitionNotFoundException, ClientException containing "not found", or ClientException containing "being deleted". Prevents silently swallowing real errors like permissions issues. Matches task_definition.go pattern.
Add defensive code patterns, fix schema issues, and improve test assertions for ECS Daemon and Daemon Task Definition resources. Daemon changes: - Add tag-on-create retry and tag-after-create for ISO partitions - Add nil output check after CreateDaemon - Add retryDaemonCreate/retryDaemonUpdate with IAM propagation retry - Add partial state save before waiter to prevent orphaned resources - Add WithIgnoredField for write-only fields in fwflex.Diff - Fix propagate_tags never being sent to API - Change capacity_provider_arns from List to Set - Refactor daemonNameFromARN to use arn.Parse - Strengthen test assertions (minimumValues, boundaryValues, deployment_arn) - Remove write-only field assertions from propagateTags test Daemon Task Definition changes: - Add tag-on-create retry and tag-after-create for ISO partitions - Add nil output check after RegisterDaemonTaskDefinition - Narrow Delete error handling to specific not-found conditions - Add validators to container_definition name and volume name - Add default values for health_check retries (3) and timeout (5) - Restore Computed on cpu and user (API returns values when not set) - Change environment block attributes from Required to Optional - Reorder functions (helpers before models) Shared: - Format testdata files with terraform fmt - Format Go files with gofmt
|
Hi @jar-b. I wanted to bring up an issue I came across in The API returns a value for I added I wanted to know if you agree with this approach. |
Rollback Plan
If a change needs to be reverted, we will publish an updated version of the library.
Changes to Security Controls
Are there any changes to security controls (access controls, encryption, logging) in this pull request? If so, explain.
Description
Adding support for the daemon and daemon task definition resources in Terraform. Users will be able to Create, Update, Delete, Read and List daemons and daemon task definitions.
Commit 2
Change Log added
Commit 3
Responded to comments left by reviewer. Specific actions taken are addressed in the comments.
Separate list resource structs (2 files):
daemon_list.go— Extracted List Resource implementation fromdaemon.gointo a separate struct, per the public clone's pattern for@FrameworkListResourcedaemon_task_definition_list.go— Same for daemon task definitionInclude resource list test configs (4 files):
3.
testdata/Daemon/list_include_resource/main.tf— Terraform config for the_IncludeResourcelist test4.
testdata/Daemon/list_include_resource/main.tfquery.hcl— Query config withinclude_resource = true5.
testdata/DaemonTaskDefinition/list_include_resource/main.tf— Same for daemon task definition6.
testdata/DaemonTaskDefinition/list_include_resource/main.tfquery.hcl— Same for daemon task definitionTemplate renames (2 files):
7.
daemon_basic.gtpl(renamed fromdaemon_tags.gtpl) — Generator expects_basic.gtplnaming8.
daemon_task_definition_basic.gtpl(renamed fromdaemon_task_definition_tags.gtpl) — Same2 new tests:
Commit 4
Made changes based on the second round of reviewer feedback and failed CI checks. The change that addresses each comment is outlined in my response to the corresponding comment. Please let me know if you have any questions.
Commit 5
-Merged main into feature branch to fix failing CI checks.
Commit 6
-Made changes based on reviewer comments. The details of each change is documented in a corresponding reply for each reviewer comment.
Commit 7
-Made changes based on output of failed CI checks and reviewer comments. Each code change is documented in the response to the corresponding reviewer comment.
Commit 8
-Made changes to Daemon Task Definition files in response to full audit and the latest reviewer comments.
-The Data Source files have been removed since they are not required for new resources. They will be added in a future update.
-Expanded test coverage for Daemon Task Definition.
Commit 9
-Made changes to Daemon Resource files based on reviewer comments. The details of the changes are documented in the comments.
-Defensive code added to both resource files
-Expanded test coverage for Daemon resource.
Relations
Closes #47200
References
Output from Acceptance Testing