-
Notifications
You must be signed in to change notification settings - Fork 240
Update to go 1.23 #1680
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
Update to go 1.23 #1680
Conversation
Signed-off-by: Simon Gellis <simongellis@gmail.com>
Signed-off-by: Simon Gellis <simongellis@gmail.com>
Signed-off-by: Simon Gellis <simongellis@gmail.com>
Signed-off-by: Simon Gellis <simongellis@gmail.com>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1680 +/- ##
=======================================
Coverage 99.95% 99.95%
=======================================
Files 339 339
Lines 29782 29782
=======================================
Hits 29770 29770
Misses 8 8
Partials 4 4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
Thanks @SupernaviX we normally try to update all the versions up from the dependencies starting with the common library https://github.com/hyperledger/firefly-common/blob/main/go.mod but recently the Golang version have been quite good keeping backwards compatibility. There is also a CVE that @ssmirr is trying to fix and has propagated through Common, Signer, EVMConnect and will add it to FireFly soon, will wait for that one so we can complete that CVE fix across the board |
|
@EnriqueL8 that CVE is the same one that I ran into here, which was making it impossible to open PRs. I'll match the versions from that PR. |
Signed-off-by: Simon Gellis <simongellis@gmail.com>
|
I think this PR should also update firefly-signer to v1.1.21 for another CVE patch: https://github.com/hyperledger/firefly-signer/releases/tag/v1.1.21 |
Signed-off-by: Simon Gellis <simongellis@gmail.com>
|
@ssmirr I looked at the changelog for firefly-common and firefly-signer, and it didn't look like there were (relevant) breaking changes in either. The metrics/monitoring change in firefly-common doesn't affect firefly-core, because it has its own APIServer independent from the common one. Does that sound right or did I miss something? |
|
Awesome thanks - yes that is correct FF Core has it's own APIServer and doesn't use that part of FF Common but it does use other parts |
EnriqueL8
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks great - one small question
Makefile
Outdated
| $(VGO) install github.com/vektra/mockery/v2@latest | ||
| ${LINT}: | ||
| $(VGO) install github.com/golangci/golangci-lint/cmd/golangci-lint@v1.54.0 | ||
| curl -sSfL https://raw.githubusercontent.com/golangci/golangci-lint/HEAD/install.sh | sh -s -- -b $(shell $(VGO) env GOPATH)/bin v1.64.8 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why this change?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@EnriqueL8 I updated the linter version to one which "officially" supported go 1.23 and 1.24. I made this change when the PR targeted go 1.24, because
- golint can't lint go 1.24 files if it was compiled with go 1.23
- by default, golint is compiled with go 1.23 (even if you're using a 1.24 compiler)
- the golint docs recommended using prebuilt binaries instead of installing from source.
But I just tested and the old install approach works fine with go 1.23, so I'll switch back to that.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep switching back worked without a hitch
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the clarification!
Signed-off-by: Simon Gellis <simongellis@gmail.com>
EnriqueL8
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good!
| @@ -106,10 +110,13 @@ paths: | |||
| a smart contract, this field can include a blockchain specific | |||
| contract identifier. For example an Ethereum contract address, | |||
| or a Fabric chaincode name and channel | |||
| nullable: true | |||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is breaking and it's due to an issue in FF Common , going to revert this
Proposed changes
Update the project's go version to 1.23. Also update the base alpine image to 3.21, and some miscellaneous build dependencies to match.
The motivation is to update a transitive dependency on golang.org/x/crypto to the latest. The current version is breaking the build, because it has a vulnerability due to some unbounded buffer.
Types of changes
Other Information
Y'all should consider setting up dependabot or something.