Skip to content
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

roachtest: return error in StopCockroachGracefullyOnNode #96689

Merged
merged 1 commit into from
Feb 7, 2023

Conversation

renatolabs
Copy link
Contributor

That function misleadingly returned an (always nil) error, calling t.Fatal() functions in it. The calls to Stop have been replaced with calls to StopE.

Epic: none

Release note: None

That function misleadingly returned an (always nil) error, calling
`t.Fatal()` functions in it. The calls to `Stop` have been replaced
with calls to `StopE`.

Epic: none

Release note: None
@renatolabs renatolabs requested a review from a team as a code owner February 6, 2023 21:40
@renatolabs renatolabs requested review from herkolategan and smg260 and removed request for a team February 6, 2023 21:40
@cockroach-teamcity
Copy link
Member

This change is Reviewable

Copy link
Collaborator

@herkolategan herkolategan left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Reviewed 1 of 1 files at r1, all commit messages.
Reviewable status: :shipit: complete! 0 of 0 LGTMs obtained (waiting on @smg260)

@srosenberg srosenberg self-requested a review February 7, 2023 01:58
Copy link
Member

@srosenberg srosenberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice catch! Curious if this caused a problem in a recent roachtest failure or just a proactive clean up?

@renatolabs
Copy link
Contributor Author

Curious if this caused a problem in a recent roachtest failure or just a proactive clean up?

I noticed this while working on mixed-versions tests. A lot of the API there prefers functions that return errors instead of calling t.Fatal directly (so that we can add context to the error that only exists at higher levels in the stack); so this function stood out because, if it fails in a mixed-version test, the error reported won't include the mixed-version error context.

TFTRs!

bors r=herkolategan,srosenberg

@craig
Copy link
Contributor

craig bot commented Feb 7, 2023

Build succeeded:

@craig craig bot merged commit f640acb into cockroachdb:master Feb 7, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants