[v3] Improve empty document handling

[felixfontein]

Parts of #684 that haven been fixed by @niemeyer in the meantime.

Thanks, but we need to have all tests either in node_test.go or in the respective test files, using existing conventions. We have plenty of tests and test files already.

So far I found exactly one test for multi-document reading and writing, namely TestEncoderMultipleDocuments in encode_test.go. Where should I put the multi-document decoding and encoding tests with Node?

[felixfontein]

@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?

[niemeyer]

@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.

[felixfontein]

@niemeyer CI is failing with Error: Unable to find Go version '1.16.0-beta1' for platform linux and architecture x64..

[felixfontein]

Restarting CI.

[niemeyer]

@felixfontein Sorry for being slow here. I'll reserve some time soon to do another push on go-yaml.

[felixfontein]

@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...).

[felixfontein]

@niemeyer but thanks for trying to take another look at this :)

[lollipopman]

@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!

[felixfontein]

@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.

[glopal]

@felixfontein @lollipopman Any update on this one? It looks like the build has some failures. Been waiting on this PR for a while now.

[felixfontein]

@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.

[pivotaljohn]

In a delightful display of dominoes, merging and releasing this fix has the effect of making SOPS work seemlessly with ytt which is a growing combination among those plumbing sensitive data straight through processing pipelines (like folks doing automated deploys to Kubernetes). It currently takes a hack.

This is a subtly valuable fix for a (expanding) group I can point at.

Thanks for all your efforts, @felixfontein!! 🙏🏻

[felixfontein]

@niemeyer I rebased since I saw that the broken workflow is gone.

[pieterocp]

One could probably un-comment the two tests in stores/yaml/store_test.go, unless they must only be un-commented post-merge? (Ignore me, different projects, brain-fart)

[pieterocp]

Any contributors willing to re-review this or does it need a rebase? Be nice to get this into 3.1.0 maybe

[felixfontein]

It's already rebased to the current v3 branch. The last change to the v3 branch was two years ago.