Fix and Enable go-mnd Linter for resource.Retry and resource.RetryContext Arguments #16753
Labels
linter
Pertains to changes to or issues with the various linters.
provider
Pertains to the provider itself, rather than any interaction with AWS.
stale
Old or inactive issues managed by automation, if no further action taken these will get closed.
technical-debt
Addresses areas of the codebase that need refactoring or redesign.
Comments
bflad
added
technical-debt
This was referenced Dec 14, 2020
|
Once golangci/golangci-lint#1557 is resolved, released, and the dependency is updated here, this configuration should get us close: linters-settings:
gomnd:
settings:
mnd:
checks:
- argument
ignored-functions:
- aws.Int64
- schema.DefaultTimeout
- validation.FloatBetween
- validation.IntAtLeast
- validation.IntBetween
- validation.StringLenBetween |
bflad
added a commit
that referenced
this issue
Feb 25, 2021
…enabling go-mnd linter Reference: #13199 Reference: #16752 Reference: #16753 IAM eventual consistency handling has long been the source of needing retries in resource logic. Due to the lack of a consistent implementation (e.g. static constant) for how long to retry for these types of errors, there have been varying retry durations. The `iamwaiter.PropagationTimeout` constant was introduced for this purpose. This change begins by introducing the `go-mnd` linter to enforce the usage of constants in function arguments. Example reports below. The rest of the changes are the minimum required to ensure `iamwaiter.PropagationTimeout` with its 2 minute duration is applied. You will note that this is fixing the duration in some cases to slightly increase it to the standard value. Any higher durations are ignored to reduce changes for now. As such, this can be reviewed by validating that a lower duration was not introduced and skipping acceptance testing since no logic changes should be introduced. One caveat to `go-mnd` is that it currently ignores `1` as a magic number, which is possible in usage such as `1*time.Minute`, and that ignored number cannot be overriden. An upstream issue will be created to ask the `ignore-number` configuration to overwrite instead of append. Example previous report: ``` aws/resource_aws_api_gateway_account.go:99:23: mnd: Magic number: 2, in <argument> detected (gomnd) err = resource.Retry(2*time.Minute, func() *resource.RetryError { ^ ```
bflad
added a commit
that referenced
this issue
Mar 22, 2021
…enabling go-mnd linter Reference: #13199 Reference: #16752 Reference: #16753 IAM eventual consistency handling has long been the source of needing retries in resource logic. Due to the lack of a consistent implementation (e.g. static constant) for how long to retry for these types of errors, there have been varying retry durations. The `iamwaiter.PropagationTimeout` constant was introduced for this purpose. This change begins by introducing the `go-mnd` linter to enforce the usage of constants in function arguments. Example reports below. The rest of the changes are the minimum required to ensure `iamwaiter.PropagationTimeout` with its 2 minute duration is applied. You will note that this is fixing the duration in some cases to slightly increase it to the standard value. Any higher durations are ignored to reduce changes for now. As such, this can be reviewed by validating that a lower duration was not introduced and skipping acceptance testing since no logic changes should be introduced. One caveat to `go-mnd` is that it currently ignores `1` as a magic number, which is possible in usage such as `1*time.Minute`, and that ignored number cannot be overriden. An upstream issue will be created to ask the `ignore-number` configuration to overwrite instead of append. Example previous report: ``` aws/resource_aws_api_gateway_account.go:99:23: mnd: Magic number: 2, in <argument> detected (gomnd) err = resource.Retry(2*time.Minute, func() *resource.RetryError { ^ ```
bflad
added a commit
that referenced
this issue
Mar 26, 2021
…enabling go-mnd linter (#17811) Reference: #13199 Reference: #16752 Reference: #16753 IAM eventual consistency handling has long been the source of needing retries in resource logic. Due to the lack of a consistent implementation (e.g. static constant) for how long to retry for these types of errors, there have been varying retry durations. The `iamwaiter.PropagationTimeout` constant was introduced for this purpose. This change begins by introducing the `go-mnd` linter to enforce the usage of constants in function arguments. Example reports below. The rest of the changes are the minimum required to ensure `iamwaiter.PropagationTimeout` with its 2 minute duration is applied. You will note that this is fixing the duration in some cases to slightly increase it to the standard value. Any higher durations are ignored to reduce changes for now. As such, this can be reviewed by validating that a lower duration was not introduced and skipping acceptance testing since no logic changes should be introduced. One caveat to `go-mnd` is that it currently ignores `1` as a magic number, which is possible in usage such as `1*time.Minute`, and that ignored number cannot be overriden. An upstream issue will be created to ask the `ignore-number` configuration to overwrite instead of append. Example previous report: ``` aws/resource_aws_api_gateway_account.go:99:23: mnd: Magic number: 2, in <argument> detected (gomnd) err = resource.Retry(2*time.Minute, func() *resource.RetryError { ^ ```
|
Marking this issue as stale due to inactivity. This helps our maintainers find and focus on the active issues. If this issue receives no comments in the next 30 days it will automatically be closed. Maintainers can also remove the stale label. If this issue was automatically closed and you feel this issue should be reopened, we encourage creating a new issue linking back to this one for added context. Thank you! |
github-actions
bot
added
the
stale
|
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. |
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Community Note
Description
NOTE: This requires upstream
golangci-lintimplementation work and release first.The
go-mndanalyzer is a magic number detector for Go. It currently is supported ingolangci-lint, however we do not enable it because it currently would have many false positives for our codebase. These false positives stem from schema validation and other cases where the magic numbers are determined by the AWS APIs, but not modeled as a constant we can reference in the AWS Go SDK, e.g.In particular though, one needed use case for
go-mndreports is usage ofresource.Retry()andresource.RetryContext()calls where thetime.Durationis a magic number. Many resources use this type of logic to handle eventual consistency issues in the API, however keeping them properly in sync is hard without a single constant to reference.In the future, we can determine how useful (and painful) it might be to enable
go-mndon other functions or detection types (such as assignment).Affected Resources
This will be updated when appropriate.
References
The text was updated successfully, but these errors were encountered: