Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Fix and enable tfproviderlint R004 check: Ensure compatible types for Set() calls #9954

Closed
ryndaniels opened this issue Sep 2, 2019 · 3 comments · Fixed by #11499
Closed

Fix and enable tfproviderlint R004 check: Ensure compatible types for Set() calls #9954

ryndaniels opened this issue Sep 2, 2019 · 3 comments · Fixed by #11499
Assignees
@bflad
Labels
linter Pertains to changes to or issues with the various linters. technical-debt Addresses areas of the codebase that need refactoring or redesign.
Milestone
v2.48.0

Comments

@ryndaniels
Copy link
Contributor

ryndaniels commented Sep 2, 2019 •
edited
Loading

Description

As per tfproviderlint check R004, we want to make sure that we are passing only allowed types to Set() calls. Correct types include a subset of basic types, slices and maps of those subset of basic types, and schema.Set types.

Failing example:

var t time.Time

d.Set("example", t)

Should become:

var t time.Time

d.Set("example", t.Format(time.RFC3339))

Definition of Done

  • In GNUmakefile, add -R004 to tfproviderlint command under lint target and have TravisCI testing pass
@ryndaniels ryndaniels added tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. technical-debt Addresses areas of the codebase that need refactoring or redesign. labels Sep 2, 2019
@github-actions github-actions bot added the needs-triage Waiting for first response or review from a maintainer. label Sep 2, 2019
@ryndaniels ryndaniels removed the needs-triage Waiting for first response or review from a maintainer. label Sep 2, 2019
@ryndaniels ryndaniels changed the title Ensure correct types for Set() calls in acceptance tests Ensure correct types for Set() calls Sep 2, 2019
@ryndaniels ryndaniels removed the tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. label Sep 2, 2019
@bflad bflad self-assigned this Jan 6, 2020
bflad added a commit that referenced this issue Jan 6, 2020
@bflad
resource/aws_batch_job_definition: Properly set container_properties …
ccd38f2 
…into Terraform state and perform drift detection

Reference: #9954
Reference: #11038

Previously, we were silently ignoring the `d.Set()` error for the `container_properties` attribute. This error can be seen with adding error checking on the call or with `tfproviderlint -R004`:

```
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_batch_job_definition.go:146:32: R004: ResourceData.Set() incompatible value type: *github.com/aws/aws-sdk-go/service/batch.ContainerProperties
```

Here we introduce the conversion the Batch `ContainerProperties` object into a JSON string, similar to the handling of ECS `ContainerDefinitions`. With the new attribute properly setting into the Terraform state, existing tests were failing with differences now being discovered:

```
--- FAIL: TestAccAWSBatchJobDefinition_basic (12.38s)
    testing.go:640: Step 0 error: After applying this step, the plan was not empty:

        DIFF:

        DESTROY/CREATE: aws_batch_job_definition.test
          ... omitted for clarity ...
          container_properties:               "{\"command\":[\"ls\",\"-la\"],\"environment\":[{\"name\":\"VARNAME\",\"value\":\"VARVAL\"}],\"image\":\"busybox\",\"memory\":512,\"mountPoints\":[{\"containerPath\":\"/tmp\",\"readOnly\":false,\"sourceVolume\":\"tmp\"}],\"resourceRequirements\":[],\"ulimits\":[{\"hardLimit\":1024,\"name\":\"nofile\",\"softLimit\":1024}],\"vcpus\":1,\"volumes\":[{\"host\":{\"sourcePath\":\"/tmp\"},\"name\":\"tmp\"}]}" => "{\"command\":[\"ls\",\"-la\"],\"environment\":[{\"name\":\"VARNAME\",\"value\":\"VARVAL\"}],\"image\":\"busybox\",\"memory\":512,\"mountPoints\":[{\"containerPath\":\"/tmp\",\"readOnly\":false,\"sourceVolume\":\"tmp\"}],\"ulimits\":[{\"hardLimit\":1024,\"name\":\"nofile\",\"softLimit\":1024}],\"vcpus\":1,\"volumes\":[{\"host\":{\"sourcePath\":\"/tmp\"},\"name\":\"tmp\"}]}" (forces new resource)
          ... omitted for clarity ...
```

Similar to some fields in ECS `ContainerDefinitions`, the API will inject an empty JSON array at least in the `RequiredResources` field. Instead of burdening operators with exactly matching the canonical API JSON, we insert difference suppression for this case. We also suppress reordered `Environment` objects (by `Name`; similar to ECS), since it is likely feature request as well.

Output from acceptance testing:

```
--- PASS: TestAccAWSBatchJobDefinition_basic (14.45s)
--- PASS: TestAccAWSBatchJobDefinition_updateForcesNewResource (24.61s)
```
@bflad bflad mentioned this issue Jan 6, 2020
Merged
@bflad
Copy link
Contributor

bflad commented Jan 6, 2020

Current R004 failures:

/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/data_source_aws_acmpca_certificate_authority.go:116:21: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/data_source_aws_acmpca_certificate_authority.go:117:22: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_acmpca_certificate_authority.go:362:21: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_acmpca_certificate_authority.go:363:22: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_appautoscaling_policy.go:333:18: R004: ResourceData.Set() incompatible value type: []*github.com/aws/aws-sdk-go/service/applicationautoscaling.Alarm
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_batch_job_definition.go:146:32: R004: ResourceData.Set() incompatible value type: *github.com/aws/aws-sdk-go/service/batch.ContainerProperties
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_dms_certificate.go:130:31: R004: ResourceData.Set() incompatible value type: []byte
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_ec2_client_vpn_endpoint.go:226:18: R004: ResourceData.Set() incompatible value type: *github.com/aws/aws-sdk-go/service/ec2.ClientVpnEndpointStatus
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_emr_security_configuration.go:96:25: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_iam_service_linked_role.go:140:23: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_lambda_event_source_mapping.go:224:25: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_organizations_account.go:228:28: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_ssm_activation.go:159:27: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_ssm_document.go:243:24: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_waf_sql_injection_match_set.go:101:38: R004: ResourceData.Set() incompatible value type: []*github.com/aws/aws-sdk-go/service/waf.SqlInjectionMatchTuple

Handling aws_batch_job_definition resource via #11488. The *time.Time fixes should be relatively straightforward but I would also like to ensure we have an easy CheckFunc for RFC3339 values (likely to be added to the Terraform Plugin SDK at some point).

bflad added a commit that referenced this issue Jan 6, 2020
@bflad
provider: Fix d.Set() calls with invalid *time.Time value input
f623913 
Reference: #9954

Previously, these attributes were incorrectly trying to use `*time.Time` as the value input to `d.Set()`. These will return errors if error checking is performed, however our preferred convention of not performing error checking for "simple" attributes means that these silently would skip being added to the Terraform state with the refreshed API value.

The `tfproviderlint` `R004` check caught these previously though (which we would like to enable in the future but cannot until these and other failures are addressed):

```
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/data_source_aws_acmpca_certificate_authority.go:116:21: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/data_source_aws_acmpca_certificate_authority.go:117:22: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_acmpca_certificate_authority.go:362:21: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_acmpca_certificate_authority.go:363:22: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_emr_security_configuration.go:96:25: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_iam_service_linked_role.go:140:23: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_lambda_event_source_mapping.go:224:25: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_organizations_account.go:228:28: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_ssm_activation.go:159:27: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_ssm_document.go:243:24: R004: ResourceData.Set() incompatible value type: *time.Time
```

Here we update each of these `d.Set()` calls with the appropriate RFC3339 timestamp value (our preferred format in lieu of other API standards).

The additional changes for `aws_ssm_activation` are to handle the API automatically setting a default value of 24 hours in advance. We do not want Terraform to return a difference in cases where the configuration does not have a value.

Output from acceptance testing:

```
--- PASS: TestAccDataSourceAwsAcmpcaCertificateAuthority_Basic (29.17s)

--- PASS: TestAccAwsAcmpcaCertificateAuthority_Basic (23.49s)

--- PASS: TestAccAWSEmrSecurityConfiguration_basic (18.22s)

--- PASS: TestAccAWSIAMServiceLinkedRole_basic (23.39s)

--- PASS: TestAccAWSLambdaEventSourceMapping_kinesis_basic (85.92s)
--- PASS: TestAccAWSLambdaEventSourceMapping_sqs_basic (134.73s)

--- PASS: TestAccAWSSSMActivation_expirationDate (24.95s)
--- PASS: TestAccAWSSSMActivation_basic (29.94s)

--- PASS: TestAccAWSSSMDocument_basic (21.52s)
```
@bflad bflad mentioned this issue Jan 6, 2020
Merged
bflad added a commit that referenced this issue Jan 6, 2020
@bflad
resource/aws_appautoscaling_policy: Remove unimplemented alarms attri…
98167fc 
…bute

Reference: #9954

Previously:

```
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_appautoscaling_policy.go:333:18: R004: ResourceData.Set() incompatible value type: []*github.com/aws/aws-sdk-go/service/applicationautoscaling.Alarm
```

The `alarms` attribute was never actually implemented:

- `d.Set()` never would have set anything in the Terraform state due to silently ignoring the error above, so Terraform configuration references to the attribute would always have been invalid trying to reference a non-existent resource attribute
- Configuring `alarms` in a configuration was never valid as it would not have done anything (CloudWatch Alarms are a read-only property of Application Auto Scaling Policies)
- Not acceptance tested
- Not documented

Due to these conditions and since there are no associated bug reports or feature requests for this attribute, it feels safe to bypass our normal attribute deprecation/removal process and just remove the errant attribute definition from the schema. If desired in the future, this could be implemented as a `Computed: true` only attribute.

Output from acceptance testing:

```
--- PASS: TestAccAWSAppautoScalingPolicy_dynamoDb (34.67s)
--- PASS: TestAccAWSAppautoScalingPolicy_multiplePoliciesSameName (40.33s)
--- PASS: TestAccAWSAppautoScalingPolicy_multiplePoliciesSameResource (41.44s)
--- PASS: TestAccAWSAppautoScalingPolicy_disappears (72.83s)
--- PASS: TestAccAWSAppautoScalingPolicy_basic (78.93s)
--- PASS: TestAccAWSAppautoScalingPolicy_spotFleetRequest (81.81s)
--- PASS: TestAccAWSAppautoScalingPolicy_ResourceId_ForceNew (87.53s)
--- PASS: TestAccAWSAppautoScalingPolicy_scaleOutAndIn (88.80s)
```
@bflad bflad mentioned this issue Jan 6, 2020
Merged
bflad added a commit that referenced this issue Jan 6, 2020
@bflad
resource/aws_dms_certificate: Properly set certificate_wallet into Te…
78faed9 
…rraform state

Reference: #9954
Reference: https://github.com/terraform-providers/terraform-provider-aws/blob/768341474a743c3db30610c6b434fed598d98cd8/aws/resource_aws_dms_certificate.go#L71

Previously:

```
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_dms_certificate.go:130:31: R004: ResourceData.Set() incompatible value type: []byte
```

Here we perform the opposite `[]byte` to `string` conversion as compared to the creation function to prevent silently ignoring a type error in the `d.Set()` call and fix drift detection for the attribute.

Output from acceptance testing:

```
--- PASS: TestAccAWSDmsCertificateBasic (17.45s)
```

Aside: Its worth noting that this attribute is not acceptance tested and potentially may not work as given in Terraform 0.12+ if there are non UTF-8 characters in the value (e.g. the `file()` function will error). Generally `[]byte` attributes must have base64 encoding/decoding added above and beyond the AWS Go SDK's handling, however I do not have intimate knowledge about this functionality, so it may be okay. There are no bug reports and it would take a non-trivial amount of effort to create a valid Oracle Wallet acceptance test, so this fix is best effort with other similar fixes to allow us to continue working towards enabling the `tfproviderlint` `R004` check in the near future.
@bflad bflad mentioned this issue Jan 6, 2020
Merged
bflad added a commit that referenced this issue Jan 6, 2020
@bflad
resource/aws_ec2_client_vpn_endpoint: Properly set status value into …
2888ed0 
…Terraform state

Reference: #9954

Previously:

```
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_ec2_client_vpn_endpoint.go:226:18: R004: ResourceData.Set() incompatible value type: *github.com/aws/aws-sdk-go/service/ec2.ClientVpnEndpointStatus
```

Output from acceptance testing:

```
--- PASS: TestAccAwsEc2ClientVpnEndpoint_basic (21.32s)
```
@bflad bflad mentioned this issue Jan 6, 2020
Merged
bflad added a commit that referenced this issue Jan 6, 2020
@bflad
resource/aws_waf_sql_injection_match_set: Properly set sql_injection_…
3e1cc56 
…match_tuples into Terraform state

Reference: #9954

Previously:

```
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_waf_sql_injection_match_set.go:101:38: R004: ResourceData.Set() incompatible value type: []*github.com/aws/aws-sdk-go/service/waf.SqlInjectionMatchTuple
```

Fixes `d.Set()` handling for this attribute with existing flatten function (used by `aws_wafregional_sql_injection_match_set` resource) and one associated test configuration so that it matches API canonicalization, which is now enforced by the testing since the value is being properly read into the Terraform state.

Output from acceptance testing:

```
--- PASS: TestAccAWSWafSqlInjectionMatchSet_noTuples (13.29s)
--- PASS: TestAccAWSWafSqlInjectionMatchSet_disappears (17.63s)
--- PASS: TestAccAWSWafSqlInjectionMatchSet_basic (17.65s)
--- PASS: TestAccAWSWafSqlInjectionMatchSet_changeTuples (23.46s)
--- PASS: TestAccAWSWafSqlInjectionMatchSet_changeNameForceNew (31.73s)
```
@bflad bflad mentioned this issue Jan 6, 2020
Merged
bflad added a commit that referenced this issue Jan 6, 2020
@bflad
tests/provider: Enable tfproviderlint R004 check
f56bdef 
Reference: https://github.com/bflad/tfproviderlint/blob/master/passes/R004/README.md
Reference: #9954
@bflad bflad mentioned this issue Jan 6, 2020
Merged
bflad added a commit that referenced this issue Jan 17, 2020
@bflad
provider: Fix d.Set() calls with invalid *time.Time value input (#11491)
bd59ac2 
Reference: #9954

Previously, these attributes were incorrectly trying to use `*time.Time` as the value input to `d.Set()`. These will return errors if error checking is performed, however our preferred convention of not performing error checking for "simple" attributes means that these silently would skip being added to the Terraform state with the refreshed API value.

The `tfproviderlint` `R004` check caught these previously though (which we would like to enable in the future but cannot until these and other failures are addressed):

```
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/data_source_aws_acmpca_certificate_authority.go:116:21: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/data_source_aws_acmpca_certificate_authority.go:117:22: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_acmpca_certificate_authority.go:362:21: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_acmpca_certificate_authority.go:363:22: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_emr_security_configuration.go:96:25: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_iam_service_linked_role.go:140:23: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_lambda_event_source_mapping.go:224:25: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_organizations_account.go:228:28: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_ssm_activation.go:159:27: R004: ResourceData.Set() incompatible value type: *time.Time
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_ssm_document.go:243:24: R004: ResourceData.Set() incompatible value type: *time.Time
```

Here we update each of these `d.Set()` calls with the appropriate RFC3339 timestamp value (our preferred format in lieu of other API standards).

The additional changes for `aws_ssm_activation` are to handle the API automatically setting a default value of 24 hours in advance. We do not want Terraform to return a difference in cases where the configuration does not have a value.

Output from acceptance testing:

```
--- PASS: TestAccDataSourceAwsAcmpcaCertificateAuthority_Basic (29.17s)

--- PASS: TestAccAwsAcmpcaCertificateAuthority_Basic (23.49s)

--- PASS: TestAccAWSEmrSecurityConfiguration_basic (18.22s)

--- PASS: TestAccAWSIAMServiceLinkedRole_basic (23.39s)

--- PASS: TestAccAWSLambdaEventSourceMapping_kinesis_basic (85.92s)
--- PASS: TestAccAWSLambdaEventSourceMapping_sqs_basic (134.73s)

--- PASS: TestAccAWSSSMActivation_expirationDate (24.95s)
--- PASS: TestAccAWSSSMActivation_basic (29.94s)

--- PASS: TestAccAWSSSMDocument_basic (21.52s)
```
bflad added a commit that referenced this issue Jan 17, 2020
@bflad
resource/aws_appautoscaling_policy: Remove unimplemented alarms attri…
071abb3 
…bute (#11494)

Reference: #9954

Previously:

```
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_appautoscaling_policy.go:333:18: R004: ResourceData.Set() incompatible value type: []*github.com/aws/aws-sdk-go/service/applicationautoscaling.Alarm
```

The `alarms` attribute was never actually implemented:

- `d.Set()` never would have set anything in the Terraform state due to silently ignoring the error above, so Terraform configuration references to the attribute would always have been invalid trying to reference a non-existent resource attribute
- Configuring `alarms` in a configuration was never valid as it would not have done anything (CloudWatch Alarms are a read-only property of Application Auto Scaling Policies)
- Not acceptance tested
- Not documented

Due to these conditions and since there are no associated bug reports or feature requests for this attribute, it feels safe to bypass our normal attribute deprecation/removal process and just remove the errant attribute definition from the schema. If desired in the future, this could be implemented as a `Computed: true` only attribute.

Output from acceptance testing:

```
--- PASS: TestAccAWSAppautoScalingPolicy_dynamoDb (34.67s)
--- PASS: TestAccAWSAppautoScalingPolicy_multiplePoliciesSameName (40.33s)
--- PASS: TestAccAWSAppautoScalingPolicy_multiplePoliciesSameResource (41.44s)
--- PASS: TestAccAWSAppautoScalingPolicy_disappears (72.83s)
--- PASS: TestAccAWSAppautoScalingPolicy_basic (78.93s)
--- PASS: TestAccAWSAppautoScalingPolicy_spotFleetRequest (81.81s)
--- PASS: TestAccAWSAppautoScalingPolicy_ResourceId_ForceNew (87.53s)
--- PASS: TestAccAWSAppautoScalingPolicy_scaleOutAndIn (88.80s)
```
bflad added a commit that referenced this issue Jan 17, 2020
@bflad
resource/aws_dms_certificate: Properly set certificate_wallet into Te…
3973812 
…rraform state (#11496)

Reference: #9954
Reference: https://github.com/terraform-providers/terraform-provider-aws/blob/768341474a743c3db30610c6b434fed598d98cd8/aws/resource_aws_dms_certificate.go#L71

Previously:

```
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_dms_certificate.go:130:31: R004: ResourceData.Set() incompatible value type: []byte
```

Here we perform the opposite `[]byte` to `string` conversion as compared to the creation function to prevent silently ignoring a type error in the `d.Set()` call and fix drift detection for the attribute.

Output from acceptance testing:

```
--- PASS: TestAccAWSDmsCertificateBasic (17.45s)
```

Aside: Its worth noting that this attribute is not acceptance tested and potentially may not work as given in Terraform 0.12+ if there are non UTF-8 characters in the value (e.g. the `file()` function will error). Generally `[]byte` attributes must have base64 encoding/decoding added above and beyond the AWS Go SDK's handling, however I do not have intimate knowledge about this functionality, so it may be okay. There are no bug reports and it would take a non-trivial amount of effort to create a valid Oracle Wallet acceptance test, so this fix is best effort with other similar fixes to allow us to continue working towards enabling the `tfproviderlint` `R004` check in the near future.
bflad added a commit that referenced this issue Jan 17, 2020
@bflad
resource/aws_ec2_client_vpn_endpoint: Properly set status value into …
6c3cc5f 
…Terraform state (#11497)

Reference: #9954

Previously:

```
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_ec2_client_vpn_endpoint.go:226:18: R004: ResourceData.Set() incompatible value type: *github.com/aws/aws-sdk-go/service/ec2.ClientVpnEndpointStatus
```

Output from acceptance testing:

```
--- PASS: TestAccAwsEc2ClientVpnEndpoint_basic (21.32s)
```
bflad added a commit that referenced this issue Jan 17, 2020
@bflad
resource/aws_waf_sql_injection_match_set: Properly set sql_injection_…
880473c 
…match_tuples into Terraform state (#11498)

Reference: #9954

Previously:

```
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_waf_sql_injection_match_set.go:101:38: R004: ResourceData.Set() incompatible value type: []*github.com/aws/aws-sdk-go/service/waf.SqlInjectionMatchTuple
```

Fixes `d.Set()` handling for this attribute with existing flatten function (used by `aws_wafregional_sql_injection_match_set` resource) and one associated test configuration so that it matches API canonicalization, which is now enforced by the testing since the value is being properly read into the Terraform state.

Output from acceptance testing:

```
--- PASS: TestAccAWSWafSqlInjectionMatchSet_noTuples (13.29s)
--- PASS: TestAccAWSWafSqlInjectionMatchSet_disappears (17.63s)
--- PASS: TestAccAWSWafSqlInjectionMatchSet_basic (17.65s)
--- PASS: TestAccAWSWafSqlInjectionMatchSet_changeTuples (23.46s)
--- PASS: TestAccAWSWafSqlInjectionMatchSet_changeNameForceNew (31.73s)
```
@bflad bflad changed the title Ensure correct types for Set() calls Fix and enable tfproviderlint R004 check: Ensure compatible types for Set() calls Feb 3, 2020
bflad added a commit that referenced this issue Feb 3, 2020
@bflad
tests/provider: Enable new passing tfproviderlint checks
dd7482b 
Gaps in `tfproviderlint` checking are covered by the following issues:

Reference: #9950 (fix and enable AT003)
Reference: #11862 (fix and enable AT005)
Reference: #9951 (fix and enable R001)
Reference: #9952 (fix and enable R002)
Reference: #9953 (fix and enable R003)
Reference: #9954 (fix and enable R004)
Reference: #11863 (fix and enable R005)
Reference: #11864 (fix and enable R006)
Reference: #9955 (fix and enable S006)
Reference: #9956 (fix and enable S018)
Reference: #11865 (fix and enable S020)
Reference: #11866 (fix and enable S022)
Reference: #11867 (fix and enable S023)
Reference: #11868 (fix and enable S024)
Reference: #11869 (fix and enable S031)
Reference: #11870 (fix and enable S032)
Reference: #11871 (fix and enable S033)
Reference: #11872 (fix and enable V001)
Reference: #11844 (fix and enable V002, V004, V005, V007, V008)
@bflad bflad mentioned this issue Feb 3, 2020
Merged
bflad added a commit that referenced this issue Feb 4, 2020
@bflad
tests/provider: Enable new passing tfproviderlint checks (#11873)
e1e3709 
Gaps in `tfproviderlint` checking are covered by the following issues:

Reference: #9950 (fix and enable AT003)
Reference: #11862 (fix and enable AT005)
Reference: #9951 (fix and enable R001)
Reference: #9952 (fix and enable R002)
Reference: #9953 (fix and enable R003)
Reference: #9954 (fix and enable R004)
Reference: #11863 (fix and enable R005)
Reference: #11864 (fix and enable R006)
Reference: #9955 (fix and enable S006)
Reference: #9956 (fix and enable S018)
Reference: #11865 (fix and enable S020)
Reference: #11866 (fix and enable S022)
Reference: #11867 (fix and enable S023)
Reference: #11868 (fix and enable S024)
Reference: #11869 (fix and enable S031)
Reference: #11870 (fix and enable S032)
Reference: #11871 (fix and enable S033)
Reference: #11872 (fix and enable V001)
Reference: #11844 (fix and enable V002, V004, V005, V007, V008)
bflad added a commit that referenced this issue Feb 5, 2020
@bflad
resource/aws_batch_job_definition: Properly set container_properties …
0a8efb1 
…and name into Terraform state and fix basic test (#11488)

* tests/resource/aws_batch_job_definition: Fix TestAccAWSBatchJobDefinition_basic ContainerProperties comparison to match updated API response

Previously:

```
--- FAIL: TestAccAWSBatchJobDefinition_basic (5.36s)
    testing.go:640: Step 0 error: Check failed: Check 2/2 error: Bad Job Definition Container Properties
        	 expected: {
          Command: ["ls","-la"],
          Environment: [{
              Name: "VARNAME",
              Value: "VARVAL"
            }],
          Image: "busybox",
          Memory: 512,
          MountPoints: [{
              ContainerPath: "/tmp",
              ReadOnly: false,
              SourceVolume: "tmp"
            }],
          Ulimits: [{
              HardLimit: 1024,
              Name: "nofile",
              SoftLimit: 1024
            }],
          Vcpus: 1,
          Volumes: [{
              Host: {
                SourcePath: "/tmp"
              },
              Name: "tmp"
            }]
        }
        	got: {
          Command: ["ls","-la"],
          Environment: [{
              Name: "VARNAME",
              Value: "VARVAL"
            }],
          Image: "busybox",
          Memory: 512,
          MountPoints: [{
              ContainerPath: "/tmp",
              ReadOnly: false,
              SourceVolume: "tmp"
            }],
          ResourceRequirements: [],
          Ulimits: [{
              HardLimit: 1024,
              Name: "nofile",
              SoftLimit: 1024
            }],
          Vcpus: 1,
          Volumes: [{
              Host: {
                SourcePath: "/tmp"
              },
              Name: "tmp"
            }]
        }
```

Output from acceptance testing:

```
--- PASS: TestAccAWSBatchJobDefinition_basic (15.23s)
--- PASS: TestAccAWSBatchJobDefinition_updateForcesNewResource (26.75s)
```

* resource/aws_batch_job_definition: Properly set container_properties into Terraform state and perform drift detection

Reference: #9954
Reference: #11038

Previously, we were silently ignoring the `d.Set()` error for the `container_properties` attribute. This error can be seen with adding error checking on the call or with `tfproviderlint -R004`:

```
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/resource_aws_batch_job_definition.go:146:32: R004: ResourceData.Set() incompatible value type: *github.com/aws/aws-sdk-go/service/batch.ContainerProperties
```

Here we introduce the conversion the Batch `ContainerProperties` object into a JSON string, similar to the handling of ECS `ContainerDefinitions`. With the new attribute properly setting into the Terraform state, existing tests were failing with differences now being discovered:

```
--- FAIL: TestAccAWSBatchJobDefinition_basic (12.38s)
    testing.go:640: Step 0 error: After applying this step, the plan was not empty:

        DIFF:

        DESTROY/CREATE: aws_batch_job_definition.test
          ... omitted for clarity ...
          container_properties:               "{\"command\":[\"ls\",\"-la\"],\"environment\":[{\"name\":\"VARNAME\",\"value\":\"VARVAL\"}],\"image\":\"busybox\",\"memory\":512,\"mountPoints\":[{\"containerPath\":\"/tmp\",\"readOnly\":false,\"sourceVolume\":\"tmp\"}],\"resourceRequirements\":[],\"ulimits\":[{\"hardLimit\":1024,\"name\":\"nofile\",\"softLimit\":1024}],\"vcpus\":1,\"volumes\":[{\"host\":{\"sourcePath\":\"/tmp\"},\"name\":\"tmp\"}]}" => "{\"command\":[\"ls\",\"-la\"],\"environment\":[{\"name\":\"VARNAME\",\"value\":\"VARVAL\"}],\"image\":\"busybox\",\"memory\":512,\"mountPoints\":[{\"containerPath\":\"/tmp\",\"readOnly\":false,\"sourceVolume\":\"tmp\"}],\"ulimits\":[{\"hardLimit\":1024,\"name\":\"nofile\",\"softLimit\":1024}],\"vcpus\":1,\"volumes\":[{\"host\":{\"sourcePath\":\"/tmp\"},\"name\":\"tmp\"}]}" (forces new resource)
          ... omitted for clarity ...
```

Similar to some fields in ECS `ContainerDefinitions`, the API will inject an empty JSON array at least in the `RequiredResources` field. Instead of burdening operators with exactly matching the canonical API JSON, we insert difference suppression for this case. We also suppress reordered `Environment` objects (by `Name`; similar to ECS), since it is likely feature request as well.

Output from acceptance testing:

```
--- PASS: TestAccAWSBatchJobDefinition_basic (14.45s)
--- PASS: TestAccAWSBatchJobDefinition_updateForcesNewResource (24.61s)
```

* resource/aws_batch_job_definition: Properly read name into Terraform state

Reference: #11407 (review)

To prevent future issues with resource import, when implemented.

Output from acceptance testing:

```
--- PASS: TestAccAWSBatchJobDefinition_basic (14.00s)
--- PASS: TestAccAWSBatchJobDefinition_updateForcesNewResource (27.64s)
```
bflad added a commit that referenced this issue Feb 5, 2020
@bflad
tests/provider: Enable tfproviderlint R004 check
75de0eb 
Reference: https://github.com/bflad/tfproviderlint/blob/master/passes/R004/README.md
Reference: #9954
bflad added a commit that referenced this issue Feb 5, 2020
@bflad
tests/provider: Enable tfproviderlint R004 check (#11499)
82b8834 
* tests/provider: Enable tfproviderlint R004 check

Reference: https://github.com/bflad/tfproviderlint/blob/master/passes/R004/README.md
Reference: #9954

* resource/aws_s3_bucket: Fix complex import of aws_s3_bucket_policy to properly set policy in state

Found via linting:

```
/Users/bflad/src/github.com/terraform-providers/terraform-provider-aws/aws/import_aws_s3_bucket.go:36:22: R004: ResourceData.Set() incompatible value type: *github.com/aws/aws-sdk-go/service/s3.GetBucketPolicyOutput
35		pData.Set("bucket", d.Id())
36		pData.Set("policy", pol)
37		results = append(results, pData)
```

This type of complex import (importing multiple resources) is likely to be removed in a future major version.

Output from relevant acceptance testing (other tests do not perform necessary ImportStateCheck to verify complex import):

```
--- PASS: TestAccAWSS3Bucket_Policy (95.74s)
```
@bflad bflad added this to the v2.48.0 milestone Feb 5, 2020
@ghost
Copy link

ghost commented Feb 7, 2020

This has been released in version 2.48.0 of the Terraform AWS provider. Please see the Terraform documentation on provider versioning or reach out if you need any assistance upgrading.

For further feature requests or bug reports with this functionality, please create a new GitHub issue following the template for triage. Thanks!

@ghost
Copy link

ghost commented Mar 27, 2020

I'm going to lock this issue because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active issues.

If you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thanks!

@ghost ghost locked and limited conversation to collaborators Mar 27, 2020
@breathingdust breathingdust added the linter Pertains to changes to or issues with the various linters. label Oct 27, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees

@bflad bflad

Labels
linter Pertains to changes to or issues with the various linters. technical-debt Addresses areas of the codebase that need refactoring or redesign.
Projects
None yet
Milestone
v2.48.0
Development

Successfully merging a pull request may close this issue.

tests/provider: Enable tfproviderlint R004 check hashicorp/terraform-provider-aws
3 participants
@bflad @breathingdust @ryndaniels