Fix usage of the Fatal call

The Fatal function calls os.Exit(1) internally. It leads to
immediate process termination and no deferred funcs are called.
It is replaced with other option that better applies in the
context.

6 files changed, 40 insertions, 32 deletions

diff --git a/cmd/gitaly/main.go b/cmd/gitaly/main.go
index 0a61756cb..cdd2d9785 100644
--- a/cmd/gitaly/main.go
+++ b/cmd/gitaly/main.go
@@ -73,23 +73,20 @@ func main() {
 	}
 
 	log.Info("Starting Gitaly", "version", version.GetVersionString())
-	if err := exec(); err != nil {
+	if err := configure(flag.Arg(0)); err != nil {
 		log.Fatal(err)
 	}
+	log.WithError(run(config.Config)).Error("shutting down")
 	log.Info("Gitaly stopped")
 }
 
-func exec() error {
-	configPath := flag.Arg(0)
+func configure(configPath string) error {
 	if err := loadConfig(configPath); err != nil {
 		return fmt.Errorf("load config: config_path %q: %w", configPath, err)
 	}
 
 	glog.Configure(config.Config.Logging.Format, config.Config.Logging.Level)
 
-	ctx, cancel := context.WithCancel(context.Background())
-	defer cancel()
-
 	cgroupsManager := cgroups.NewManager(config.Config.Cgroups)
 	if err := cgroupsManager.Setup(); err != nil {
 		return fmt.Errorf("failed setting up cgroups: %w", err)
@@ -100,39 +97,48 @@ func exec() error {
 		}
 	}()
 
+	if err := verifyGitVersion(); err != nil {
+		return err
+	}
+
+	sentry.ConfigureSentry(version.GetVersion(), sentry.Config(config.Config.Logging.Sentry))
+	config.Config.Prometheus.Configure()
+	config.ConfigureConcurrencyLimits(config.Config)
+	tracing.Initialize(tracing.WithServiceName("gitaly"))
+
+	return nil
+}
+
+func verifyGitVersion() error {
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
+
 	gitVersion, err := git.Version(ctx)
 	if err != nil {
-		return fmt.Errorf("Git version detection: %w", err)
+		return fmt.Errorf("git version detection: %w", err)
 	}
 
 	supported, err := git.SupportedVersion(gitVersion)
 	if err != nil {
-		return fmt.Errorf("Git version comparison: %w", err)
+		return fmt.Errorf("git version comparison: %w", err)
 	}
 	if !supported {
 		return fmt.Errorf("unsupported Git version: %q", gitVersion)
 	}
+	return nil
+}
+
+func run(cfg config.Cfg) error {
+	tempdir.StartCleaning(cfg.Storages, time.Hour)
+
+	ctx, cancel := context.WithCancel(context.Background())
+	defer cancel()
 
 	b, err := bootstrap.New()
 	if err != nil {
 		return fmt.Errorf("init bootstrap: %w", err)
 	}
 
-	sentry.ConfigureSentry(version.GetVersion(), sentry.Config(config.Config.Logging.Sentry))
-	config.Config.Prometheus.Configure()
-	config.ConfigureConcurrencyLimits(config.Config)
-	tracing.Initialize(tracing.WithServiceName("gitaly"))
-
-	tempdir.StartCleaning(config.Config.Storages, time.Hour)
-
-	log.WithError(run(ctx, config.Config, b)).Error("shutting down")
-	return nil
-}
-
-func run(ctx context.Context, cfg config.Cfg, b *bootstrap.Bootstrap) error {
-	var gitlabAPI hook.GitlabAPI
-	var err error
-
 	transactionManager := transaction.NewManager(cfg)
 	prometheus.MustRegister(transactionManager)
 
@@ -143,7 +149,7 @@ func run(ctx context.Context, cfg config.Cfg, b *bootstrap.Bootstrap) error {
 	if config.SkipHooks() {
 		log.Warn("skipping GitLab API client creation since hooks are bypassed via GITALY_TESTING_NO_GIT_HOOKS")
 	} else {
-		gitlabAPI, err = hook.NewGitlabAPI(cfg.Gitlab, cfg.TLS)
+		gitlabAPI, err := hook.NewGitlabAPI(cfg.Gitlab, cfg.TLS)
 		if err != nil {
 			return fmt.Errorf("could not create GitLab API client: %w", err)
 		}
diff --git a/internal/gitaly/service/blob/testhelper_test.go b/internal/gitaly/service/blob/testhelper_test.go
index 4452c4b9d..d8ea88ac3 100644
--- a/internal/gitaly/service/blob/testhelper_test.go
+++ b/internal/gitaly/service/blob/testhelper_test.go
@@ -27,7 +27,8 @@ func testMain(m *testing.M) int {
 	defer cleanup()
 
 	if err := testhelper.ConfigureRuby(&config.Config); err != nil {
-		log.Fatal(err)
+		log.Error(err)
+		return 1
 	}
 
 	if err := rubyServer.Start(); err != nil {
diff --git a/internal/praefect/nodes/init_sql_test.go b/internal/praefect/nodes/init_sql_test.go
index 3f0340f56..262439400 100644
--- a/internal/praefect/nodes/init_sql_test.go
+++ b/internal/praefect/nodes/init_sql_test.go
@@ -15,7 +15,7 @@ func TestMain(m *testing.M) {
 	os.Exit(testMain(m))
 }
 
-func testMain(m *testing.M) int {
+func testMain(m *testing.M) (code int) {
 	defer testhelper.MustHaveNoChildProcess()
 	cleanup := testhelper.Configure()
 	defer cleanup()
@@ -23,7 +23,8 @@ func testMain(m *testing.M) int {
 	// Clean closes connection to database once all tests are done
 	defer func() {
 		if err := glsql.Clean(); err != nil {
-			log.Fatalln(err, "database disconnection failure")
+			log.Println("database disconnection failure", err)
+			code = 1
 		}
 	}()
 
diff --git a/internal/testhelper/repo.go b/internal/testhelper/repo.go
index a98c6d5ac..e230036cb 100644
--- a/internal/testhelper/repo.go
+++ b/internal/testhelper/repo.go
@@ -114,7 +114,7 @@ func NewTestRepoWithWorktree(t testing.TB) (repo *gitalypb.Repository, repoPath
 func testRepositoryPath(t testing.TB) string {
 	_, currentFile, _, ok := runtime.Caller(0)
 	if !ok {
-		log.Fatal("could not get caller info")
+		require.Fail(t, "could not get caller info")
 	}
 
 	path := filepath.Join(filepath.Dir(currentFile), "..", "..", "_build", "testrepos", "gitlab-test.git")
diff --git a/internal/testhelper/testhelper.go b/internal/testhelper/testhelper.go
index f1d0123d0..389878bca 100644
--- a/internal/testhelper/testhelper.go
+++ b/internal/testhelper/testhelper.go
@@ -70,7 +70,7 @@ func MustReadFile(t testing.TB, filename string) []byte {
 // GitlabTestStoragePath returns the storage path to the gitlab-test repo.
 func GitlabTestStoragePath() string {
 	if testDirectory == "" {
-		log.Fatal("you must call testhelper.Configure() before GitlabTestStoragePath()")
+		panic("you must call testhelper.Configure() before GitlabTestStoragePath()")
 	}
 	return filepath.Join(testDirectory, "storage")
 }
@@ -122,7 +122,7 @@ func MustRunCommand(t testing.TB, stdin io.Reader, name string, args ...string)
 		if t == nil {
 			log.Print(name, args)
 			log.Printf("%s", stderr)
-			log.Fatal(err)
+			panic(err)
 		} else {
 			t.Log(name, args)
 			t.Logf("%s", stderr)
@@ -364,7 +364,7 @@ func AssertPathNotExists(t testing.TB, path string) {
 // TempDir is a wrapper around ioutil.TempDir that provides a cleanup function.
 func TempDir(t testing.TB) (string, func()) {
 	if testDirectory == "" {
-		log.Fatal("you must call testhelper.Configure() before TempDir()")
+		panic("you must call testhelper.Configure() before TempDir()")
 	}
 
 	tmpDir, err := ioutil.TempDir(testDirectory, "")
diff --git a/internal/testhelper/testserver.go b/internal/testhelper/testserver.go
index 127854ef1..3d80a3ffa 100644
--- a/internal/testhelper/testserver.go
+++ b/internal/testhelper/testserver.go
@@ -717,7 +717,7 @@ func handlePostReceive(options GitlabTestServerOptions) func(w http.ResponseWrit
 		w.Header().Set("Content-Type", "application/json")
 		w.WriteHeader(http.StatusOK)
 		if err := json.NewEncoder(w).Encode(&response); err != nil {
-			log.Fatal(err)
+			panic(err)
 		}
 	}
 }