Allow using sigstore keys with empty passphrases #2720
Conversation
mtrmac
added
the
kind/bug
mtrmac
force-pushed
the
empty-sigstore
branch
from
18ebbd3 to
332cd11
Compare
| opts := fakeSharedCopyOptions(t, c.options) | ||
| res, cleanup, err := opts.copyOptions(&someStdout) | ||
| require.NoError(t, err) | ||
| defer cleanup() |
There was a problem hiding this comment.
LGTM
nit: defer cleanup() will run only after the loop finishes, is it possible to run the cleanup() at the end of each loop to avoid resources leaking or exhaustion?
Or is this anything that it needs to be kept there for re usage ?
There was a problem hiding this comment.
You’re right that the cleanup could be done earlier, and that it would be conceptually cleaner.
I don’t think it matters much either way here; this is a short-running test, and the loop iterations are all listed just above (5–6 of them).
The cost would be adding another func(){…}() nesting level — a very minor downside in readability.
mtrmac
force-pushed
the
empty-sigstore
branch
from
332cd11 to
e669ef5
Compare
|
LGTM |
|
@aguidirh PTAL. If you give this a LGTM, we'll move it along. |
|
@aguidirh "LGTM" works just fine, TYVM! |
|
/approve |
|
/lgtm |
mtrmac
force-pushed
the
empty-sigstore
branch
from
e669ef5 to
434518a
Compare
|
There is no merge bot here… just an (out-of-sync) GitHub list of user accounts allowed to click on the merge button. |
Should not change (test) behavior. Signed-off-by: Miloslav Trmač <[email protected]>
Signed-off-by: Miloslav Trmač <[email protected]>
mtrmac
force-pushed
the
empty-sigstore
branch
from
434518a to
e25230f
Compare
3 participants
Fixes #2712 . See individual commit messages for details.