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

Switch from ffjson to json-iterator #889

Merged
merged 2 commits into from
May 7, 2021
Merged

Conversation

nalind
Copy link
Member

@nalind nalind commented May 6, 2021

Per #886 (comment), replace our uses of ffjson with json-iterator.

Signed-off-by: Nalin Dahyabhai <[email protected]>
@cevich
Copy link
Member

cevich commented May 6, 2021

Holy crap! Thanks @nalind !

Let me try rebasing my PR onto this to give it some runtime with the newer VM images...

@cevich cevich mentioned this pull request May 6, 2021
@cevich
Copy link
Member

cevich commented May 6, 2021

With F34, I see one thing that appears like the tests simply need some adjusting.. I think that can be addressed in #886 so this LGTM (though I really don't fully understand the complete details).

@nalind
Copy link
Member Author

nalind commented May 6, 2021

Hmm, looks like Go 1.16's path/filepath.Match() is rejecting as syntactically invalid a test case that 1.15 accepted. Will take a closer look.

@rhatdan
Copy link
Member

rhatdan commented May 6, 2021

@nalind Will this cause an upgrade problem?

@rhatdan
Copy link
Member

rhatdan commented May 7, 2021

LGTM Depending on the question.
@vrothberg @saschagrunert @giuseppe PTAL

Copy link
Member

@giuseppe giuseppe left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@rhatdan rhatdan merged commit b64e13a into containers:master May 7, 2021
@nalind nalind deleted the json-iterator branch May 7, 2021 14:02
@nalind
Copy link
Member Author

nalind commented May 7, 2021

@nalind Will this cause an upgrade problem?

I would be surprised. Which logic we call into to encode or decode structures should be an implementation detail that our consumers already don't have to care about outside of vendoring the deps, and the logic that reads and writes encoded data from and to files isn't changing.

@TomSweeneyRedHat
Copy link
Member

Just to be sure, could we spin up a new c/storage and get it merged upstream into Buildah and Podman early next week?

@rhatdan
Copy link
Member

rhatdan commented May 9, 2021

Yup, just waiting for the dependabots to fire up.

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