-
Notifications
You must be signed in to change notification settings - Fork 1k
Allow read data from stdin on Windows #1104
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
|
In case tests are missing, please give me a hint where should I add the tests, thanks. |
mulder999
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.
Seems mostly OK for me. A few improvements needed now that error message is managed centrally.
|
How to move forward here? |
|
@ajvb (chosen as an active maintainer) could you take a look here? |
|
@ajvb any change of taking a look and merging? This is a nice fix that resolves having to use a custom build of SOPS on Windows |
|
Changing merge target to develop |
|
Reading #927, its like it will take some time. |
| } | ||
| } | ||
|
|
||
| func ReadFile(path string) (content []byte, err error) { |
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.
I think this tries to do two things which really should be separate things.
In my opinion, handling of files should really be just handling of files, with os.Stdin not classifying as such. Part because from the perspective of SOPS as an SDK, decrypt.Decrypt magically reading from os.Stdin would be kind of surprising.
Having said this, I would deal with - as a special path case within commands, and e.g. create another wrapper around DataWithFormat for an io.Reader.
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.
files should really be just handling of files
I don't know. On Unix, stdin is just an other file descriptor and could be handle as file, too. (like /dev/stdin)
Living the unix philosophy, there should not be a difference between /mnt/file and /proc/self/fd/1.
From technical point of view, it may should fine.
Creating an other wrapper, it possible. But the other wrapper would be the exact same copy/paste 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.
How is this the same copy/paste code?
func Reader(r io.Reader, format string) (cleartext []byte, err error) {
encryptedData, err := io.ReadAll(r)
if err != nil {
return nil, err
}
return DataWithFormat(encryptedData, FormatFromString(format))
}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.
NB: I am fine with a function called ReadContent (or something something) existing (and utilized) within cmd/sops, but I wouldn't want this to be the default for decrypt.
Fixes #739
Allow
-as cross platform alternative to/dev/stdinfor reading data from STDIN.