Skip to content

Add a ready mechanism to the conf pkg

Warren Gifford requested to merge main-dry-run/mvjh/fix-conf into main

Created by: jhchabran

Fixes https://github.com/sourcegraph/sourcegraph/issues/29222

Originally, the issue mentions fixing the call sites to avoid having conf function calls in init packages, but this is a tentacular set of changes and introduce complexity, as all exported function from a package relying on conf package must be able to register the Watch hook to react on configuration changes.

So with @vrto, we took a different route: it's semantically perfectly correct to register a conf.Watch in an init function of a given package.

What is incorrect (and causes trouble) is that the conf.Watch function will immediately fire an HTTP request to poll the config, regardless of where the application is in its initialization process. This is what led to the problem described in the original issue.

Our proposed solution is to instead delay the configuration polling until the service is starting, ie the main function. To achieve this, we have updated the conf.Init function to be the one starting the continuously polling goroutine, if and only if we are in client mode.

The resulting changes are very minimal and solve the original problem of unexpectedly firing HTTP requests by simply importing a package, (sg being a recurring place where this happened).

It also makes the configuration pulling more logical to comprehend until we get the chance of refactoring this package entirely as it's rather confusing in its present form.

The tests themselves are not impacted in any way. We still need the empty mechanism for those, so the test workaround has been left in place.

Test plan

  • CI runs
  • Manual QA in local:
    1. sg start oss
    2. Added a print statement in a conf.Watch in gitserver
    3. Observed that the app is reloaded by sg because it changed
    4. Observed the print statement output in the logs
    5. Opened a browser, updated the site config
    6. Observed the print statement being displayed again
      • This shows that the config package is still able to pull changes as intended even with the delayed start.
  • Check for every application and make sure that conf.Init is being called during application start.

Merge request reports