-
Notifications
You must be signed in to change notification settings - Fork 444
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
Upgrade to go-msgpack/v2 2.1.1 #292
Conversation
This adds the option to specify what version of the `time.Time` encoding format is desired. The default option is `false`, which preserves compatibility with the current dependency of `hashicorp/go-msgpack 0.5.3` in `go.mod`, but users of this library will probably want to override the the setting. While we're here, test against the latest versions of Go and upgrade the `go.mod` file. I tested that this appears to work with Nomad.
This adds the option to specify what version of the `time.Time` encoding format is desired. The default option is `false`, which preserves compatibility with the current dependency of `hashicorp/go-msgpack 0.5.5` in go.mod, but users of this library will probably want to override the the setting. While we're here, upgrade the go.mod file. I tested that this appears to work with Nomad. This needs to be updated and merged after hashicorp/memberlist#292
And set the `time.Time` option to use the go-msgpack-1.1.5-compatible encoding in all the places, since that is the (now previous) version in `go.mod`. v2 2.1.1 was specifically designe to honor backwards compatibility with 1.1.5 and 0.5.5, and to clean up the code base to be more maintainable. There may performance lost with the 1.1.5 to 2.1.1 migration since the fastpath code was removed, but the increased safety is probably worth it. See [the release notes for go-msgkack 2.1.0](https://github.com/hashicorp/go-msgpack/releases/tag/v2.1.0) for more details. I tested this by running this code, and booting up a cluster with a node also running the prior version of Nomad (before the upgrade). Before I made the changes to set the right `time.Time` option, the previous-version node would throw a bunch of time-decoding errors. After fixing the option, the node came up smoothly, even after changing leadership between them. This relies on - [ ] hashicorp/serf#705 - [ ] hashicorp/raft-boltdb#38 - [ ] hashicorp/raft#577 - [ ] hashicorp/memberlist#292 and maybe - [ ] hashicorp/net-rpc-msgpackrpc#12
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.
Thank you for leading this effort @swenson!
I left few comments but nothing blocking.
.github/workflows/check.yml
Outdated
@@ -15,7 +15,7 @@ jobs: | |||
runs-on: ubuntu-latest | |||
strategy: | |||
matrix: | |||
GO_VERSION: [ "1.16","1.17","1.18" ] | |||
GO_VERSION: [ "1.16","1.17","1.18","1.19","1.20","1.21"" ] |
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.
Shouldn't we only stick with the supported versions of go 🤔 ?
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.
I only added versions. I didn't want to remove anything. :)
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.
IMO we should remove the older versions, as to my knowledge none of our supported products is using a non supported version of go and this leave the impression that we still care about those versions.
(test failures appear to be unrelated to these changes) |
Thanks! |
This adds the option to specify what version of the `time.Time` encoding format is desired. The default option is `false`, which preserves compatibility with the current dependency of `hashicorp/go-msgpack 0.5.5` in go.mod, but users of this library will probably want to override the the setting. While we're here, upgrade the go.mod file. I tested that this appears to work with Nomad. This needs to be updated and merged after hashicorp/memberlist#292
This adds the option to specify what version of the `time.Time` encoding format is desired. The default option is `false`, which preserves compatibility with the current dependency of `hashicorp/go-msgpack 0.5.5` in go.mod, but users of this library will probably want to override the the setting. While we're here, upgrade the go.mod file. I tested that this appears to work with Nomad. This needs to be updated and merged after hashicorp/memberlist#292
This adds the option to specify what version of the
time.Time
encoding format is desired. The default option isfalse
, which preserves compatibility with the current dependency ofhashicorp/go-msgpack 0.5.3
ingo.mod
, but users of this library will probably want to override the the setting.While we're here, test against the latest versions of Go and upgrade the
go.mod
file.I tested that this appears to work with Nomad.