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.

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

Fix and enable tfproviderlint R003 check: Remove extraneous Exists functions #9953

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

Fix and enable tfproviderlint R003 check: Remove extraneous Exists functions #9953

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

Comments

@ryndaniels
Copy link
Contributor

ryndaniels commented Sep 2, 2019

Description

As per tfproviderlint check R003, we want to remove extraneous Exists functions from resources. This logic can be handled inside Read functions instead to prevent duplication.

Failing example:

func resourceExampleThingExists(d *schema.ResourceData, meta interface{}) (bool, error) { /* ... */ }

func resourceExampleThingRead(d *schema.ResourceData, meta interface{}) error { /* ... */ }

&schema.Resource{
    Exists: resourceExampleThingExists,
    Read:   resourceExampleThingRead,
    /* ... */
}

Should become:



func resourceExampleThingRead(d *schema.ResourceData, meta interface{}) error { /* ... */ }

&schema.Resource{
    Read: resourceExampleThingRead,
    /* ... */
}

Definition of Done

  • In GNUmakefile, add -R003 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 Remove extraneous Exists functions from acceptance tests Remove extraneous Exists functions 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 changed the title Remove extraneous Exists functions Fix and enable tfproviderlint R003 check: Remove extraneous Exists functions Feb 3, 2020
bflad added a commit that referenced this issue Feb 3, 2020
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 4, 2020
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 19, 2020
Reference: #12074

Gaps in `tfproviderlint`/`awsproviderlint` checking are covered by the following issues:

Reference: #11888 (fix and enable AWSAT001)
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: #11863 (fix and enable R005)
Reference: #11864 (fix and enable R006)
Reference: #12083 (fix and enable R007, R008)
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 Mar 10, 2020
Reference: #12074

Gaps in `tfproviderlint`/`awsproviderlint` checking are covered by the following issues:

Reference: #11888 (fix and enable AWSAT001)
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: #11863 (fix and enable R005)
Reference: #11864 (fix and enable R006)
Reference: #12083 (fix and enable R007, R008)
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 Apr 8, 2020
…creation

Reference: #9953
Reference: #11781
Reference: #12427 (comment)

This refactors the resource logic to prevent `Update` after `Create` type logic errors with duplicate API calls (potential error points for eventual consistency):

- Setting `description` on creation previously was done once during the `CreateKey` call and again via a separate `UpdateKeyDescription` call
- Setting `policy` on creation previously was done once during the `CreateKey` call and again via a separate `PutKeyPolicy` call
- Setting `tags` on creation previously was done once during the `CreateKey` call and again via a separate `TagResource` call

This also adds eventual consistency retries for reading tags on resource creation and removes the resource `Exists` function, which can be another source of issues and required for the upcoming Terraform Plugin SDK v2.

Previously from operator error reports:

```
Error: error listing tags for KMS Key (***): NotFoundException: Key 'arn:aws:kms:***:key/***' does not exist

Error: error updating KMS Key (key-123) tags: error tagging resource (key-123): NotFoundException: Key 'arn:aws:kms:us-east-1:1234567890:key/key-123' does not exist
```

Output from acceptance testing:

```
--- PASS: TestAccAWSKmsKey_disappears (14.50s)
--- PASS: TestAccAWSKmsKey_asymmetricKey (40.34s)
--- PASS: TestAccAWSKmsKey_basic (43.60s)
--- PASS: TestAccAWSKmsKey_policy (58.38s)
--- PASS: TestAccAWSKmsKey_tags (59.07s)
--- PASS: TestAccAWSKmsKey_isEnabled (324.81s)
```
@ewbankkit
Copy link
Contributor

After all the linked PRs are merged we can enable R003 in GNUmakefile.

adamdecaf pushed a commit to adamdecaf/terraform-provider-aws that referenced this issue May 28, 2020
…creation

Reference: hashicorp#9953
Reference: hashicorp#11781
Reference: hashicorp#12427 (comment)

This refactors the resource logic to prevent `Update` after `Create` type logic errors with duplicate API calls (potential error points for eventual consistency):

- Setting `description` on creation previously was done once during the `CreateKey` call and again via a separate `UpdateKeyDescription` call
- Setting `policy` on creation previously was done once during the `CreateKey` call and again via a separate `PutKeyPolicy` call
- Setting `tags` on creation previously was done once during the `CreateKey` call and again via a separate `TagResource` call

This also adds eventual consistency retries for reading tags on resource creation and removes the resource `Exists` function, which can be another source of issues and required for the upcoming Terraform Plugin SDK v2.

Previously from operator error reports:

```
Error: error listing tags for KMS Key (***): NotFoundException: Key 'arn:aws:kms:***:key/***' does not exist

Error: error updating KMS Key (key-123) tags: error tagging resource (key-123): NotFoundException: Key 'arn:aws:kms:us-east-1:1234567890:key/key-123' does not exist
```

Output from acceptance testing:

```
--- PASS: TestAccAWSKmsKey_disappears (14.50s)
--- PASS: TestAccAWSKmsKey_asymmetricKey (40.34s)
--- PASS: TestAccAWSKmsKey_basic (43.60s)
--- PASS: TestAccAWSKmsKey_policy (58.38s)
--- PASS: TestAccAWSKmsKey_tags (59.07s)
--- PASS: TestAccAWSKmsKey_isEnabled (324.81s)
```
@ghost
Copy link

ghost commented May 31, 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 May 31, 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.
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
3 participants