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

provider: Fix updated govet composites and nilness checks #7993

Merged
merged 3 commits into from
Mar 19, 2019

Conversation

bflad
Copy link
Contributor

@bflad bflad commented Mar 18, 2019

Community Note

  • Please vote on this pull request by adding a 👍 reaction to the original pull request comment to help the community and maintainers prioritize this request
  • Please do not leave "+1" comments, they generate extra noise for pull request followers and do not help prioritize the request

Closes #7992

Output from acceptance testing (test failures unrelated):

--- PASS: TestAccAWSDBInstance_iamAuth (433.73s)
--- PASS: TestAccAWSDBInstance_generatedName (433.85s)
--- PASS: TestAccAWSDBInstance_kmsKey (472.91s)
--- PASS: TestAccAWSDBInstance_IsAlreadyBeingDeleted (474.40s)
--- PASS: TestAccAWSDBInstance_basic (504.75s)
--- PASS: TestAccAWSDBInstance_namePrefix (515.35s)
--- PASS: TestAccAWSDBInstance_DeletionProtection (519.84s)
--- PASS: TestAccAWSDBInstance_optionGroup (524.56s)
--- PASS: TestAccAWSDBInstance_FinalSnapshotIdentifier_SkipFinalSnapshot (615.45s)
--- PASS: TestAccAWSDBInstance_FinalSnapshotIdentifier (889.07s)
--- PASS: TestAccAWSDBInstance_subnetGroup (941.03s)
--- FAIL: TestAccAWSDBInstance_S3Import (670.42s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb (1325.20s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_AutoMinorVersionUpgrade (1335.22s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_IamDatabaseAuthenticationEnabled (1397.87s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_Monitoring (1427.89s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_BackupWindow (1467.48s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_MaintenanceWindow (1505.86s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_BackupRetentionPeriod (1597.63s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_AvailabilityZone (1620.53s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_AutoMinorVersionUpgrade (1070.32s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier (1230.49s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_VpcSecurityGroupIds (1306.43s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_AllocatedStorage (1788.44s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_AllocatedStorage (1331.92s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_AvailabilityZone (1041.75s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_Port (1516.69s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_Io1Storage (1562.85s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_ParameterGroupName (1655.35s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_BackupRetentionPeriod (1422.63s)
--- PASS: TestAccAWSDBInstance_enhancedMonitoring (668.86s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_IamDatabaseAuthenticationEnabled (1123.73s)
--- PASS: TestAccAWSDBInstance_portUpdate (532.58s)
--- PASS: TestAccAWSDBInstance_ReplicateSourceDb_MultiAZ (2112.96s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_BackupWindow (1260.79s)
--- PASS: TestAccAWSDBInstance_separate_iops_update (698.74s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_DeletionProtection (1337.11s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_MaintenanceWindow (1322.37s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_ParameterGroupName (1211.08s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_Monitoring (1406.51s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_Port (1190.56s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_Tags (1130.64s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_BackupRetentionPeriod_Unset (1703.38s)
--- PASS: TestAccAWSDBInstance_diffSuppressInitialState (452.04s)
--- PASS: TestAccAWSDBInstance_ec2Classic (429.38s)
--- PASS: TestAccAWSDBInstance_MinorVersion (462.68s)
--- PASS: TestAccAWSEgressOnlyInternetGateway_basic (9.07s)
--- PASS: TestAccAWSEmrSecurityConfiguration_basic (11.70s)
--- PASS: TestAccAWSEmrSecurityConfiguration_importBasic (15.62s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_VpcSecurityGroupIds (1223.74s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_VpcSecurityGroupIds_Tags (1232.99s)
--- PASS: TestAccAWSVpnGateway_disappears (22.85s)
--- PASS: TestAccAWSVpnConnection_disappears (159.00s)
--- FAIL: TestAccAWSDBInstance_cloudwatchLogsExportConfigurationUpdate (560.67s)
--- PASS: TestAccAWSDBInstance_cloudwatchLogsExportConfiguration (700.56s)
--- PASS: TestAccAWSDBInstance_EnabledCloudwatchLogsExports_Oracle (745.47s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_MultiAZ (1935.89s)
--- PASS: TestAccAWSDBInstance_EnabledCloudwatchLogsExports_Postgresql (766.60s)
--- PASS: TestAccAWSDocDBClusterInstance_namePrefix (649.47s)
--- PASS: TestAccAWSDocDBClusterInstance_generatedName (659.47s)
--- PASS: TestAccAWSDocDBClusterInstance_az (676.29s)
--- PASS: TestAccAWSDocDBClusterInstance_kmsKey (694.98s)
--- PASS: TestAccAWSDocDBClusterInstance_disappears (640.17s)
--- PASS: TestAccAWSDBInstance_MSSQL_TZ (1792.55s)
--- PASS: TestAccAWSDocDBClusterInstance_basic (1339.75s)
--- PASS: TestAccAWSDBInstance_MySQL_SnapshotRestoreWithEngineVersion (2067.04s)
--- PASS: TestAccAWSDBInstance_MSSQL_DomainSnapshotRestore (3150.09s)
--- PASS: TestAccAWSDBInstance_SnapshotIdentifier_MultiAZ_SQLServer (4083.21s)
--- PASS: TestAccAWSDBInstance_MSSQL_Domain (3593.05s)

@bflad bflad added the technical-debt Addresses areas of the codebase that need refactoring or redesign. label Mar 18, 2019
@ghost ghost added size/M Managed by automation to categorize the size of a PR. service/ec2 Issues and PRs that pertain to the ec2 service. service/emr Issues and PRs that pertain to the emr service. service/gamelift Issues and PRs that pertain to the gamelift service. service/organizations Issues and PRs that pertain to the organizations service. service/rds Issues and PRs that pertain to the rds service. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure. labels Mar 18, 2019
Trusty is EOL soon and trying to workaround memory issues:

```
==> Checking source code against linters...
fatal error: runtime: out of memory
```
…iguration by default

We may want to wait until staticcheck 2019.1.1:

* golangci/golangci-lint#445
* https://github.com/dominikh/go-tools/releases/tag/2019.1.1

Still trying to workaround memory issues in TravisCI:

```
$ make lint
==> Checking source code against linters...
fatal error: runtime: out of memory```
```
@@ -57,14 +58,6 @@ func testSweepGameliftGameSessionQueue(region string) error {
}
}

if err != nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removing duplicate logic.

if err != nil {
if ec2err, ok := err.(awserr.Error); ok && ec2err.Code() == "InvalidVpnConnectionID.NotFound" {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The acceptance test should fail if this error is returned here.

@@ -374,18 +374,7 @@ func testAccAWSVpnGatewayDisappears(gateway *ec2.VpnGateway) resource.TestCheckF
VpcId: gateway.VpcAttachments[0].VpcId,
})
if err != nil {
ec2err, ok := err.(awserr.Error)
if ok {
if ec2err.Code() == "InvalidVpnGatewayID.NotFound" {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The acceptance test should fail if either of these errors are returned here.

@@ -1,34 +1,33 @@
dist: trusty
dist: xenial
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was updated to see if it helped with the memory issue since TravisCI does not start services by default on the newer OS. It didn't make the memory issue disappear, but the update is helpful anyways.

@@ -11,7 +11,6 @@ linters:
- gosimple
- ineffassign
- misspell
- staticcheck
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Temporary for now to fix out of memory issue. We can consider adding a separate TravisCI matrix job, running staticcheck by itself, or waiting for staticcheck 2019.1.1 support in golangci-lint:

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here's the memory usage difference locally for me:

# golangci-lint on master
6227824640  maximum resident set size
# golangci-lint 1.15.0
5142007808  maximum resident set size

We should also watch: dominikh/go-tools#419

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi! golangci/golangci-lint#445 was merged. Also, I've added the section about memory usage into README. You can try to set GOGC=30 for example to trade memory for CPU.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks so much @jirfag 😄 This should be a huge help for us.

@@ -58,21 +58,20 @@ func testAccCheckEmrSecurityConfigurationDestroy(s *terraform.State) error {
resp, err := conn.DescribeSecurityConfiguration(&emr.DescribeSecurityConfigurationInput{
Name: aws.String(rs.Primary.ID),
})
if err == nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Invert logic here to match preferred style across codebase of error checking first.

@bflad bflad requested a review from a team March 18, 2019 14:58
Copy link
Contributor

@nywilken nywilken left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 👍

@bflad bflad added this to the v2.3.0 milestone Mar 19, 2019
@bflad bflad merged commit dd27707 into master Mar 19, 2019
@bflad bflad deleted the td-govet-nilness branch March 19, 2019 14:02
bflad added a commit that referenced this pull request Mar 21, 2019
References:

* #7993
* https://github.com/go-modules-by-example/index/blob/master/010_tools/README.md

This installation method allows us to control tooling versions via Go Modules instead of receiving unexpected updates from latest upstream code. We prefer this method over installing binaries from shell scripts and needing to manually manage tooling versions in other ways.

We start by pinning golangci-lint@v1.15.0 and misspell@v0.3.4.
bflad added a commit that referenced this pull request Apr 1, 2019
References:
* https://github.com/golangci/golangci-lint/releases/tag/v1.16.0
* #7993 (comment)

Updated via:

```console
$ go get github.com/golangci/golangci-lint/cmd/golangci-lint@v1.16.0
$ go mod tidy
$ go mod vendor
```
@ghost
Copy link

ghost commented Mar 30, 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 30, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
service/ec2 Issues and PRs that pertain to the ec2 service. service/emr Issues and PRs that pertain to the emr service. service/gamelift Issues and PRs that pertain to the gamelift service. service/organizations Issues and PRs that pertain to the organizations service. service/rds Issues and PRs that pertain to the rds service. size/M Managed by automation to categorize the size of a PR. technical-debt Addresses areas of the codebase that need refactoring or redesign. tests PRs: expanded test coverage. Issues: expanded coverage, enhancements to test infrastructure.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Fix Updated govet Checks
3 participants