fix: Parse GitLab API URL during SetClient setup

*   Added validation by parsing the determined GitLab API URL before
initializing the client in `SetClient`.
* Let the user know if the URL is invalid by returning an error instead
  of a cryptic error message later on.
*   Refactored `TestSetClientDetectAPIURL` into a table-driven test for better
coverage and clarity of URL detection logic.

Jira: https://issues.redhat.com/browse/SRVKP-7451

Signed-off-by: Chmouel Boudjnah <[email protected]>

Author: chmouel

pkg/provider/gitlab/gitlab.go

@@ -130,6 +130,10 @@ func (v *Provider) SetClient(_ context.Context, run *params.Run, runevent *info.
 		// this really should not happen but let's just hope this is it
 		apiURL = apiPublicURL
 	}
+	_, err = url.Parse(apiURL)
+	if err != nil {
+		return fmt.Errorf("failed to parse api url %s: %w", apiURL, err)
+	}
 	v.apiURL = apiURL
 
 	v.Client, err = gitlab.NewClient(runevent.Provider.Token, gitlab.WithBaseURL(apiURL))

pkg/provider/gitlab/gitlab_test.go

@@ -276,33 +276,140 @@ func TestSetClient(t *testing.T) {
 }
 
 func TestSetClientDetectAPIURL(t *testing.T) {
-	fakehost := "https://fakehost.com"
 	ctx, _ := rtesting.SetupFakeContext(t)
-	client, _, tearDown := thelp.Setup(t)
+	mockClient, _, tearDown := thelp.Setup(t)
 	defer tearDown()
-	v := &Provider{Client: client}
-	event := info.NewEvent()
-	err := v.SetClient(ctx, nil, event, nil, nil)
-	assert.ErrorContains(t, err, "no git_provider.secret has been set")
-
-	event.Provider.Token = "hello"
-	event.TargetProjectID = 10
-	event.SourceProjectID = 10
-
-	v.repoURL, event.URL, event.Provider.URL = "", "", ""
-	event.URL = fmt.Sprintf("%s/hello-this-is-me-ze/project", fakehost)
-	err = v.SetClient(ctx, nil, event, nil, nil)
-	assert.NilError(t, err)
-	assert.Equal(t, fakehost, v.apiURL)
 
-	v.repoURL, event.URL, event.Provider.URL = "", "", ""
-	event.Provider.URL = fmt.Sprintf("%s/hello-this-is-me-ze/anotherproject", fakehost)
-	assert.Equal(t, fakehost, v.apiURL)
+	// Define test cases
+	tests := []struct {
+		name              string
+		providerToken     string
+		providerURL       string // input: event.Provider.URL
+		repoURL           string // input: v.repoURL
+		pathWithNamespace string // input: v.pathWithNamespace (needed if repoURL is used)
+		eventURL          string // input: event.URL
+		// Define expected outcomes
+		expectedAPIURL string
+		expectedError  string // Substring expected in the error message, "" for no error
+	}{
+		{
+			name:          "Error: No token provided",
+			providerToken: "",
+			expectedError: "no git_provider.secret has been set",
+		},
+		{
+			name:              "Success: API URL from event.Provider.URL (highest precedence)",
+			providerToken:     "token",
+			providerURL:       "https://provider.example.com",
+			repoURL:           "https://repo.example.com/foo/bar", // Should be ignored
+			pathWithNamespace: "foo/bar",
+			eventURL:          "https://event.example.com/foo/bar", // Should be ignored
+			expectedAPIURL:    "https://provider.example.com",
+			expectedError:     "",
+		},
+		{
+			name:              "Success: API URL from v.repoURL (non-public)",
+			providerToken:     "token",
+			providerURL:       "", // This must be empty to test the next case
+			repoURL:           "https://private-gitlab.com/my/repo",
+			pathWithNamespace: "my/repo",
+			eventURL:          "https://event.example.com/my/repo", // Should be ignored
+			expectedAPIURL:    "https://private-gitlab.com/",
+			expectedError:     "",
+		},
+		{
+			name:           "Success: API URL from event.URL",
+			providerToken:  "token",
+			providerURL:    "", // This must be empty
+			repoURL:        "", // This must be empty
+			eventURL:       "https://event-url.com/org/project",
+			expectedAPIURL: "https://event-url.com",
+			expectedError:  "",
+		},
+		{
+			name:           "Success: Fallback to default public API URL",
+			providerToken:  "token",
+			providerURL:    "",
+			repoURL:        "",
+			eventURL:       "",
+			expectedAPIURL: apiPublicURL, // Default case
+			expectedError:  "",
+		},
+		{
+			name:              "Success: Default URL when repoURL is public Gitlab",
+			providerToken:     "token",
+			providerURL:       "",
+			repoURL:           apiPublicURL + "/public/repo", // Starts with public URL, so skipped
+			pathWithNamespace: "public/repo",
+			eventURL:          "", // Falls through to default
+			expectedAPIURL:    apiPublicURL,
+			expectedError:     "",
+		},
+		{
+			name:          "Error: Invalid URL from event.URL",
+			providerToken: "token",
+			providerURL:   "",
+			repoURL:       "",
+			eventURL:      "://bad-schema",
+			expectedError: "parse \"://bad-schema\": missing protocol scheme", // Specific error from url.Parse
+		},
+		{
+			name:          "Error: Invalid URL from event.Provider.URL (final parse)",
+			providerToken: "token",
+			providerURL:   "ht tp://invalid host", // Invalid URL format
+			repoURL:       "",
+			eventURL:      "",
+			expectedError: "failed to parse api url", // Wrapper error message
+		},
+		{
+			name:              "Error: Invalid URL from v.repoURL (final parse)",
+			providerToken:     "token",
+			providerURL:       "",
+			repoURL:           "ht tp://invalid.repo.url/foo/bar", // Invalid format
+			pathWithNamespace: "foo/bar",
+			eventURL:          "",
+			// Note: The calculated apiURL would be "ht tp://invalid.repo.url" before parsing
+			expectedError: "failed to parse api url",
+		},
+	}
 
-	v.repoURL = fmt.Sprintf("%s/hello-this-is-me-ze/anotherproject", fakehost)
-	v.pathWithNamespace = "hello-this-is-me-ze/anotherproject"
-	event.URL, event.Provider.URL = "", ""
-	assert.Equal(t, fakehost, v.apiURL)
+	// Run test cases
+	for _, tc := range tests {
+		t.Run(tc.name, func(t *testing.T) {
+			// Setup specific to this test case
+			v := &Provider{
+				Client:            mockClient, // Use the shared mock client
+				repoURL:           tc.repoURL,
+				pathWithNamespace: tc.pathWithNamespace,
+			}
+			event := info.NewEvent()
+			event.Provider.Token = tc.providerToken
+			event.Provider.URL = tc.providerURL
+			event.URL = tc.eventURL
+			// Set some default IDs to avoid potential nil pointer issues or side effects
+			// if the GetProject part of SetClient is reached unexpectedly.
+			event.TargetProjectID = 1
+			event.SourceProjectID = 1
+
+			// Execute the function under test
+			// Using placeholder nil values for arguments not directly related to URL detection
+			err := v.SetClient(ctx, nil, event, nil, nil)
+
+			// Assertions
+			if tc.expectedError != "" {
+				assert.ErrorContains(t, err, tc.expectedError)
+				// If an error is expected, we usually don't check the apiURL state,
+				// as it might be indeterminate or irrelevant.
+			} else {
+				assert.NilError(t, err)
+				// Only check the resulting apiURL if no error was expected
+				assert.Equal(t, tc.expectedAPIURL, v.apiURL)
+				// Optionally, check if the client was actually set (if no error)
+				assert.Assert(t, v.Client != nil)
+				assert.Assert(t, v.Token != nil && *v.Token == tc.providerToken)
+			}
+		})
+	}
 }
 
 func TestGetTektonDir(t *testing.T) {