Security exporter #3
Conversation
There was a problem hiding this comment.
You'll probably need to bump the version too: https://github.com/usertesting/biscuit/blob/master/lib/biscuit/version.rb
lib/biscuit.rb
Outdated
| raise(result.slice(0, 200)) unless $?.success? | ||
| result | ||
| stdout, stderr, status = Open3.capture3("#{__dir__}/../bin/_biscuit #{command}") | ||
| raise(stderr.slice(0, 200)) unless status == 0 |
There was a problem hiding this comment.
A difference (which may be an improvement) - previously we raised with a slice of the stderr output, but you have changed it to a slice of the stdout output. That seems likely to be more correct, but I wanted to make sure you were aware, and had checked out the actual behavior around this.
There was a problem hiding this comment.
Open3.capture3 is a way better tool for this by the way, even without the stubbing benefits :-)
There was a problem hiding this comment.
Would you mind making this into a named exception? (maybe Biscuit::Error)
There was a problem hiding this comment.
@nevinera previously the result holds both stdout and stderr I believe. maybe it should be Biscuit::Error.new("#{stdout} #{stderr}".slice(0, 200)) thoughts?
There was a problem hiding this comment.
never mind, it is stdout only
|
@nevinera good call on version bump. since a new small feature is introduced, I will bump it to |
|
I can't decide whether I should raise error with stderr , or stdout. so I defer that to the user that i put both in the execution error object. |
lib/biscuit/execution_error.rb
Outdated
|
|
||
| module Biscuit | ||
| class ExecutionError < StandardError | ||
| def initialize(stdout, stderr) |
There was a problem hiding this comment.
I'd feel a lot better about this if you changed the stdout parameter to be optional - I've run into a lot of places in the past where it was really irritating that someone had changed the initialization signature of a subclass.
cc59684 to
cf5f00f
Compare
|
I'm late to the party on this one, but I love it! 😁 |
We are using the secrets decrypter across many projects. Moving it here would make sense.
Implementation
I use Open3 so I can stub