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

Update to gopkg.in/yaml.v3 #556

Merged
merged 1 commit into from
Jun 6, 2022
Merged

Update to gopkg.in/yaml.v3 #556

merged 1 commit into from
Jun 6, 2022

Conversation

sks
Copy link
Contributor

@sks sks commented Jun 6, 2022

@blgm
Copy link
Collaborator

blgm commented Jun 6, 2022

Thanks for this contribution @sks

@blgm blgm merged commit f5a83b1 into onsi:master Jun 6, 2022
@sks sks deleted the feature/ugprade-to-yaml.v3 branch June 6, 2022 11:14
@thediveo
Copy link
Collaborator

thediveo commented Jun 6, 2022

The CVE was totally messed up and basically declared everything up to but not including the final v3.0.0 to be vulnerable, while its text stated something else. The GH-CVE has been fixed. Updating to v3 isn't a good idea as the behavior and API has changed, not least in some cases map keys are now string instead of interface{}. Any upgrades needs careful screening and testing. I know as I've been bitten.

@blgm
Copy link
Collaborator

blgm commented Jun 6, 2022

Thanks for the warning @thediveo. Now that I think about it, I've fallen over the interface{} -> string key issue before too.

In this case, the YAML parser is used only in the YAML matcher. It's used to decode an Expected and Actual YAML. Because the same parser is used for both the Expected and Actual, I think we should be ok in this case. I understand your argument for not upgrading, and I did consider reverting this PR. But on reflection, we will have to take the leap one day, and all the tests of the YAML matcher are passing. So personally I'm in favour of taking the risk. But I'll defer to @onsi who may want to weigh in.

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.

3 participants