batches: always set RemoteRepo
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.