release: update_versions: continue on other repos if one repo attempt fails #130063
Conversation
This file contains 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
![blathers[crl]](https://avatars.githubusercontent.com/in/59700?s=40&u=373d9101a12edc3847d2fb6ae78e3adcfb4ca41e&v=4){kind=avatar}
|
It looks like your PR touches production code but doesn't add or edit any test code. Did you consider adding tests to your PR? 🦉 Hoot! I am a Blathers, a bot for CockroachDB. My owner is dev-inf. |
|
This change is |
celiala
force-pushed
the
update-versions.continue-upon-repo-error
branch
from
b8c183b to
5769187
Compare
celiala
changed the title
jlinder
reviewed
Sep 4, 2024
There was a problem hiding this comment.
I think this change is LGTM, but I've a few questions:
- If this fails for one repo, will the job still show as failed? If not, then it's going to hide the fact that this failed for some of the repos.
- If this needs to be rerun, will it be safe for all the repos what had previously run successfully for the given version?
celiala
force-pushed
the
update-versions.continue-upon-repo-error
branch
from
5769187 to
2c8cd59
Compare
celiala
commented
Sep 4, 2024
| @@ -330,6 +326,9 @@ func updateVersions(_ *cobra.Command, _ []string) error { | |||
| if err := sendPrReport(releasedVersion, prs, smtpPassword); err != nil { | |||
| return fmt.Errorf("cannot send email: %w", err) | |||
| } | |||
| if len(workOnRepoErrors) > 0 { | |||
| return errors.Join(workOnRepoErrors...) | |||
| } | |||
There was a problem hiding this comment.
If this fails for one repo, will the job still show as failed? If not, then it's going to hide the fact that this failed for some of the repos.
Ah, good callout: I'd updated updateVersions() to return the workOnRepoErrors if any errors occurred during the workOnRepo for loop.
| } | ||
| } | ||
|
|
||
| // Now that our local changes are staged, we can try and publish them. | ||
| for _, repo := range reposToWorkOn { | ||
| if repo.workOnRepoError != nil { | ||
| continue | ||
| } | ||
| dest := path.Join(globalWorkDir, repo.checkoutDir()) | ||
| // We avoid creating duplicated PRs to allow this command to be | ||
| // run multiple times. |
There was a problem hiding this comment.
If this needs to be rerun, will it be safe for all the repos what had previously run successfully for the given version?
I believe so, based on the comment on line 307 (was 303):
// We avoid creating duplicated PRs to allow this command to be
// run multiple times.
jlinder
reviewed
Sep 4, 2024
pkg/cmd/release/update_versions.go
Outdated
Comment on lines
299
to
302
| if repo.workOnRepoError != nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
One other thought. What about noting in the email that the PR for this repo wasn't created?
In "Create version update PRs" for v24.1.4", the whole TC job fails if "work on repo X" fails for any repo. This PR continues to work on the other repos, should any one repo fail. Release note: None Epic: None Release justification: release-infra only change.
celiala
force-pushed
the
update-versions.continue-upon-repo-error
branch
from
2c8cd59 to
ba971a7
Compare
celiala
commented
Sep 4, 2024
| } | ||
| } | ||
|
|
||
| // Now that our local changes are staged, we can try and publish them. | ||
| for _, repo := range reposToWorkOn { | ||
| if repo.workOnRepoError != nil { | ||
| log.Printf("PR creation skipped due to previous errors while working on %s: %s", repo.name(), repo.workOnRepoError) | ||
| continue |
There was a problem hiding this comment.
One other thought. What about noting in the email that the PR for this repo wasn't created?
Good idea - done! (logging that PR creation was skipped due to previous errors)
|
TFTR! bors r=jlinder |
celiala
added a commit
to celiala/cockroach
that referenced
this pull request
Sep 4, 2024
Follow-up to cockroachdb#130063: after merging 130063, update_versions failed to complete[1], due to encountering another / different error. This commit adjusts update_versions so that it continues & attempts to create PRs for all the remaining repos, despite encountering errors along the way. [1] Latest "Create version update PRs" job: https://teamcity.cockroachdb.com/buildConfiguration/Internal_Cockroach_Release_Publish_CreateVersionUpdatePRs/16752534?buildTab=log&focusLine=3552&logView=flowAware&expandAll=true Release note: None Epic: None Release justification: release-infra only change.
craig bot
pushed a commit
that referenced
this pull request
Sep 4, 2024
129758: parser,mt: remove CREATE TENANT ... LIKE r=dt a=dt The problem -- needing to apply a large number of configuration knobs -- solved by the 'template tenant' functionality has since been solved instead by having named service modes for each distinct common set of configurations, for example 'shared' which implies all capabilities in addition to in-process service. The template functionality was never pushed on PCR UA users, and is not used in cloud, and thus has no users and is accordingly removed here. Release note: none. Epic: none. 130054: rac2: add AdmittedTracker interface for use by RangeController r=pav-kv,kvoli a=sumeerbhola The AdmittedTracker provides the latest AdmittedVector, and will be implemented by processorImpl. Informs #129508 Epic: CRDB-37515 Release note: None 130096: release: update_versions: continue, even if one repo attempt fails r=celiala a=celiala Follow-up to #130063: after merging 130063, update_versions failed to complete[1], due to encountering another / different error. This commit adjusts update_versions so that it continues & attempts to create PRs for all the remaining repos, despite encountering errors along the way. [1] Latest "Create version update PRs" job: https://teamcity.cockroachdb.com/buildConfiguration/Internal_Cockroach_Release_Publish_CreateVersionUpdatePRs/16752534?buildTab=log&focusLine=3552&logView=flowAware&expandAll=true Release note: None Epic: None Release justification: release-infra only change. 130111: execinfrapb: move MockDistSQLServer to flowinfra r=dt a=dt pb packages are intended to be leaf packages with few or no dependencies. The mock server in the test utils was pulling in dependencies on packages that encode business logic, including logic that wants to depend on pb packages such as pkg/rpc's auth logic that depends on request types. Release note: none. Epic: none. Co-authored-by: David Taylor <[email protected]> Co-authored-by: sumeerbhola <[email protected]> Co-authored-by: Celia La <[email protected]>
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.
In "Create version update PRs" for v24.1.4", the whole TC job fails if "work on repo X" fails for any repo.
This PR continues to work on the other repos, should any one repo fail.
Release note: None
Epic: None
Release justification: release-infra only change.