Make initialization of gitlab client more explicit

Add more test cases for domains. Support sourceAuto and use IsReady
for gitlab source.

3 files changed, 81 insertions, 38 deletions

diff --git a/app_test.go b/app_test.go
index 3c485804..f35e90c5 100644
--- a/app_test.go
+++ b/app_test.go
@@ -85,7 +85,8 @@ func TestHealthCheckMiddleware(t *testing.T) {
 
 	app := theApp{
 		appConfig: appConfig{
-			StatusPath: "/-/healthcheck",
+			StatusPath:                "/-/healthcheck",
+			DomainConfigurationSource: "auto",
 		},
 	}
 
diff --git a/internal/source/domains.go b/internal/source/domains.go
index 53956b6b..e4a582ea 100644
--- a/internal/source/domains.go
+++ b/internal/source/domains.go
@@ -4,6 +4,8 @@ import (
 	"fmt"
 	"regexp"
 
+	"gitlab.com/gitlab-org/labkit/log"
+
 	"gitlab.com/gitlab-org/gitlab-pages/internal/domain"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/disk"
 	"gitlab.com/gitlab-org/gitlab-pages/internal/source/gitlab"
@@ -42,45 +44,56 @@ func NewDomains(config Config) (*Domains, error) {
 		disk: disk.New(),
 	}
 
-	domains.setConfigSource(config)
-
-	// Creating a glClient will start polling connectivity in the background
-	glClient, err := newGitlabClient(config)
-	if err != nil && domains.configSource != sourceDisk {
+	if err := domains.setConfigSource(config); err != nil {
 		return nil, err
 	}
 
-	// TODO: handle DomainConfigSource == "auto" https://gitlab.com/gitlab-org/gitlab/-/issues/218358
-	// attach gitlab by default when source is not disk (auto, gitlab)
-	if domains.configSource != sourceDisk {
-		domains.gitlab = glClient
-	}
-
 	return domains, nil
 }
 
-// defaults to disk
-func (d *Domains) setConfigSource(config Config) {
+// setConfigSource and initialize gitlab source
+// returns error if -domain-config-source is not valid
+// returns error if -domain-config-source=gitlab and init fails
+func (d *Domains) setConfigSource(config Config) error {
+	// TODO: Handle domain-config-source=auto https://gitlab.com/gitlab-org/gitlab/-/issues/218358
+	// attach gitlab by default when source is not disk (auto, gitlab)
 	switch config.DomainConfigSource() {
 	case "gitlab":
 		// TODO:  https://gitlab.com/gitlab-org/gitlab/-/issues/218357
 		d.configSource = sourceGitlab
+		return d.setGitLabClient(config)
 	case "auto":
-		// TODO: https://gitlab.com/gitlab-org/gitlab/-/issues/218358
+		// TODO: handle DomainConfigSource == "auto" https://gitlab.com/gitlab-org/gitlab/-/issues/218358
 		d.configSource = sourceAuto
+		return d.setGitLabClient(config)
 	case "disk":
-		fallthrough
-	default:
 		d.configSource = sourceDisk
+	default:
+		return fmt.Errorf("invalid option for -domain-config-source: %q", config.DomainConfigSource())
 	}
+
+	return nil
 }
 
-func newGitlabClient(config Config) (*gitlab.Gitlab, error) {
-	if len(config.InternalGitLabServerURL()) == 0 || len(config.GitlabAPISecret()) == 0 {
-		return nil, fmt.Errorf("missing -internal-gitlab-server and/or -api-secret-key")
+// setGitLabClient when domain-config-source is `gitlab` or `auto`, only return error for `gitlab` source
+func (d *Domains) setGitLabClient(config Config) error {
+	// We want to notify users about any API issues
+	// Creating a glClient will start polling connectivity in the background
+	// and spam errors in log
+	glClient, err := gitlab.New(config)
+	if err != nil {
+		if d.configSource == sourceGitlab {
+			return err
+		}
+
+		log.WithError(err).Warn("failed to initialize GitLab client for `-domain-config-source=auto`")
+
+		return nil
 	}
 
-	return gitlab.New(config)
+	d.gitlab = glClient
+
+	return nil
 }
 
 // GetDomain retrieves a domain information from a source. We are using two
@@ -120,7 +133,9 @@ func (d *Domains) source(domain string) Source {
 		return d.disk
 	}
 
-	if d.gitlab.IsReady() {
+	// TODO: handle sourceAuto https://gitlab.com/gitlab-org/gitlab/-/issues/218358
+	// check IsReady for sourceAuto for now
+	if d.configSource == sourceGitlab || d.gitlab.IsReady() {
 		return d.gitlab
 	}
 
diff --git a/internal/source/domains_test.go b/internal/source/domains_test.go
index 8431810e..173fc1d3 100644
--- a/internal/source/domains_test.go
+++ b/internal/source/domains_test.go
@@ -35,47 +35,59 @@ func (c sourceConfig) DomainConfigSource() string {
 	return c.domainSource
 }
 
-func TestDomainSources(t *testing.T) {
+func TestNewDomains(t *testing.T) {
 	tests := []struct {
 		name            string
 		sourceConfig    sourceConfig
-		wantErr         bool
+		expectedErr     string
 		expectGitlabNil bool
 	}{
 		{
-			name:            "no_source_config",
-			sourceConfig:    sourceConfig{},
-			wantErr:         false,
-			expectGitlabNil: true,
+			name:         "no_source_config",
+			sourceConfig: sourceConfig{},
+			expectedErr:  "invalid option for -domain-config-source: \"\"",
+		},
+		{
+			name:         "invalid_source_config",
+			sourceConfig: sourceConfig{domainSource: "invalid"},
+			expectedErr:  "invalid option for -domain-config-source: \"invalid\"",
 		},
 		{
 			name:            "disk_source",
 			sourceConfig:    sourceConfig{domainSource: "disk"},
-			wantErr:         false,
 			expectGitlabNil: true,
 		},
 		{
+			name:            "auto_without_api_config",
+			sourceConfig:    sourceConfig{domainSource: "auto"},
+			expectGitlabNil: true,
+		},
+		{
+			name:            "auto_with_api_config",
+			sourceConfig:    sourceConfig{api: "https://gitlab.com", secret: "abc", domainSource: "auto"},
+			expectGitlabNil: false,
+		},
+		{
 			name:         "gitlab_source_success",
 			sourceConfig: sourceConfig{api: "https://gitlab.com", secret: "abc", domainSource: "gitlab"},
-			wantErr:      false,
 		},
 		{
 			name:         "gitlab_source_no_url",
 			sourceConfig: sourceConfig{api: "", secret: "abc", domainSource: "gitlab"},
-			wantErr:      true,
+			expectedErr:  "GitLab API URL or API secret has not been provided",
 		},
 		{
 			name:         "gitlab_source_no_secret",
 			sourceConfig: sourceConfig{api: "https://gitlab.com", secret: "", domainSource: "gitlab"},
-			wantErr:      true,
+			expectedErr:  "GitLab API URL or API secret has not been provided",
 		},
 	}
 
 	for _, tt := range tests {
 		t.Run(tt.name, func(t *testing.T) {
 			domains, err := NewDomains(tt.sourceConfig)
-			if tt.wantErr {
-				require.EqualError(t, err, "missing -internal-gitlab-server and/or -api-secret-key")
+			if tt.expectedErr != "" {
+				require.EqualError(t, err, tt.expectedErr)
 				return
 			}
 			require.NoError(t, err)
@@ -87,14 +99,13 @@ func TestDomainSources(t *testing.T) {
 }
 
 func TestGetDomain(t *testing.T) {
-	t.Run("when requesting an existing domain", func(t *testing.T) {
+	t.Run("when requesting an existing domain for gitlab source", func(t *testing.T) {
 		testDomain := "new-source-test.gitlab.io"
 
 		newSource := NewMockSource()
 		newSource.On("GetDomain", testDomain).
 			Return(&domain.Domain{Name: testDomain}, nil).
 			Once()
-		newSource.On("IsReady").Return(true).Once()
 		defer newSource.AssertExpectations(t)
 
 		domains := newTestDomains(t, newSource, sourceGitlab)
@@ -104,12 +115,28 @@ func TestGetDomain(t *testing.T) {
 		require.NotNil(t, domain)
 	})
 
-	t.Run("when requesting a domain that doesn't exist", func(t *testing.T) {
+	t.Run("when requesting an existing domain for auto source", func(t *testing.T) {
+		testDomain := "new-source-test.gitlab.io"
+
+		newSource := NewMockSource()
+		newSource.On("GetDomain", testDomain).
+			Return(&domain.Domain{Name: testDomain}, nil).
+			Once()
+		newSource.On("IsReady").Return(true).Once()
+		defer newSource.AssertExpectations(t)
+
+		domains := newTestDomains(t, newSource, sourceAuto)
+
+		domain, err := domains.GetDomain(testDomain)
+		require.NoError(t, err)
+		require.NotNil(t, domain)
+	})
+
+	t.Run("when requesting a domain that doesn't exist for gitlab source", func(t *testing.T) {
 		newSource := NewMockSource()
 		newSource.On("GetDomain", "does-not-exist.test.io").
 			Return(nil, nil).
 			Once()
-		newSource.On("IsReady").Return(true).Once()
 
 		defer newSource.AssertExpectations(t)