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

Use revive instead of golint #1685

Merged
merged 2 commits into from
May 6, 2022
Merged

Conversation

pigletfly
Copy link
Contributor

@pigletfly pigletfly commented Apr 25, 2022

Signed-off-by: pigletfly [email protected]

Description

The linter 'golint' is deprecated (since v1.41.0).ref: golangci/golangci-lint#1892

Issue reference

We strive to have all PR being opened based on an issue, where the problem or feature have been discussed prior to implementation.

Please reference the issue this PR will close: #[issue number]

Checklist

Please make sure you've completed the relevant tasks for this PR, out of the following list:

@pigletfly pigletfly requested review from a team as code owners April 25, 2022 06:49
@pigletfly pigletfly force-pushed the remove-golint branch 3 times, most recently from 7e1b894 to 4f3047c Compare April 25, 2022 07:06
@daixiang0
Copy link
Member

daixiang0 commented Apr 25, 2022

We need to keep the same consistent code style across all projects, so please open an issue for this and track it.

Honestly, I do not want to make this change, since no useful and necessary linter was introduced.

@pigletfly
Copy link
Contributor Author

pigletfly commented Apr 25, 2022

We need to keep the same consistent code style across all projects, so please open an issue for this and track it.

Honestly, I do not want to make this change, since no useful and necessary linter was introduced.

Task list is created, #1687. As said in the description, revive is a drop-in replacement, golint repo is archived already.

.golangci.yml Outdated Show resolved Hide resolved
@shubham1172
Copy link
Member

We need to keep the same consistent code style across all projects, so please open an issue for this and track it.

Honestly, I do not want to make this change, since no useful and necessary linter was introduced.

golint is now deprecated in favor of revive. We should update it here too.

bindings/http/http.go Outdated Show resolved Hide resolved
state/redis/redis.go Outdated Show resolved Hide resolved
Copy link
Member

@shubham1172 shubham1172 left a comment

Choose a reason for hiding this comment

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

Please address the requested changes.

Copy link
Member

@berndverst berndverst left a comment

Choose a reason for hiding this comment

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

Please address the feedback from @shubham1172

@pigletfly pigletfly force-pushed the remove-golint branch 2 times, most recently from ddc6177 to a269c47 Compare April 26, 2022 01:51
@pigletfly
Copy link
Contributor Author

a lot of new lint errors are coming.

@shubham1172
Copy link
Member

@pigletfly Are the linter errors introduced due to golintci's version upgrade? If yes, we might need to fix them too.

@pigletfly
Copy link
Contributor Author

pigletfly commented Apr 26, 2022

@pigletfly Are the linter errors introduced due to golintci's version upgrade? If yes, we might need to fix them too.

I tested locally.It's the golangci-lint version upgrade.There are more linters in golangci-lint in newer version.One possible solution is we just choose linters that we want, such as

linters:
  fast: false
  enable:
    - revive

@pigletfly
Copy link
Contributor Author

@daixiang0 any ideas ?

@shubham1172
Copy link
Member

If there are some linters enabled out of box in the newer versions, we can choose to keep it (unless we are strongly opinionated about it).

@pigletfly
Copy link
Contributor Author

pigletfly commented Apr 26, 2022

these are new linters:

    - tparallel
    - paralleltest
    - wrapcheck
    - tagliatelle
    - ireturn
    - exhaustivestruct
    - errchkjson
    - contextcheck
    - gomoddirectives
    - godot
    - cyclop
    - varnamelen
    - gosec
    - errorlint
    - forcetypeassert
    - ifshort
    - maintidx
    - nilnil
    - predeclared
    - tenv
    - thelper
    - wastedassign
    - containedctx
    - gosimple

I disabled them locally,then there are only some stylecheck and staticcheck lint errors.

@shubham1172
Copy link
Member

@berndverst do you think we should track a separate issue to re-enable the new linters and fix the linting issues in our code?

@ItalyPaleAle
Copy link
Contributor

ItalyPaleAle commented May 5, 2022

Note that as I just learnt, when updating to Go 1.18 a bunch of linters get disabled.

On dapr/dapr, with Go 1.18, golangci-lint now shows this:

WARN [linters context] bodyclose is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649. 
WARN [linters context] nilerr is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649. 
WARN [linters context] rowserrcheck is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649. 
WARN [linters context] sqlclosecheck is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649. 
WARN [linters context] staticcheck is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649. 
WARN [linters context] structcheck is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649. 
WARN [linters context] stylecheck is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649. 
WARN [linters context] unused is disabled because of go1.18. You can track the evolution of the go1.18 support by following the https://github.com/golangci/golangci-lint/issues/2649. 

This has caused MORE linter failures in the PR that switched to Go 1.18 because lint rules such as "nilerr" don't exist anymore, so //nolint:nilerr in itself throws an error.

So I wouldn't stress too much about what linters are enabled or disabled here, as when the PR to upgrade to Go 1.18 is merged, we'll have to revisit that anyways.

@daixiang0
Copy link
Member

We should track linter changes and re-enable them when they are ready for Go 1.18.

@ItalyPaleAle
Copy link
Contributor

We should track linter changes and re-enable them when they are ready for Go 1.18.

For the linters that are not available in 1.18, they are not explicitly disabled, so once golangci-lint is updated and adds them, they should be re-enabled automatically.

For the linters that were explicitly disabled in this PR, that's a different story.

@daixiang0
Copy link
Member

Agree, but we need to update golangci-lint version, so still need to track.

@pigletfly pigletfly mentioned this pull request May 6, 2022
3 tasks
@codecov
Copy link

codecov bot commented May 6, 2022

Codecov Report

Merging #1685 (9deeddf) into master (bfd87eb) will increase coverage by 0.01%.
The diff coverage is 0.00%.

@@            Coverage Diff             @@
##           master    #1685      +/-   ##
==========================================
+ Coverage   36.37%   36.38%   +0.01%     
==========================================
  Files         166      166              
  Lines       15488    15488              
==========================================
+ Hits         5633     5635       +2     
+ Misses       9228     9227       -1     
+ Partials      627      626       -1     
Impacted Files Coverage Δ
bindings/azure/cosmosdb/cosmosdb.go 18.00% <ø> (ø)
bindings/cron/cron.go 87.23% <ø> (ø)
bindings/http/http.go 87.87% <ø> (ø)
nameresolution/consul/configuration.go 96.59% <ø> (ø)
pubsub/azure/servicebus/message.go 85.84% <ø> (ø)
state/postgresql/postgresdbaccess.go 38.60% <0.00%> (ø)
state/redis/redis.go 35.00% <ø> (ø)
nameresolution/mdns/mdns.go 70.67% <0.00%> (+0.55%) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 47db6cc...9deeddf. Read the comment docs.

@berndverst berndverst merged commit db99b43 into dapr:master May 6, 2022
@pigletfly pigletfly deleted the remove-golint branch May 7, 2022 03:36
ItalyPaleAle pushed a commit to ItalyPaleAle/dapr-components-contrib that referenced this pull request May 7, 2022
Signed-off-by: pigletfly <[email protected]>

Co-authored-by: Yaron Schneider <[email protected]>
ItalyPaleAle pushed a commit to ItalyPaleAle/dapr-components-contrib that referenced this pull request May 7, 2022
Signed-off-by: pigletfly <[email protected]>

Co-authored-by: Yaron Schneider <[email protected]>
Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>
ChenCong0207 pushed a commit to ChenCong0207/components-contrib that referenced this pull request May 8, 2022
Signed-off-by: pigletfly <[email protected]>

Co-authored-by: Yaron Schneider <[email protected]>
Signed-off-by: Chen Cong <[email protected]>
@yaron2 yaron2 added this to the v1.8 milestone May 9, 2022
ewassef pushed a commit to ewassef/components-contrib that referenced this pull request May 13, 2022
Signed-off-by: pigletfly <[email protected]>

Co-authored-by: Yaron Schneider <[email protected]>
Signed-off-by: Eddie <[email protected]>
yaron2 added a commit that referenced this pull request May 13, 2022
* Update readme of bindings (#1690)

Signed-off-by: pigletfly <[email protected]>

Co-authored-by: Looong Dai <[email protected]>
Signed-off-by: Eddie <[email protected]>

* Fixing includedHeaders problem with spaces (#1610)

Signed-off-by: Ben Kotvis <[email protected]>

Co-authored-by: Bernd Verst <[email protected]>
Co-authored-by: Looong Dai <[email protected]>
Signed-off-by: Eddie <[email protected]>

* Simplify vault token read (#1560)

* Simplify vault token get

Signed-off-by: zhangchao <[email protected]>

* fix lint

Signed-off-by: zhangchao <[email protected]>

* update tests

Signed-off-by: zhangchao <[email protected]>

Co-authored-by: Looong Dai <[email protected]>
Co-authored-by: Yaron Schneider <[email protected]>
Signed-off-by: Eddie <[email protected]>

* GH-1609 : Fix for MongoDB Atlas conn strings

Added recommended fix in the issue

Signed-off-by: Eddie <[email protected]>

* updating the comment based on PR feedback

Signed-off-by: Eddie <[email protected]>

* Initial Certification test for eventhubs binding [incomplete] (#1670)

* certification test for eventhubs binding

Signed-off-by: tanvigour <[email protected]>

* modified go.mod and go.sum

Signed-off-by: tanvigour <[email protected]>

* Add connection string testing

Signed-off-by: tanvigour <[email protected]>

* iothub testing

Signed-off-by: tanvigour <[email protected]>

* address feedback and run test

Signed-off-by: tanvigour <[email protected]>

* Install Azure CLI IOT hub extension

Signed-off-by: Bernd Verst <[email protected]>

* make modtidy-all

Signed-off-by: Bernd Verst <[email protected]>

* covering all eventhubs test cases

Signed-off-by: tanvigour <[email protected]>

* dependency changes after go modtidy-all

Signed-off-by: tanvigour <[email protected]>

Co-authored-by: Bernd Verst <[email protected]>
Co-authored-by: Yaron Schneider <[email protected]>
Co-authored-by: Looong Dai <[email protected]>
Signed-off-by: Eddie <[email protected]>

* Use revive instead of golint (#1685)

Signed-off-by: pigletfly <[email protected]>

Co-authored-by: Yaron Schneider <[email protected]>
Signed-off-by: Eddie <[email protected]>

* Updated to Go 1.18 (#1697)

* Updated to Go 1.18

Signed-off-by: Alessandro (Ale) Segala <[email protected]>

* Added go.work file
With Go 1.18, this allows gopls (the Go language server used for example in VS Code) to work inside test apps too.
See: https://go.dev/doc/tutorial/workspaces

Signed-off-by: ItalyPaleAle <[email protected]>

Signed-off-by: ItalyPaleAle <[email protected]>

* Removed go.work

Signed-off-by: ItalyPaleAle <[email protected]>

* 💄

Signed-off-by: ItalyPaleAle <[email protected]>

Co-authored-by: Bernd Verst <[email protected]>
Signed-off-by: Eddie <[email protected]>

* Add metadata property to configure Batching in Pulsar (#1707)

* Add metadata property to configure BatchingMaxSize&batchingMaxMessages in Pulsar
Signed-off-by: saberwang <[email protected]>

* sort field
Signed-off-by: saberwang <[email protected]>

* [pubsub]fix unit test bug
Signed-off-by: saberwang <[email protected]>

* remove unrelated changes
Signed-off-by: saberwang <[email protected]>

* Delete hard coded Metadata
Signed-off-by: saberwang <[email protected]>

* remove  .history

Signed-off-by: saberwang <[email protected]>

* restore .gitignore

Signed-off-by: saberwang <[email protected]>

* Hard coding default values and adding 'BatchingMaxPublishDelay' metadata

Signed-off-by: saberwang <[email protected]>

* fix code format

Signed-off-by: saberwang <[email protected]>

* formatting code

Signed-off-by: saberwang <[email protected]>

Co-authored-by: Looong Dai <[email protected]>
Co-authored-by: Bernd Verst <[email protected]>
Signed-off-by: Eddie <[email protected]>

* Fix 4529: Ignore Subscribe/Get wrong redis configuration type keys. (#1693)

* fix: 4529

Signed-off-by: LaurenceLiZhixin <[email protected]>

* Fix: add test does not throw error for wrong type during get all test case of redis configuration

Signed-off-by: LaurenceLiZhixin <[email protected]>

Co-authored-by: Yaron Schneider <[email protected]>
Co-authored-by: Ian Luo <[email protected]>
Signed-off-by: Eddie <[email protected]>

* Signed-off-by: Eddie Wassef <[email protected]>

Fixing leading newline in linter error

Signed-off-by: Eddie <[email protected]>

* Implment a secret store based on Huawei CSMS (#1710)

Signed-off-by: Chen Cong <[email protected]>

Co-authored-by: Chen Cong <[email protected]>
Signed-off-by: Eddie <[email protected]>

* Add yet another missing secret to eventhub binding cert test (#1713)

Signed-off-by: Bernd Verst <[email protected]>
Signed-off-by: Eddie <[email protected]>

* Support custom queueEndpoint in Azure Storage Queues (#1692)

* Support custom queueEndpoint in Azure Storage Queues

Signed-off-by: Janusz Dziurzynski <[email protected]>

* run gofmt

Signed-off-by: Janusz Dziurzynski <[email protected]>

* Add "Url" to JSON field name for clarity

Suggested by @msfussell in dapr/docs#2424

Signed-off-by: Janusz Dziurzynski <[email protected]>
Signed-off-by: Eddie <[email protected]>

* Refactory kafka binding to reuse the kafka common code extracting from kafka pubsub component (#1696)

* refactory kafka pubsub code to extract common kafka code for reuse

Signed-off-by: Sky Ao <[email protected]>

* fix lint;add unit test for subscribeAdapter

Signed-off-by: Sky Ao <[email protected]>

* move topics filed from internal kafak struct to pubsub kafka struct, since in input binding the topics will confiured in metadata

Signed-off-by: Sky Ao <[email protected]>

* reuse internal  kafka code for bindings

Signed-off-by: Sky Ao <[email protected]>

* add redis standalone_test back which is delete by mistaken

Signed-off-by: Sky Ao <[email protected]>

* small code improvement to trigger test

Signed-off-by: Sky Ao <[email protected]>

* add license headers

Signed-off-by: Sky Ao <[email protected]>

* try to set disbaleTls to true to verify the kafka connection fail

Signed-off-by: Sky Ao <[email protected]>

* don't enable consum retry in kafka binding component;if authenticaion is disabled, need not set TLSDisable at the same time;

Signed-off-by: Sky Ao <[email protected]>

* fix lint

Signed-off-by: Sky Ao <[email protected]>

Co-authored-by: Loong Dai <[email protected]>
Signed-off-by: Eddie <[email protected]>

* Add topic metadata for mqtt input binding and support user defined topic for mqtt output binding (#1674)

* feat(bindings/mqtt): add data incoming topic to metadata

Signed-off-by: lotuc <[email protected]>

* feat(bindings/mqtt): support user defined topic on create action

Signed-off-by: lotuc <[email protected]>

* chore(bindings/mqtt): add integration test and topic response check test

Signed-off-by: lotuc <[email protected]>

* fix(bindings/mqtt): ignore misspell linting error for word mosquitto

Signed-off-by: lotuc <[email protected]>

Co-authored-by: Yaron Schneider <[email protected]>
Signed-off-by: Eddie <[email protected]>

* Expire -> ExpiryInSeconds (#1721)

Signed-off-by: seeflood <[email protected]>
Signed-off-by: Eddie <[email protected]>

* running gofmt -s -w state/mongodb/mongodb.go

Signed-off-by: Eddie <[email protected]>

* Update mongodb.go

Co-authored-by: Wang Bing <[email protected]>
Co-authored-by: Looong Dai <[email protected]>
Co-authored-by: Ben Kotvis <[email protected]>
Co-authored-by: Bernd Verst <[email protected]>
Co-authored-by: Taction <[email protected]>
Co-authored-by: Yaron Schneider <[email protected]>
Co-authored-by: tanvigour <[email protected]>
Co-authored-by: Alessandro (Ale) Segala <[email protected]>
Co-authored-by: saber-wang <[email protected]>
Co-authored-by: Laurence <[email protected]>
Co-authored-by: Ian Luo <[email protected]>
Co-authored-by: Chock Chen <[email protected]>
Co-authored-by: Chen Cong <[email protected]>
Co-authored-by: Janusz Dziurzynski <[email protected]>
Co-authored-by: Sky Ao <[email protected]>
Co-authored-by: lotuc <[email protected]>
Co-authored-by: seeflood <[email protected]>
berndverst added a commit that referenced this pull request May 13, 2022
* Updated to Go 1.18

Signed-off-by: Alessandro (Ale) Segala <[email protected]>
Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

* Updated Azure SDKs that are on track2
Includes some minor refactoring of auth code

Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

* Updated Service Bus components to track2 SDK

Co-authored-by: halspang <[email protected]>
Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

* Initial Certification test for eventhubs binding [incomplete] (#1670)

* certification test for eventhubs binding

Signed-off-by: tanvigour <[email protected]>

* modified go.mod and go.sum

Signed-off-by: tanvigour <[email protected]>

* Add connection string testing

Signed-off-by: tanvigour <[email protected]>

* iothub testing

Signed-off-by: tanvigour <[email protected]>

* address feedback and run test

Signed-off-by: tanvigour <[email protected]>

* Install Azure CLI IOT hub extension

Signed-off-by: Bernd Verst <[email protected]>

* make modtidy-all

Signed-off-by: Bernd Verst <[email protected]>

* covering all eventhubs test cases

Signed-off-by: tanvigour <[email protected]>

* dependency changes after go modtidy-all

Signed-off-by: tanvigour <[email protected]>

Co-authored-by: Bernd Verst <[email protected]>
Co-authored-by: Yaron Schneider <[email protected]>
Co-authored-by: Looong Dai <[email protected]>
Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

* Use revive instead of golint (#1685)

Signed-off-by: pigletfly <[email protected]>

Co-authored-by: Yaron Schneider <[email protected]>
Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

* Updated to Go 1.18 (#1697)

* Updated to Go 1.18

Signed-off-by: Alessandro (Ale) Segala <[email protected]>

* Added go.work file
With Go 1.18, this allows gopls (the Go language server used for example in VS Code) to work inside test apps too.
See: https://go.dev/doc/tutorial/workspaces

Signed-off-by: ItalyPaleAle <[email protected]>

Signed-off-by: ItalyPaleAle <[email protected]>

* Removed go.work

Signed-off-by: ItalyPaleAle <[email protected]>

* 💄

Signed-off-by: ItalyPaleAle <[email protected]>

Co-authored-by: Bernd Verst <[email protected]>
Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

* 💄 & 🧹

Signed-off-by: ItalyPaleAle <[email protected]>
Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

* Add metadata property to configure Batching in Pulsar (#1707)

* Add metadata property to configure BatchingMaxSize&batchingMaxMessages in Pulsar
Signed-off-by: saberwang <[email protected]>

* sort field
Signed-off-by: saberwang <[email protected]>

* [pubsub]fix unit test bug
Signed-off-by: saberwang <[email protected]>

* remove unrelated changes
Signed-off-by: saberwang <[email protected]>

* Delete hard coded Metadata
Signed-off-by: saberwang <[email protected]>

* remove  .history

Signed-off-by: saberwang <[email protected]>

* restore .gitignore

Signed-off-by: saberwang <[email protected]>

* Hard coding default values and adding 'BatchingMaxPublishDelay' metadata

Signed-off-by: saberwang <[email protected]>

* fix code format

Signed-off-by: saberwang <[email protected]>

* formatting code

Signed-off-by: saberwang <[email protected]>

Co-authored-by: Looong Dai <[email protected]>
Co-authored-by: Bernd Verst <[email protected]>
Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

* This test can't work with track2 SDKs
The methods to create a message with a body are not exported

Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

* 🧹

Signed-off-by: Alessandro Segala (ItalyPaleAle) <[email protected]>

* There's such thing as too much logging

Signed-off-by: ItalyPaleAle <[email protected]>

* Refactored subscription.go
This greatly simplifies certain parts of the code, reducing the number of goroutines and likely improving performance.
Performance for end-users improves too as there's no need anymore to pause for 2 seconds every time that we reach `maxActiveMessages`.
Additionally, with this change the config options `prefetchCount` and `maxActiveMessagesRecoveryInSec` are removed as unnecessary anymore.

Signed-off-by: ItalyPaleAle <[email protected]>

* 💄

Signed-off-by: ItalyPaleAle <[email protected]>

* Fixed pubsub tests

Signed-off-by: ItalyPaleAle <[email protected]>

* These packages should have never been upgraded

Signed-off-by: ItalyPaleAle <[email protected]>

* Ensuring we don't fetch 1 message more than max active

Signed-off-by: ItalyPaleAle <[email protected]>

* Adding configurable timeout for servicebusqueues operations

Signed-off-by: ItalyPaleAle <[email protected]>

* Persistent connection for invoking SB queues

Signed-off-by: ItalyPaleAle <[email protected]>

* Reverted Event Hub SDK update

Signed-off-by: ItalyPaleAle <[email protected]>

* Revert "Reverted Event Hub SDK update"

This reverts commit 212220a.

Signed-off-by: ItalyPaleAle <[email protected]>

* Fix Azure deploy for users with a first.last email

Signed-off-by: ItalyPaleAle <[email protected]>

* Added some sleep that should help reduce flakiness in eventhubs binding cert test

Signed-off-by: ItalyPaleAle <[email protected]>

* Changed servicebusqueue cert test
In case of a failure in the handler (ie. users' code), the message should be correctly re-enqueued, which means that messages will be re-delivered later and won't be in order.

Signed-off-by: ItalyPaleAle <[email protected]>

Co-authored-by: halspang <[email protected]>
Co-authored-by: tanvigour <[email protected]>
Co-authored-by: Bernd Verst <[email protected]>
Co-authored-by: Yaron Schneider <[email protected]>
Co-authored-by: Looong Dai <[email protected]>
Co-authored-by: Wang Bing <[email protected]>
Co-authored-by: saber-wang <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants