Skip to content
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

Merge upstream changes #1

Merged
merged 0 commits into from
Nov 30, 2022
Merged

Merge upstream changes #1

merged 0 commits into from
Nov 30, 2022

Conversation

erikdubbelboer
Copy link

This branch is 31 commits behind ugorji:master. This pull will merge all upstream changes.

I have tested this with memberlist and all tests passed.

@armon
Copy link
Member

armon commented Mar 17, 2015

This is a very large change and could potentially break things in subtle ways. We purposed forked to avoid these kinds of large changes, especially as there is very little testing upstream for breaking of compatibility. I think we will sit on this for now.

@erikdubbelboer
Copy link
Author

If you're afraid of this why not just use encoding/json or encoding/gob? The structs being encoded are quite small and having one less external dependency plus having the stability of the encoding packages could actually be a good thing.

@ryanuber
Copy link
Member

@erikdubbelboer We still prefer msgpack for a few reasons, mainly its efficiency over the wire. Our unit tests have fairly good coverage, but alone are not enough to guard us against this kind of upstream breakage, as we learned quickly with ugorji#37. The commit we forked at is known stable and performant, which is why we are sticking with it for now.

Longer term, it would be nice to move forward with this and not maintain our own msgpack repo, we just haven't prioritized it yet.

@hashicorp-cla
Copy link

CLA assistant check

Thank you for your submission! We require that all contributors sign our Contributor License Agreement ("CLA") before we can accept the contribution. Read and sign the agreement

Learn more about why HashiCorp requires a CLA and what the CLA includes

Have you signed the CLA already but the status is still pending? Recheck it.

@swenson swenson merged commit 141348d into hashicorp:main Nov 30, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants