Fix #369: Implement TOML format support#533
Fix #369: Implement TOML format support#533VojtechVitek wants to merge 4 commits intogetsops:developfrom
Conversation
github.com/BurntSushi/toml knows int64 only for integers
Codecov Report
@@ Coverage Diff @@
## develop #533 +/- ##
===========================================
- Coverage 36.46% 36.32% -0.14%
===========================================
Files 20 20
Lines 2863 2874 +11
===========================================
Hits 1044 1044
- Misses 1725 1736 +11
Partials 94 94
Continue to review full report at Codecov.
|
autrilla
left a comment
There was a problem hiding this comment.
Looks pretty good to me! Just a few minor nits.
I'd like to see some tests before merging too, ideally both unit and functional. Should be able to pretty much copy the ones from other stores.
| // TOML reader that preserves original order via toml.Metadata.Keys(), | ||
| // see https://godoc.org/github.com/BurntSushi/toml#MetaData.Keys | ||
| // and converts the unoredered data (multi-level tree of | ||
| // map[string]interface{}) into sops.BranchTree. |
There was a problem hiding this comment.
| // map[string]interface{}) into sops.BranchTree. | |
| // map[string]interface{}) into sops.TreeBranch. |
| return printTreeItem(newIndenter(w, ""), item) | ||
| } | ||
|
|
||
| // TOML indenter. |
There was a problem hiding this comment.
Documenting why this is needed would be nice.
(not sure it's actually needed -- if it's just for better styled output, mentioning it would be good too)
There was a problem hiding this comment.
Cool, will do. It all comes down to limitations of the TOML libraries, I'll explain further in the comment.
| metadataHolder := stores.SopsFile{} | ||
| err := toml.Unmarshal(in, &metadataHolder) | ||
| if err != nil { | ||
| return sops.Tree{}, fmt.Errorf("Error unmarshalling metadata: %s", err) |
There was a problem hiding this comment.
As a general rule, error messages should start with a lower case letter. Can you please fix this for all your errors?
| case []interface{}: | ||
| return nil, fmt.Errorf("Error extracting array of %v items. Please, access an individual item.", len(v)) | ||
| default: | ||
| if err := printTreeBranch(&b, sops.TreeBranch{sops.TreeItem{Key: "_delete", Value: v}}); err != nil { |
There was a problem hiding this comment.
Not sure how I feel about this. At the very least I'd add a comment describing why this is done. I imagine the alternative is harder, but people are going to wonder why this happens when reading the code.
There was a problem hiding this comment.
TOML library lets you encrypt structs and maps only, ie. a key is required for each value. Marshal() would fail on plain values.
This is actually a limitation of the TOML language specs. Let me document this better.
|
What's left to be done on this PR? Any help needed to merge it? |
|
Sorry, I lost context and moved on. However, this PR was working. I'm busy with other work now. If you want me to finish this PR, please PM me at |
Mainly tests, but there are also some outstanding review comments. |

Support TOML format via https://github.com/BurntSushi/toml (see comparison of pkgs).
Fixes #369.
example.toml:
$ sops -e example.com