Allow configuration of indentation for YAML and JSON stores

[Ph0tonic]

This PR aims to finish the work started in #930, thanks to @jamesgoodhouse.

I added the use of the Indent param in the store as well as a new --indent option in the cli.
Let me know what you think about it.

Fixes #900, fixes #1048 and fixes #1028

[hiddeco]

In favor of adding support for this but not before 3.9.0.

[felixfontein]

Thanks for your contribution! I've some first comments after glancing over the implementation.

[felixfontein]

For some reason this doesn't compile in CI anymore. The only error printed is Error: , which is not exactly helpful. I'm not sure whether this short error output is caused by GHA or by Go itself. In another PR, #1274, the compile steps worked, but the functional tests fail, also with Error: , so this might be related to GHA. Did you try to compile this locally?

[felixfontein]

Meh, actually the problem was on my side - apparently GHA changed some code and the error reports are now loaded from another domain name, which I was blocking by default.

[Ph0tonic]

Meh, actually the problem was on my side - apparently GHA changed some code and the error reports are now loaded from another domain name, which I was blocking by default.

I found an issue in main.go which I fixed. 👍

[Ph0tonic]

I looked at other opened issues and identified #1048 which we could also fix in this PR.
Here is the doc for the json formatter:

• https://pkg.go.dev/encoding/json#Encoder.SetIndent

The issue asks for the choice of indent character but for the first version we could already use the added cli option --indent for json. What do you think?

[felixfontein]

How about using tabs if no indent is specified, and the amount of spaces if --indent is specified (or the JSON indent value is configured in the config)?

It might also make sense to allow 0 as a valid value for JSON. (IIRC parts of the code assume that not specified == 0.)

[Ph0tonic]

How about using tabs if no indent is specified, and the amount of spaces if --indent is specified (or the JSON indent value is configured in the config)?

It might also make sense to allow 0 as a valid value for JSON. (IIRC parts of the code assume that not specified == 0.)

Yes, we could do that, I think it would make sense.
I just see one technical problem if we want to allow 0, I do not think it's possible to differentiate whether zero was provided during the unmarshaling of .sops.yaml or if it was empty and got the default 0 value.

[Ph0tonic]

I applied what we discussed. However, while looking at other issues, #1028 seems to propose having default json indentation with spaces instead of tab to be compatible with yaml parser. I have no idea whether it is a good idea to change the default indentation of json to 4 spaces instead of one tab.
What do you think ?

[felixfontein]

However, while looking at other issues, #1028 seems to propose having default json indentation with spaces instead of tab to be compatible with yaml parser. I have no idea whether it is a good idea to change the default indentation of json to 4 spaces instead of one tab.

I tend a bit to keep the tabs. No idea what the other maintainers think though :)

In any case, as long as it is possible to explicitly configure indentation by tabs (for example by setting indentation to a negative number) I'm ok with the default changing, assuming there is a majority for changing it.

I just see one technical problem if we want to allow 0, I do not think it's possible to differentiate whether zero was provided during the unmarshaling of .sops.yaml or if it was empty and got the default 0 value.

I would expect it is possible to provide a default value. I'm not that familiar with Go's serialization/deserialization mechanism though...

[felixfontein]

I think you can simply assign default values to conf in

sops/config/config.go

Lines 252 to 253 in d46b702

	conf := &configFile{}
	err = conf.load(confBytes)

before loading, like

	conf := &configFile{}
        conf.Stores.JSON.Indent = -1
	err = conf.load(confBytes)

[Ph0tonic]

Great thank you for your feedback, I appreciate it 👍. I implemented a default value of -1 which allows to support no indentation at all if the user provides 0. I think we can keep the tab as the default and then users can configure whatever they prefer. Let's see what other maintainers think of it.

[Ph0tonic]

Note that commit 07c06c7 contains some merge artefacts that are removed again in commit c7b04e1. Would you mind rebasing again to clean up both commits so that the merge artefacts are not part of the commits added to the main branch?

Besides that, I think this PR is ready for merging. @hiddeco @devstein do you have further comments?

Fixed ! 👍

[felixfontein]

While looking at this again I have some more comments (or maybe I forgot some context from before).

[felixfontein]

Besides that, looks good to me!

[felixfontein]

If nobody objects, I'll merge this during the next few days.

[felixfontein]

@jamesgoodhouse @Ph0tonic thanks a lot for working on this and getting it over the finish line! :)

[consideRatio]

Wieee happy to learn that the json files are now being valid YAML files by no longer including tabs! Nice work on this @Ph0tonic @jamesgoodhouse and reviewers! 🎉

[felixfontein]

To be precise by default they still use tabs, but now you can configure sops to use spaces instead of tabs :)