roachprod: minor fixes for snapshots #104573
Conversation
Support for disk snapshots has been recently added in [1]. This change adds minor fixes which were necessary to run in a different environment which was previously tested. Specifically, several commands were missing `--project` which resulted in (silent) failures. Error propagation from `c.Parallel` was signaling an internal error instead of passing command's error. (Granted, the existing mechanism for error propagation is to be revisited in [2].) [1] cockroachdb#103757 [2] cockroachdb#104312 Epic: none Release note: None
srosenberg
requested review from
herkolategan,
renatolabs,
irfansharif and
a team
and removed request for
a team
|
This change is |
srosenberg
requested review from
smg260
and removed request for
renatolabs and
a team
There was a problem hiding this comment.
The naming validation/trimming LGTM, but the error handling, I was cargo culting from the other uses of c.Parallel. I remember running into more opaque-looking errors when I didn't have it, but I'd have to try it again to be more specific. Perhaps that's what you're alluding to in #104312 but I'm too far from the code to fully understand it. All that's to say I'll defer the real LGTM to wiser folks.
Yep. That's exactly why we want to revisit this (new) API. It has confounded many including the original author :) |
|
TFTR! bors r=irfansharif,herkolategan |
|
This PR was included in a batch that successfully built, but then failed to merge into master. It will not be retried. Additional information: {"message":"Changes must be made through a pull request.","documentation_url":"https://docs.github.com/articles/about-protected-branches"} |
|
bors r+ single on |
|
Build succeeded: |
Support for disk snapshots has been recently added in [1]. This change adds minor fixes which were necessary to run in a different environment which was previously tested. Specifically, several commands were missing
--projectwhich resulted in (silent) failures. Error propagation fromc.Parallelwas signaling an internal errorinstead of passing command's error. (Granted, the existing mechanism for error propagation is to be revisited in [2].)
[1] #103757
[2] #104312
Epic: none
Release note: None