Surface log file creation errors in GITSIGN_LOG handling - #853
Closed
pujitha24 wants to merge 1 commit into
Closed
Conversation
Motivation: Issue sigstore#291 reports that when GITSIGN_LOG points to a file gitsign can't create (e.g. a permission error, or a missing parent directory), gitsign silently proceeds with no log file and no diagnostic at all. Since Git consumes gitsign's stdout/stderr, the user just sees a generic "error: gpg failed to sign the data" / "fatal: failed to write commit object" from Git with no clue that GITSIGN_LOG itself failed to be set up. PR sigstore#292 already fixed a related issue (writers being closed too early) but explicitly left this part of sigstore#291 unaddressed. Approach: internal/io/streams.go's Streams.New() previously ignored the error from os.Create(logPath) entirely. This change captures that error and, once the TTY-vs-stderr fallback is resolved, writes a "failed to create log file ..." message to Streams.TTYOut (the real TTY if one is attached, otherwise stderr) using the same mechanism Streams.Wrap already uses to surface other errors. To make this independent of whether a real TTY happens to be attached to the test process, TTY opening is now routed through an overridable openTTY var (matching the existing execFn/rekorUpload test-seam pattern already used elsewhere in this repo). This does not fix commit signing failures themselves, and does not change behavior when the log file is created successfully. The concrete, narrower benefit is that a user with a misconfigured GITSIGN_LOG (bad path/permissions) now gets a visible diagnostic message instead of silent, unexplained absence of the log file. Validation: go build ./... go test ./internal/io/... -v Both pass, including the new TestNew_LogFileCreateError test which verifies the error message is written to stderr when log file creation fails and no TTY is attached. Fixes sigstore#291 Signed-off-by: Pujitha Paladugu <10557236+pujitha24@users.noreply.github.com>
Author
|
This has been sitting for a bit and is rebased and green — happy to make any changes if something would help move review along. |
Author
|
Closing this to keep my open PR queue manageable - it has been open a while without review, and I would rather not leave stale PRs sitting in your queue. The change itself still applies; happy to reopen and rebase if it is useful to you. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Motivation:
Issue #291 reports that when GITSIGN_LOG points to a file gitsign can't
create (e.g. a permission error, or a missing parent directory), gitsign
silently proceeds with no log file and no diagnostic at all. Since Git
consumes gitsign's stdout/stderr, the user just sees a generic
"error: gpg failed to sign the data" / "fatal: failed to write commit
object" from Git with no clue that GITSIGN_LOG itself failed to be set
up. PR #292 already fixed a related issue (writers being closed too
early) but explicitly left this part of #291 unaddressed.
Approach:
internal/io/streams.go's Streams.New() previously ignored the error
from os.Create(logPath) entirely. This change captures that error and,
once the TTY-vs-stderr fallback is resolved, writes a
"failed to create log file ..." message to Streams.TTYOut (the real
TTY if one is attached, otherwise stderr) using the same mechanism
Streams.Wrap already uses to surface other errors. To make this
independent of whether a real TTY happens to be attached to the test
process, TTY opening is now routed through an overridable openTTY var
(matching the existing execFn/rekorUpload test-seam pattern already
used elsewhere in this repo).
This does not fix commit signing failures themselves, and does not
change behavior when the log file is created successfully. The
concrete, narrower benefit is that a user with a misconfigured
GITSIGN_LOG (bad path/permissions) now gets a visible diagnostic
message instead of silent, unexplained absence of the log file.
Validation:
go build ./...
go test ./internal/io/... -v
Both pass, including the new TestNew_LogFileCreateError test which
verifies the error message is written to stderr when log file creation
fails and no TTY is attached.
Fixes #291
Signed-off-by: Pujitha Paladugu 10557236+pujitha24@users.noreply.github.com