GreenStage commented 3 years ago

https://github.com/bitmaelum/bitmaelum-suite/issues/94

tried to stay within the issue scope

jaytaph commented 3 years ago

I see a lot of nice cleanups! I will need to check the PR a bit later, and let @acalatrava give a look as well..

jaytaph commented 3 years ago

If you're ready with the PR, just add me or acalatrava as PR reviewers. you probably need to do a go fmt ./... to make the tests pass (we still need to work on getting a better feedback)

GreenStage commented 3 years ago

@jaytaph running go fmt ./... does not change a single file on my computer,

Not sure what to make of the failing tests 🐧

jaytaph commented 3 years ago

@GreenStage can you run "make test" to check if you see any issues there?

jaytaph commented 3 years ago

@GreenStage i see we actually use $(goimports -l .) instead of go fmt.. maybe this will give other results?

GreenStage commented 3 years ago

@jaytaph right now make test works fine on my machine

GreenStage commented 3 years ago

@jaytaph hehe, i dont know. I'm not familiar with goimports :P

jaytaph commented 3 years ago

@GreenStage it's the same thing as go fmt, but does some checks on import files as well.. maybe it finds some issues there, but in that case it should be visible with make test..

i'll try your branch locally on my system.. see if it works

jaytaph commented 3 years ago

@GreenStage

➜  bitmaelum-suite git:(sonar_clean_up) make test
Check goimports
cmd/bm-server/processor/process.go
internal/message/signature.go
Check licenses

seems these 2 files have an issue.. we really should have better error messages in the github builds :/ ah, also found why go fmt .// works.. because the issues are in the imports (just the things that are not formatted with go fmt )

jaytaph commented 3 years ago

diff -u cmd/bm-server/processor/process.go.orig cmd/bm-server/processor/process.go
--- cmd/bm-server/processor/process.go.orig     2021-01-23 22:15:08.473621000 +0100
+++ cmd/bm-server/processor/process.go  2021-01-23 22:15:08.473621000 +0100
@@ -21,6 +21,9 @@

 import (
        "errors"
+       "io/ioutil"
+       "os"
+
        "github.com/bitmaelum/bitmaelum-suite/cmd/bm-server/internal/account"
        "github.com/bitmaelum/bitmaelum-suite/cmd/bm-server/internal/container"
        "github.com/bitmaelum/bitmaelum-suite/internal/api"
@@ -31,8 +34,6 @@
        "github.com/bitmaelum/bitmaelum-suite/pkg/hash"
        "github.com/sirupsen/logrus"
        "golang.org/x/sync/errgroup"
-       "io/ioutil"
-       "os"
 )

 var errValidating = errors.New("error while validating")
diff -u internal/message/signature.go.orig internal/message/signature.go
--- internal/message/signature.go.orig  2021-01-23 22:15:08.543621000 +0100
+++ internal/message/signature.go       2021-01-23 22:15:08.543621000 +0100
@@ -23,6 +23,7 @@
        "crypto/sha256"
        "encoding/base64"
        "encoding/json"
+
        "github.com/bitmaelum/bitmaelum-suite/pkg/hash"

        "github.com/bitmaelum/bitmaelum-suite/internal/config"

codecov[bot] commented 3 years ago

Codecov Report

Merging #171 (4d88e54) into develop (d360b13) will increase coverage by 0.27%. The diff coverage is 47.27%.

@@             Coverage Diff             @@
##           develop     #171      +/-   ##
===========================================
+ Coverage    48.17%   48.44%   +0.27%     
===========================================
  Files          104      104              
  Lines         4079     4081       +2     
===========================================
+ Hits          1965     1977      +12     
+ Misses        1873     1866       -7     
+ Partials       241      238       -3

Flag	Coverage Δ
unittests	`48.44% <47.27%> (+0.27%)`	:arrow_up:

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
cmd/bm-server/processor/process.go	`0.00% <0.00%> (ø)`
internal/container/bitmaelum.go	`0.00% <0.00%> (ø)`
internal/vault/vault.go	`50.68% <0.00%> (+2.63%)`	:arrow_up:
internal/vault/migrate.go	`21.42% <63.63%> (+5.63%)`	:arrow_up:
internal/message/signature.go	`79.41% <86.36%> (+0.62%)`	:arrow_up:
cmd/bm-server/middleware/auth/auth_apikey.go	`100.00% <0.00%> (+6.12%)`	:arrow_up:
cmd/bm-server/middleware/auth/auth_jwt.go	`92.10% <0.00%> (+7.89%)`	:arrow_up:

GreenStage commented 3 years ago

thanks a lot @jaytaph

since the goimport did not cause the make script to halt (at least in my machine), i assumed it ran without problems. My bad!

Maybe changing the makefile to fail fast upon a negative goimport feedback would prevent further confusions

jaytaph commented 3 years ago

thanks a lot @jaytaph

since the goimport did cause the make script to halt (at least in my machine), i assumed it ran without problems. My bad!

Maybe change the makefile to fail fast upon a negative goimport feedback would prevent further confusions

Good idea. I'm so used to know where to look for things when to go wrong. I think it needs to be more verbose and indeed fail fast.

GreenStage commented 3 years ago

By the way, i cant set the reviewer, lack of permissions I believe