-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
[v3] Improve empty document handling #690
base: v3
Are you sure you want to change the base?
[v3] Improve empty document handling #690
Conversation
@niemeyer if you have time, could you at least take a quick look at the tests? Where should I put multi-document tests? Is a new file ok for that? |
@felixfontein Thanks for the changes. Node test seems nice. We still need to move those tests in yaml_test.go into their respective files. If you have a look around encode_test.go and decode_test.go you'll probably spot the patterns already in use. |
89c04e8
to
3cebf96
Compare
@niemeyer CI is failing with |
3cebf96
to
35d69a4
Compare
Restarting CI. |
@felixfontein Sorry for being slow here. I'll reserve some time soon to do another push on go-yaml. |
@niemeyer I mainly wanted to re-run tests to see whether the semgrep issue is still around, but now it looks like you (or someone else) has to manually enable the workflow for this PR (GitHub changed policies for GHA to combat folks abusing GHA for crypto mining...). |
@niemeyer but thanks for trying to take another look at this :) |
@felixfontein & @niemeyer this pull request seems to work correctly against the current v3 branch, is it ready to be merged or is more work required? Would love to be able to preserve comments in empty yaml docs! Thanks! |
@lollipopman from my point of view, it is working correctly and can be merged. I'm not that familiar with the code base, though, so maybe I'm missing something. That's why it needs a review. |
@felixfontein @lollipopman Any update on this one? It looks like the build has some failures. Been waiting on this PR for a while now. |
@glopal IIRC the CI errors were unrelated to this PR. From my side this PR is still ready. It still needs review by the maintainer. |
In a delightful display of dominoes, merging and releasing this fix has the effect of making SOPS work seemlessly with This is a subtly valuable fix for a (expanding) group I can point at. Thanks for all your efforts, @felixfontein!! 🙏🏻 |
35d69a4
to
af49682
Compare
@niemeyer I rebased since I saw that the broken workflow is gone. |
af49682
to
21e0769
Compare
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.
One could probably un-comment the two tests in (Ignore me, different projects, brain-fart)stores/yaml/store_test.go
, unless they must only be un-commented post-merge?
Any contributors willing to re-review this or does it need a rebase? Be nice to get this into 3.1.0 maybe |
It's already rebased to the current |
Parts of #684 that haven been fixed by @niemeyer in the meantime.
So far I found exactly one test for multi-document reading and writing, namely
TestEncoderMultipleDocuments
inencode_test.go
. Where should I put the multi-document decoding and encoding tests withNode
?