-
Notifications
You must be signed in to change notification settings - Fork 1k
Fix #369: Implement TOML format support #533
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| // 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As a general rule, error messages should start with a lower case letter. Can you please fix this for all your errors?
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.
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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