Skip to content

add boolean flag to run repo update in the background or not in Syncer.SyncRepo

Administrator requested to merge mlw/remove-SyncRepo-goroutine into main

Created by: mollylogue

Background behind this change: In working to test https://github.com/sourcegraph/sourcegraph/pull/28522 I realized that the asynchronous nature of syncer.SyncRepo made testing unnecessarily difficult. It would be easier for this function to have synchronous behavior and then if we need the update to happen in the background, add the goroutine at a higher level for ease of testing these pieces. So, I'm proposing removing the goroutine from this function entirely. Below is my reasoning for why we can safely do that.

Currently, this goroutine is never being touched. The only place in the code that SyncRepo is being called is the repo-lookup endpoint in repo-updater query to validate this

If we look where the RepoLookup function in the repo updater client (which corresponds to this endpoint) is called, we see there are 3 cases

Cases:

  1. repos.Add since this is for adding new repos, repo will always be nil and SyncRepo will exit before the goroutine.
  2. set_user_public_repos.go but if you look on line 102, you see that if the repo already exists we return, so we again have a nil repo and exit before the goroutine in SyncRepo
  3. dependency_indexing_scheduler.go This is the only one I'm not sure about, since we could potentially be updating existing repos. However, I still think that in this case the functionality should be synchronous as opposed to asynchronous but I'd like to validate that with someone who knows this part of the code already.

UPDATE: Adding in a boolean flag to do the sync (for non-nil repos) in the background or not and setting it to true so this shouldn't change any functionality. Then when I call this function in https://github.com/sourcegraph/sourcegraph/pull/28522 I can set that boolean to false

Merge request reports

Loading