8000 Go vet and staticcheck with gov1.20.5 by troy0820 · Pull Request #2792 · getporter/porter · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

Go vet and staticcheck with gov1.20.5 #2792

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
Merged
merged 1 commit into from
Jun 23, 2023
Merged

Go vet and staticcheck with gov1.20.5 #2792

merged 1 commit into from
Jun 23, 2023

Conversation

troy0820
Copy link
Member

What does this change

Using gov1.20.5, performing go vet ./... and staticcheck ./... produces a number of errors that need to be fixed. This PR fixes them and adjusts the test to not only run the last value in the iteration of tests but maximizes the t.Parallel() with go test -v -race ./...

go vet:

# get.porter.sh/porter/pkg/cnab
pkg/cnab/extensions_test.go:84:38: loop variable supported captured by func literal
pkg/cnab/extensions_test.go:86:21: loop variable supported captured by func literal
pkg/cnab/extensions_test.go:92:38: loop variable supported captured by func literal
pkg/cnab/extensions_test.go:94:21: loop variable supported captured by func literal
# get.porter.sh/porter/pkg/cache
pkg/cache/cache_test.go:174:10: loop variable tc captured by func literal
pkg/cache/cache_test.go:232:10: loop variable tc captured by func literal
# get.porter.sh/porter/pkg/cnab/dependencies/v1
pkg/cnab/dependencies/v1/solver_test.go:86:10: loop variable tc captured by func literal
# get.porter.sh/porter/pkg/cnab/config-adapter
pkg/cnab/config-adapter/adapter_test.go:321:10: loop variable tc captured by func literal
pkg/cnab/config-adapter/adapter_test.go:932:10: loop variable tc captured by func literal
# get.porter.sh/porter/pkg/porter
pkg/porter/parameters_test.go:584:80: loop variable action captured by func literal

staticcheck:

pkg/porter/list_test.go:21:3: this value of i is never used (SA4006)
pkg/porter/list_test.go:42:3: this value of i is never used (SA4006)
pkg/porter/list_test.go:111:2: this value of i is never used (SA4006)

What issue does it fix

Closes #2727

Notes for the reviewer

Put any questions or notes for the reviewer here.

Checklist

  • Did you write tests?
    • I had to adjust the tests with the output of go vet ./... and the staticcheck results. The adjustments came to optimize what we need from t.Parallel() so we don't just test the last value in the test cases.
  • Did you write documentation?
    • No
  • Did you change porter.yaml or a storage document record? Update the corresponding schema file.
  • If this is your first pull request, please add your name to the bottom of our Contributors list. Thank you for making Porter better! 🙇‍♀️

Reviewer Checklist

  • Comment with /azp run test-porter-release if a magefile or build script was modified
  • Comment with /azp run porter-integration if it's a non-trivial PR

troy0820
troy0820 commented Jun 22, 2023
View reviewed changes
@troy0820 troy0820 force-pushed the troy0820/lint-issues branch from fa45a36 to 3e0398d Compare June 22, 2023 21:31
schristoff
schristoff approved these changes Jun 23, 2023
View reviewed changes
Copy link
Member
@schristoff schristoff left a comment

Choose a reason for hiding this comment

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

I'll leave it up to the pipeline to yell if this royally upset something, but overall seems great. Thank you for these improvements :)

pkg/cache/cache_test.go Outdated
Comment on lines 172 to 173
for name, test := range tests {
test := test
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
for name, test := range tests {
test := test
for name, tc := range tests {
tc := tc

Minor nit so this is similar with other tests

Copy link
Member Author

Choose a reason for hiding this comment

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

This has been updated on the latest push.

pkg/porter/list_test.go Outdated
require.NoError(t, err, "ReadInstallation failed")

di := NewDisplayInstallation(i)
di := NewDisplayInstallation(install)
Copy link
Member
8000

Choose a reason for hiding this comment

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

Suggested change
di := NewDisplayInstallation(install)
displayinstall := NewDisplayInstallation(install)

I feel like if we make i less ambiguous we should follow the same with di - but open to just leaving it because it is nbd

Copy link
Member Author

Choose a reason for hiding this comment

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

This has been updated on the latest push.

@schristoff
Copy link
Member

/azp run porter-integration

@azure-pipelines
Copy link
Azure Pipelines successfully started running 1 pipeline(s).

@troy0820 troy0820 force-pushed the troy0820/lint-issues branch from 3e0398d to 3dc065b Compare June 23, 2023 03:42
@troy0820
go vet/staticcheck with gov1.20
367913c 
adjusted test to run all instead of the last one in the list

Signed-off-by: Troy Connor <troy0820@users.noreply.github.com>
@troy0820 troy0820 force-pushed the troy0820/lint-issues branch from 3dc065b to 367913c Compare June 23, 2023 03:43
@troy0820
Copy link
Member Author

/azp run porter-integration

@azure-pipelines
Copy link
Commenter does not have sufficient privileges for PR 2792 in repo getporter/porter

@schristoff schristoff merged commit 13387ca into getporter:main Jun 23, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@schristoff schristoff schristoff approved these changes

@sgettys sgettys Awaiting requested review from sgettys sgettys is a code owner

@bdegeeter bdegeeter Awaiting requested review from bdegeeter bdegeeter is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Fix lint and vet issues found on Go 1.20
2 participants
@troy0820 @schristoff
0