Skip to content

batches: always set RemoteRepo

Warren Gifford requested to merge aharvey/bitbucket-cloud-executor-fix into main

Created by: LawnGnome

This fixes #37585 (closed), which was caused by a combination of the Bitbucket Cloud source assuming that RemoteRepo would always be non-nil, and executor.updateChangeset no longer enforcing that invariant. This was compounded by the fake changeset source not checking if RemoteRepo was set when TargetRepo was set, which meant that we didn't have test coverage of this specific problem.

While the simpler fix here would have been to simply move constructing the sources.Changeset down a few lines in updateChangeset, I've chosen to refactor this into what I think is a safer pattern. The computed, memoised fields on executor were a bit of a mess: the changesetSource method implicitly also set the remoteRepo field, which was obviously surprising, and wasn't documented in the code.

Mea culpa.

Instead, remoteRepo access is now hidden behind a separate method, similar to changesetSource. This method implicitly invokes changesetSource, but since that's memoised, practically this shouldn't add any real overhead.

decorateChangesetBody has also been updated to decouple it further from the implementation detail of constructing a sources.Changeset: instead, it now takes btypes.Changeset and the spec body, and returns a string rather than modifying a field in place.

Finally, calculating the remote repo has moved to sources — which is probably where it should have been all along — so it can be reused by the bulk processor, which should also be setting the remote repo as a best practice. (Right now, no operations are supported that actually require the remote repo to be correct, but we're now going to enforce that all ChangesetSource methods expect RemoteRepo to be set, since that's what the test updates require.)

Test plan

This has been tested manually (indeed, this was originally found while QA-ing work on a different project), but more importantly, the missing test coverage that allowed this bug to sneak in has been identified and rectified.

Merge request reports

Loading