Bump go to 1.20 #5027
Bump go to 1.20 #5027
Conversation
Jenkins BuildsClick to see older builds (115)
|
|
Hey @siddarthkay! Thank you for your PR and your great work upgrading basically everything. Just to give my opinion, I'm uneasy about dropping Other two linter checks I'm not too bothered about, they do help keep the code idiomatic though, particularly |
|
|
|
I enabled The key offenders are :
|
|
Regarding G202: SQL string concatenation -> https://securego.io/docs/rules/g201-g202#the-right-way |
|
next up is An easy fix for such cases is defining a local variable for memory references in for loop and use that variable ahead. We could also exclude just which approach do you prefer @Samyoul ? |
There were few ugly bugs due to ignoring this check, mainly trying to modify a temporary variable instead of the original data. I think it is better to keep the check and fix the aliasing using temporary variables or index until we upgrade to 1.22 when this "optimization" is finally fixed |
|
Thanks for weighing in @stefandunca, just to confirm, the changes in this commit -> 3150f35 seem good to you ? |
They seems right if you need a quick fix and don't have time to look if the G202 can be fixed instead of ignoring. I added some comments for minor improvements and to add a follow up tasks for G202. However, please note that Run from tests can be parallelized and the aliasing become a real issue, therefore the G601 being a serious thing. |
|
Thanks @stefandunca : reversing the alias is a good idea, I'll push a commit and fix that. |
siddarthkay
force-pushed
the
bump-go-to-1-20
branch
from
923d124
to
7fcf11b
Compare
|
issue created to track |
siddarthkay
force-pushed
the
bump-go-to-1-20
branch
from
7fcf11b
to
6d28f90
Compare
siddarthkay
force-pushed
the
bump-go-to-1-20
branch
from
6d28f90
to
3b8f754
Compare
siddarthkay
requested review from
richard-ramos,
stefandunca,
cammellos,
Samyoul,
jakubgs,
apentori and
igor-sirotin
siddarthkay
force-pushed
the
bump-go-to-1-20
branch
from
3b8f754
to
ddff9e0
Compare
siddarthkay
force-pushed
the
bump-go-to-1-20
branch
from
ddff9e0
to
caaa3ed
Compare
siddarthkay
force-pushed
the
bump-go-to-1-20
branch
7 times, most recently
from
6dfb180
to
cc57586
Compare
|
@igor-sirotin : don't worry we found a dumb fix for the storenode problem :) |
siddarthkay
force-pushed
the
bump-go-to-1-20
branch
7 times, most recently
from
3255da8
to
7a1af10
Compare
siddarthkay
force-pushed
the
bump-go-to-1-20
branch
from
7a1af10
to
ef96e22
Compare
This commit attempts to upgrade go version to 1.20.12 This commit also removes the following items from lint checks : * `goconst` * `structcheck` * `deadcode` * `golint` * `varcheck` Mobile PR for QA purposes -> status-im/status-mobile#19564
siddarthkay
force-pushed
the
bump-go-to-1-20
branch
from
ef96e22
to
f521530
Compare
## Summary This commit also points to status-go branch where we have upgraded go to 1.20 Related statusgo PR -> status-im/status-go#5027 ### Testing notes Please test everything, specially the store node stuff. #### Platforms - Android - iOS status: ready
status-im/status-go@a39c01d...5e9f961 ## Summary This commit also points to status-go branch where we have upgraded go to 1.20 Related statusgo PR -> status-im/status-go#5027 ### Testing notes Please test everything, specially the store node stuff. #### Platforms - Android - iOS status: ready
## Summary This commit also points to status-go branch where we have upgraded go to 1.20 Related status-go PR -> status-im/status-go#5027 ### Testing notes Please test everything, specially the store node stuff. #### Platforms - Android - iOS status: ready
| @@ -27,6 +28,8 @@ const defaultBackoff = 10 * time.Second | |||
| const graylistBackoff = 3 * time.Minute | |||
| const isAndroidEmulator = runtime.GOOS == "android" && runtime.GOARCH == "amd64" | |||
| const findNearestMailServer = !isAndroidEmulator | |||
| const overrideDNS = runtime.GOOS == "android" | |||
| const bootstrapDNS = "8.8.8.8:53" | |||
There was a problem hiding this comment.
I don't like this. We should at least leave a FIXME comment explaining that this should be changed.
It's a bard practice because:
- It's bad in terms of preserving privacy of our users.
- It doesn't respect user DNS settings on device or in their local network.
- It unreliable since it depends on only on server.
There was a problem hiding this comment.
Ah sorry I missed adding the FIXME.. there is an issue open for this DNS entry, we also do the same thing for waku nodes in mobile config -> #3024
Summary
This PR attempts to upgrade go version to 1.20
This PR also removes the following items from lint checks :
goconststructcheckdeadcodegolintvarcheckMobile PR for QA purposes -> status-im/status-mobile#19564
Todo