Upgrade to Golang 1.23 #8452
Upgrade to Golang 1.23 #8452
Conversation
E2E Test Results - DynamoDB Local - Local Block Adapter |
E2E Test Results - Quickstart |
N-o-Z
added
the
include-changelog
There was a problem hiding this comment.
Thanks Im very happy you took the initiative to upgrade!
I left some questions.
Got couple more here:
- A "meta" question I also have is are all those changes nesscearly because of the upgrade?
- Will fluffy repo (everest, fluffy, replication) be able to use the packages here? it's go version
1.22. If the answer is NO then it's a risk to upgrade here and leave fluffy as is.
pkg/ident/ident.go
Outdated
| @@ -46,36 +46,36 @@ func marshalType(h hash.Hash, addressType AddressType) { | |||
|
|
|||
| func MarshalBytes(h hash.Hash, v []byte) { | |||
| marshalType(h, AddressTypeBytes) | |||
| MarshalInt64(h, int64(len(v))) | |||
There was a problem hiding this comment.
Commenting here but it's about all the int -> uint changes in ident.go.
I don't see any test coverage for this pkg, and don't feel comfortable with this change - Also I'm not familiar with this code.
Why are you changing this in the same PR?
There was a problem hiding this comment.
I wanted to fix the overflow warning and make the code clearer. However I had to keep the nolint comment so reverting
|
There was a problem hiding this comment.
@N-o-Z Thanks for addressing the comments, almost all resolves just this one thing!
Since fluffy is using a fixed lakeFS version this does not affect it immediately. The next step however is to upgrade the version in fluffy (and all other repositories for that matter)
I know that fluffy version is fixed, but I don't want to be blocked from upgrading fluffy and all of it's services because of go version diff.
To be clear, you're saying that it's literally your next step? There's an issue to track? because it has to be coupled.
|
@Isan-Rivkin I never said that it is blocking. I said that we need to upgrade the go version for all our repos. |
@N-o-Z Following that same argument - if we can't patch a security issue in fluffy repo we're in a problem. |
|
@Isan-Rivkin if you feel that way nobody is stopping you from prioritizing this task and upgrading fluffy s go version once this is in... |
It doesn't make sense to now block both cloud and fluffy from upgrading, so I'm not sure how to respond to this because the way I see this it should be prioritized together or not at all (With the exception of urgent matters if there is one). |
Closes #8451