Skip to content
This repository was archived by the owner on Sep 30, 2024. It is now read-only.

fix(ci): check command out for error when git fails#63993

Merged
BolajiOlajide merged 9 commits into
mainfrom
bo/fix-git-errors-not-showing-up-ci
Jul 23, 2024
Merged

fix(ci): check command out for error when git fails#63993
BolajiOlajide merged 9 commits into
mainfrom
bo/fix-git-errors-not-showing-up-ci

Conversation

@BolajiOlajide

@BolajiOlajide BolajiOlajide commented Jul 22, 2024

Copy link
Copy Markdown
Contributor

Closes #1110
Closes DINF-96

We don't print the stdErr when a command fails … in particular when git fails. Therefore we see very little in the panic of what went wrong.

Explanation:

There's a weird behavior that occurs where an error isn't accessible in the err variable
// from a *Cmd executing a git command after calling CombinedOutput().
// This occurs due to how Git handles errors and how the exec package in Go interprets the command's output.
// Git often writes error messages to stderr, but it might still exit with a status code of 0 (indicating success).
// In this case, CombinedOutput() won't return an error, but the error message will be in the out variable.

Test plan

Manual testing

func main() {
	ctx := context.Background()
	cmd := exec.CommandContext(ctx, "git", "rev-parse", "--is-inside-work-tree")
	out, err := handleGitCommandExec(cmd)
	if err != nil {
		// er := errors.Wrap(err, fmt.Sprintf("idsdsd: %s", string(out)))
		panic(err)
	}
	fmt.Println("hello", string(out))
}

Changelog

@BolajiOlajide BolajiOlajide requested a review from a team July 22, 2024 16:00
@BolajiOlajide BolajiOlajide self-assigned this Jul 22, 2024
@cla-bot cla-bot Bot added the cla-signed label Jul 22, 2024
@Strum355

Strum355 commented Jul 23, 2024

Copy link
Copy Markdown
Contributor

Im wondering whether we should do something more generic for this, because this tends to come up anywhere exec.Command is used 😄 imo we should have a generic exec.Command wrapper that includes stderr + the exit code in the returned error always. Thoughts @BolajiOlajide @burmudar ?

@burmudar

burmudar commented Jul 23, 2024

Copy link
Copy Markdown
Contributor

Im wondering whether we should do something more generic for this, because this tends to come up anywhere exec.Command is used 😄 imo we should have a generic exec.Command wrapper that includes stderr + the exit code in the returned error always. Thoughts @BolajiOlajide @burmudar ?

I agree with you @Strum355 - I see a few attempts at this has been made if I look at sg/internal like https://sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/dev/sg/internal/execute/exec.go?L10:6-10:9#tab=references. Then there is also the run package that handles stuff too 🤔.

Maybe a initial wrapper that just does the error stuff and we add a depguard lint check to not allow exec import under sg/internal to promote usage of the new package?

@BolajiOlajide

BolajiOlajide commented Jul 23, 2024

Copy link
Copy Markdown
Contributor Author

Im wondering whether we should do something more generic for this, because this tends to come up anywhere exec.Command is used 😄 imo we should have a generic exec.Command wrapper that includes stderr + the exit code in the returned error always. Thoughts @BolajiOlajide @burmudar ?

I agree with you @Strum355 - I see a few attempts at this has been made if I look at sg/internal like sourcegraph.com/github.com/sourcegraph/sourcegraph/-/blob/dev/sg/internal/execute/exec.go?L10:6-10:9. Then there is also the run package that handles stuff too 🤔.

Maybe a initial wrapper that just does the error stuff and we add a depguard lint check to not allow exec import under sg/internal to promote usage of the new package?

Funny thing is I created the linked package lol, I totally forgot that. I'll switch to using that.
Thanks.

@Strum355

Strum355 commented Jul 23, 2024

Copy link
Copy Markdown
Contributor

note that the linked package doesnt seem to make use of stderr, Id probably just unconditionally include stderr along with exit code in the error returned 😄

@BolajiOlajide

BolajiOlajide commented Jul 23, 2024

Copy link
Copy Markdown
Contributor Author

@Strum355 @burmudar This is ready for another round of review.

I've moved the shared snippet to internal/execute so other consumers can make use of it.

burmudar
burmudar reviewed Jul 23, 2024
View reviewed changes
Comment thread internal/execute/git.go
@BolajiOlajide BolajiOlajide merged commit 0671159 into main Jul 23, 2024
@BolajiOlajide BolajiOlajide deleted the bo/fix-git-errors-not-showing-up-ci branch July 23, 2024 14:56
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@burmudar burmudar burmudar approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@BolajiOlajide BolajiOlajide

Labels

cla-signed

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@BolajiOlajide @Strum355 @burmudar