Consolidate Flatten/Unflatten pre/post processing

[lancerushing]

Fixes #1353

Consolidate the common json marshalling and \n handing to stores/flatten.go
Make existing functions unexported.
Add casts for MACOnlyEncrypted.

When I was investigating #1353 I noticed code duplication in dotenv/store.go and ini/store.go. It seemed cleaner to consolidate the the code.

[lancerushing]

If you merge this one, do not merge #1355. Just one or the other.

[felixfontein]

Thanks for your contribution! I like the general idea, though I would avoid breaking backwards compatibility (Flatten/Unflatten signatures) and would make it more modular. What do you (and also others) think?

[hiddeco]

I am in favor of this solution, but would prefer it not breaking signatures of existing functions.

[lancerushing]

@felixfontein I'll make the suggested changes. Should take me about an hour.

(phantom edit, never mind the second sentence)

@hiddeco Do we still want to leave the functions exported (capital function name)? There are no callers outside of the package.

[lancerushing]

@felixfontein / @hiddeco The suggested changes are in.

• I left Flatten and Unflatten as they were originally.
• The new functions are now FlattenMetadata and UnflattenMetadata
• Added dedicated functions for encodeNewLines and decodeNewLines
• Added dedicated functions for encodeNonStrings and decodeNonStrings

Let me know if there you need any additional changes.

[lancerushing]

🤔 Thinking more about the Metadata encoding for the ini and dotenv formats. It's an interesting goal to encode the Metadata into the target format. There are limitations. Notice how the dotenv store checking for isComplexValue.

🤔 🤔 I wonder if it might be advantageous to encode it as one line of json? or even json base64 encoded? It would eliminate the \n and non-string handling. But it would make the resulting file less "readable". meh.

I'm not advocating the json (w/ or w/o base64). Just thinking of alternatives that would be more robust. Just something to think about for the next major version.

Anyway, thank you for a great tool. I'm making a lot of use it in container images.

[lancerushing]

@felixfontein / @hiddeco Do I need to keep this branch up to date with main? (I did not see any recommendations in CONTRIBUTING.md)

[felixfontein]

@lancerushing I think that's only necessary in case of conflicts; we can simply rebase before merging with the UI (at least as long as there are no conflicts and rebase just runs through).

[felixfontein]

🤔 Thinking more about the Metadata encoding for the ini and dotenv formats. It's an interesting goal to encode the Metadata into the target format. There are limitations. Notice how the dotenv store checking for isComplexValue.

🤔 🤔 I wonder if it might be advantageous to encode it as one line of json? or even json base64 encoded? It would eliminate the \n and non-string handling. But it would make the resulting file less "readable". meh.

One addition to "less readable" is that diffs are no longer useful. Right now if some metadata changes (for example you add a new key, or remove one), you can easily see what happens from the diff. That wouldn't be possible anymore.

Also changing the way how metadata is stored would break backwards compatibility, so for some time we would have to support both the old and new way how metadata is stored.

Having generic code for such stores that allow to flatten/unflatten metadata and do other operations (like handling formats where every value is a string) is probably the best way forward.

[lancerushing]

Thanks for the all the discussion. Do we have an ETA when we this could be merged? Thank you again.

[felixfontein]

I would mention 'metadata' in these comments. Besides that, LGTM!

[hiddeco]

Besides the lack of doc blocks, I am happy with this implementation.

[lancerushing]

Anything I need to do to get the PR merged?

[hiddeco]

@lancerushing if you could add doc blocks to the functions which do not have one, then this should be good to go.

[lancerushing]

@lancerushing if you could add doc blocks to the functions which do not have one, then this should be good to go.

Done. Added comments for the new functions that were missing comments. -Thank you, Lance

[felixfontein]

@lancerushing thank you very much for fixing this!
@hiddeco thanks for reviewing!

[lancerushing]

🎉 Thank you for the merge and a great project! Let me know if I can help with anything else.