fix for #548 - handle .ini files in decrypt.Data, add other helper
#549
Conversation
|
retest |
dnozay
force-pushed
the
pr-548
branch
2 times, most recently
from
fd5c42d
to
392657f
Compare
|
looks like sometimes tests are failing with |
dnozay
changed the title
decrypt.Data, add other helper
|
@autrilla ^ WDYT? |
|
@dnozay Could you change this PR to be merged against the |
dnozay
force-pushed
the
pr-548
branch
2 times, most recently
from
321b504
to
5c43cf0
Compare
|
@ajvb i changed the merge base to |
There was a problem hiding this comment.
Why should we have two separate implementations for "format" and for "path"? Correct me if I'm wrong, but it seems to me like we have a mapping of file extensions to stores, and then a default of BinaryStore. We could implement this in a single function, and even better, define a map[string]*Store for the extensions.
I can see the reason being that passing an "extension" instead of a file type to sops.Data is a bit weird, but if that's the reason, I'd make the file types an enum-like thing, so they're statically typed and not strings, and then write a PathToFormat function or something like that for conversion of file extensions to file types.
decrypt/decrypt.go
Outdated
| default: | ||
| store = &sopsjson.BinaryStore{} | ||
| } | ||
| func DataWithStore(data []byte, store sops.Store) (cleartext []byte, err error) { |
There was a problem hiding this comment.
Do you have any use case for which this should be public?
There was a problem hiding this comment.
Choosing the store is the contentious part. There are several strategies:
- based on a format
- based on an extension
- based on whatever custom logic
Example: you can usego-bindatato include assets.
if decrypt has the only stable api, then basically that says "trust me i know what i am doing", let me pass in the correct store.
There was a problem hiding this comment.
Do you think DataWithFormat would be a more useful API? Considering Store is not really meant to be used by third parties. And I think we should re-export Format in this file as well. Keep in mind only the decrypt code should be public to external projects, and right now we're requiring them to access go.mozilla.org/sops.
Codecov Report
@@ Coverage Diff @@
## develop #549 +/- ##
===========================================
+ Coverage 36.46% 36.54% +0.07%
===========================================
Files 20 20
Lines 2863 2857 -6
===========================================
Hits 1044 1044
+ Misses 1725 1719 -6
Partials 94 94
Continue to review full report at Codecov.
|
|
I don't have a strong opinion with
|
cmd/sops/common/common.go
Outdated
| @@ -139,19 +170,40 @@ func IsIniFile(path string) bool { | |||
| return strings.HasSuffix(path, ".ini") | |||
| } | |||
|
|
|||
| // DefaultStoreForFormat returns the correct format-specific implementation | |||
| // of the Store interface given the format string | |||
| func DefaultStoreForFormat(format string) Store { | |||
There was a problem hiding this comment.
With my suggested changes, this should not be needed anymore.
There was a problem hiding this comment.
Either StoreConstructors needs to be public or DefaultStoreForFormat needs to be public, so the CLI can consume it.
There was a problem hiding this comment.
Yeah, you're right. I think I'd expose Format and the associated way to get a Store from a given format in the go.mozilla.org/sops package directly (so at the root of the repository) since it isn't command-line specific. It's not super important since it's not part of the public API, though, so feel free to just keep it here.
|
By the way, sorry this initially simple PR has turned into a somewhat major refactor of how we handle formats :(. And thanks a lot for all your work and patience! |
|
@dnozay looks like you forgot to commit the formats package though |
|
@autrilla - sorry about that; it is there now. |
|
No worries! All good from my end, I'll merge now, and if @ajvb has any concerns he can raise them later and we can revert. |
No description provided.