Add golangci config file and fix linter issues #108

PanGan21 · 2023-03-08T22:26:36Z

nguyer · 2023-03-16T16:31:35Z

@PanGan21 Looks like this is on the right track! Now it's just a matter of working through all of the issues that the linter is catching.

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

PanGan21 · 2023-03-20T17:34:56Z

internal/rest/restgateway.go

@@ -133,7 +133,7 @@ func (g *RESTGateway) ValidateConf() error {
 }

 // Start kicks off the HTTP listener and router
-func (g *RESTGateway) Start() error {


It seems that I need to add a ReadHeaderTimeout because of the following error from gosec:

G112: Potential Slowloris Attack because ReadHeaderTimeout is not configured in the http.Server (gosec)

Should I add that in RESTGatewayConf ? What is recommended?

I ignored the linter on those places for now

That's fine for now. Can you put a // TODO: comment in the code explaining that please?

nguyer · 2023-09-15T19:20:11Z

@PanGan21 Apologies that I didn't realize this one was ready for re-review. Your comment brought it back up in my inbox and I'll take another look now. Thanks!

nguyer · 2023-09-15T19:21:24Z

internal/errors/errors.go

@@ -158,7 +158,7 @@ const (
 	// LevelDBFailedRetriveOriginalKey problem retrieving entry - original key
 	LevelDBFailedRetriveOriginalKey = "Failed to retrieve the entry for the original key: %s. %s"
 	// LevelDBFailedRetriveGeneratedID problem retrieving entry - generated ID
-	LevelDBFailedRetriveGeneratedID = "Failed to retrieve the entry for the generated ID: %s. %s"
+	LevelDBFailedRetriveGeneratedID = "failed to retrieve the entry for the generated ID: %s. %s"


Just curious why this one was lowercased but not the others?

That's because I am getting this:

internal/rest/receipt/leveldb.go:227:15: ST1005: error strings should not be capitalized (stylecheck) return nil, fmt.Errorf(errors.LevelDBFailedRetriveGeneratedID, requestID, err)

nguyer · 2023-09-15T20:09:55Z

internal/rest/receipt/leveldb.go

@@ -44,7 +44,7 @@ type levelDBReceipts struct {
 func newLevelDBReceipts(conf *conf.ReceiptsDBConf) *levelDBReceipts {
 	store := kvstore.NewLDBKeyValueStore(conf.LevelDB.Path)
 	t := time.Unix(1000000, 0)
-	entropy := ulid.Monotonic(rand.New(rand.NewSource(t.UnixNano())), 0)
+	entropy := ulid.Monotonic(rand.New(rand.NewSource(t.UnixNano())), 0) // #nosec


I'm curious why the #nosec on this one?

It actually throws this error:

internal/rest/receipt/leveldb.go:47:28: G404: Use of weak random number generator (math/rand instead of crypto/rand) (gosec) entropy := ulid.Monotonic(rand.New(rand.NewSource(t.UnixNano())), 0)

nguyer · 2023-09-15T20:17:21Z

internal/utils/tlsutils.go

@@ -61,11 +61,11 @@ func CreateTLSConfiguration(tlsConfig *conf.TLSConfig) (t *tls.Config, err error
 		caCertPool = x509.NewCertPool()
 		caCertPool.AppendCertsFromPEM(caCert)
 	}
-
+	// #nosec G402


Do you mind putting a comment here as to why this is required also please?

added todo as well

nguyer

This looks great! Just a couple questions and minor places a comment could be helpful, but I'm happy to merge this after that!

PanGan21 · 2023-09-16T17:33:49Z

.golangci.yml

+  disable:
+    - structcheck
+  enable:
+    # - depguard


I had to disable this linter because it throws error for many packages being used. Maybe another issue is required to upgrade those packages which might require more fixes.

I actually removed the depguard linter because I saw that it is also removed here: https://github.com/hyperledger/firefly/blob/main/.golangci.yml

PanGan21 · 2023-09-16T17:38:00Z

@PanGan21 Apologies that I didn't realize this one was ready for re-review. Your comment brought it back up in my inbox and I'll take another look now. Thanks!

No worries, i also have forgotten to submit my comments so i did it now😅
A bit of a hassle to fix all the conflicts after such long period but managed it.

codecov-commenter · 2023-09-18T20:46:11Z

Codecov Report

Merging #108 (ed7b113) into main (2346ff4) will increase coverage by 0.11%.
The diff coverage is 56.35%.

❗ Current head ed7b113 differs from pull request most recent head 6385858. Consider uploading reports for the commit 6385858 to get more accurate results

@@            Coverage Diff             @@
##             main     #108      +/-   ##
==========================================
+ Coverage   68.56%   68.67%   +0.11%     
==========================================
  Files          47       47              
  Lines        4504     4482      -22     
==========================================
- Hits         3088     3078      -10     
+ Misses       1215     1209       -6     
+ Partials      201      195       -6

Files Changed	Coverage Δ
cmd/debugrouter.go	`0.00% <0.00%> (ø)`
internal/errors/errors.go	`100.00% <ø> (ø)`
internal/events/test_helper.go	`90.90% <ø> (-3.04%)`	⬇️
internal/fabric/client/api.go	`0.00% <ø> (ø)`
internal/fabric/client/client_common.go	`11.90% <0.00%> (ø)`
internal/fabric/client/invokehandler.go	`0.00% <0.00%> (ø)`
internal/fabric/tx.go	`0.00% <0.00%> (ø)`
internal/messages/messages.go	`0.00% <ø> (ø)`
internal/utils/swagger-ui.go	`0.00% <0.00%> (ø)`
internal/utils/utils.go	`59.57% <ø> (ø)`
... and 33 more

nguyer · 2023-09-18T20:50:54Z

The code looks good. You will need to amend your commit history to satisfy the DCO requirement though. The bot has linked to a page that explains how to do that, and which commits need to be updated: https://github.com/hyperledger/firefly-fabconnect/pull/108/checks?check_run_id=16863753560

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

PanGan21 · 2023-09-19T09:31:30Z

The code looks good. You will need to amend your commit history to satisfy the DCO requirement though. The bot has linked to a page that explains how to do that, and which commits need to be updated: https://github.com/hyperledger/firefly-fabconnect/pull/108/checks?check_run_id=16863753560

I think I fixed it but I didn't sign the merge commit because it was double signing older commits. Could you please rerun the pipeline to see if everything is in place?

nguyer

DCO looks good now. Thanks!

PanGan21 requested review from jimthematrix, peterbroadhurst, vdamle and nguyer as code owners March 8, 2023 22:26

PanGan21 mentioned this pull request Mar 15, 2023

Add linter rules #106

Open

PanGan21 marked this pull request as draft March 15, 2023 22:41

PanGan21 added 4 commits March 17, 2023 20:44

Add golangci config file and fix linter issues

95e9e91

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

fix linting issues with naming, ifelse and switch

7211cf5

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

fix more revive errors

5cbc3ba

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

fix revive errors

03725d3

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

PanGan21 force-pushed the 106-add-linter-rules branch from 6040c5f to 03725d3 Compare March 17, 2023 19:44

PanGan21 added 2 commits March 18, 2023 09:40

fix license related issues

d24028d

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

fix gosec issues, ignore false positive and fix unit tests

82eb2e9

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

PanGan21 marked this pull request as ready for review March 20, 2023 18:25

PanGan21 commented Sep 14, 2023

View reviewed changes

nguyer reviewed Sep 15, 2023

View reviewed changes

nguyer suggested changes Sep 15, 2023

View reviewed changes

fix conflicts

3913e3e

PanGan21 commented Sep 16, 2023

View reviewed changes

PanGan21 added 4 commits September 19, 2023 12:28

update dependencies

f07e1f7

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

change back to upper case error message

f100ae4

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

fix linting except from depguard

9a73954

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

add todos for security issues

6ba512e

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

PanGan21 added 2 commits September 19, 2023 12:28

tidy the project deps

5243b2e

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

remove depguard linter

d86d6ba

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

PanGan21 force-pushed the 106-add-linter-rules branch from 6385858 to d86d6ba Compare September 19, 2023 09:28

nguyer approved these changes Sep 19, 2023

View reviewed changes

nguyer merged commit 92a9508 into hyperledger:main Sep 19, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add golangci config file and fix linter issues #108

Add golangci config file and fix linter issues #108

PanGan21 commented Mar 8, 2023 •

edited

Loading

nguyer commented Mar 16, 2023

PanGan21 Mar 20, 2023

PanGan21 Mar 20, 2023

nguyer Sep 15, 2023

PanGan21 Sep 16, 2023

nguyer commented Sep 15, 2023

nguyer Sep 15, 2023

PanGan21 Sep 16, 2023

nguyer Sep 15, 2023

PanGan21 Sep 16, 2023

nguyer Sep 15, 2023

PanGan21 Sep 16, 2023

nguyer left a comment

PanGan21 Sep 16, 2023

PanGan21 Sep 17, 2023

PanGan21 commented Sep 16, 2023

codecov-commenter commented Sep 18, 2023

nguyer commented Sep 18, 2023

PanGan21 commented Sep 19, 2023

nguyer left a comment

Add golangci config file and fix linter issues #108

Add golangci config file and fix linter issues #108

Conversation

PanGan21 commented Mar 8, 2023 • edited Loading

nguyer commented Mar 16, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyer commented Sep 15, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nguyer left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PanGan21 commented Sep 16, 2023

codecov-commenter commented Sep 18, 2023

Codecov Report

nguyer commented Sep 18, 2023

PanGan21 commented Sep 19, 2023

nguyer left a comment

Choose a reason for hiding this comment

PanGan21 commented Mar 8, 2023 •

edited

Loading