From 3bd1fe9b588ce49382f91b6ea8e6425dc1924f1c Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 25 Nov 2025 23:25:44 -0500 Subject: [PATCH 1/3] Fix GitLab integration test failures from PR #339 MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes go vet errors introduced in the GitLab integration PR that merged with failing tests. Changes improve code testability by using interfaces instead of concrete types. Changes: - Fix test assertions to use correct struct fields (result.User.Username instead of result.Username, result.User.ID instead of result.GitLabUserID) - Change kubernetes.Clientset to kubernetes.Interface in function signatures to support both real and fake clients for testing - Fix GetCurrentUser call to use package-level function instead of method - Remove unused imports (time, corev1) Updated functions: - k8s.StoreGitLabToken, k8s.GetGitLabToken, k8s.DeleteGitLabToken - k8s.HasGitLabToken, k8s.StoreGitLabConnection, k8s.GetGitLabConnection - k8s.DeleteGitLabConnection, k8s.HasGitLabConnection - k8s.ListGitLabConnections, git.GetGitLabToken - gitlab.NewConnectionManager and ConnectionManager.clientset field This follows Go best practices (accept interfaces, return structs) and makes the code more testable without changing runtime behavior. Related: PR #339 (GitLab integration) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Signed-off-by: sallyom --- components/backend/git/operations.go | 2 +- components/backend/gitlab/connection.go | 4 ++-- components/backend/k8s/configmap.go | 10 +++++----- components/backend/k8s/secrets.go | 8 ++++---- .../integration/gitlab/gitlab_integration_test.go | 9 ++++----- 5 files changed, 16 insertions(+), 17 deletions(-) diff --git a/components/backend/git/operations.go b/components/backend/git/operations.go index 69022267f..3ebf9ad46 100644 --- a/components/backend/git/operations.go +++ b/components/backend/git/operations.go @@ -114,7 +114,7 @@ func GetGitHubToken(ctx context.Context, k8sClient *kubernetes.Clientset, dynCli } // GetGitLabToken retrieves a GitLab Personal Access Token for a user -func GetGitLabToken(ctx context.Context, k8sClient *kubernetes.Clientset, project, userID string) (string, error) { +func GetGitLabToken(ctx context.Context, k8sClient kubernetes.Interface, project, userID string) (string, error) { if k8sClient == nil { log.Printf("Cannot read GitLab token: k8s client is nil") return "", fmt.Errorf("no GitLab credentials available. Please connect your GitLab account") diff --git a/components/backend/gitlab/connection.go b/components/backend/gitlab/connection.go index 1d6e924b5..0832e49c1 100644 --- a/components/backend/gitlab/connection.go +++ b/components/backend/gitlab/connection.go @@ -14,12 +14,12 @@ import ( // ConnectionManager handles GitLab connection operations type ConnectionManager struct { - clientset *kubernetes.Clientset + clientset kubernetes.Interface namespace string } // NewConnectionManager creates a new connection manager -func NewConnectionManager(clientset *kubernetes.Clientset, namespace string) *ConnectionManager { +func NewConnectionManager(clientset kubernetes.Interface, namespace string) *ConnectionManager { return &ConnectionManager{ clientset: clientset, namespace: namespace, diff --git a/components/backend/k8s/configmap.go b/components/backend/k8s/configmap.go index f5d791113..c97b69535 100644 --- a/components/backend/k8s/configmap.go +++ b/components/backend/k8s/configmap.go @@ -19,7 +19,7 @@ const ( ) // StoreGitLabConnection stores GitLab connection metadata in a ConfigMap -func StoreGitLabConnection(ctx context.Context, clientset *kubernetes.Clientset, namespace string, connection *types.GitLabConnection) error { +func StoreGitLabConnection(ctx context.Context, clientset kubernetes.Interface, namespace string, connection *types.GitLabConnection) error { configMapsClient := clientset.CoreV1().ConfigMaps(namespace) // Serialize connection to JSON @@ -68,7 +68,7 @@ func StoreGitLabConnection(ctx context.Context, clientset *kubernetes.Clientset, } // GetGitLabConnection retrieves GitLab connection metadata from a ConfigMap -func GetGitLabConnection(ctx context.Context, clientset *kubernetes.Clientset, namespace, userID string) (*types.GitLabConnection, error) { +func GetGitLabConnection(ctx context.Context, clientset kubernetes.Interface, namespace, userID string) (*types.GitLabConnection, error) { configMapsClient := clientset.CoreV1().ConfigMaps(namespace) configMap, err := configMapsClient.Get(ctx, GitLabConnectionsConfigMapName, metav1.GetOptions{}) @@ -93,7 +93,7 @@ func GetGitLabConnection(ctx context.Context, clientset *kubernetes.Clientset, n } // DeleteGitLabConnection removes GitLab connection metadata from a ConfigMap -func DeleteGitLabConnection(ctx context.Context, clientset *kubernetes.Clientset, namespace, userID string) error { +func DeleteGitLabConnection(ctx context.Context, clientset kubernetes.Interface, namespace, userID string) error { configMapsClient := clientset.CoreV1().ConfigMaps(namespace) configMap, err := configMapsClient.Get(ctx, GitLabConnectionsConfigMapName, metav1.GetOptions{}) @@ -119,7 +119,7 @@ func DeleteGitLabConnection(ctx context.Context, clientset *kubernetes.Clientset } // HasGitLabConnection checks if a user has a GitLab connection stored -func HasGitLabConnection(ctx context.Context, clientset *kubernetes.Clientset, namespace, userID string) (bool, error) { +func HasGitLabConnection(ctx context.Context, clientset kubernetes.Interface, namespace, userID string) (bool, error) { configMapsClient := clientset.CoreV1().ConfigMaps(namespace) configMap, err := configMapsClient.Get(ctx, GitLabConnectionsConfigMapName, metav1.GetOptions{}) @@ -135,7 +135,7 @@ func HasGitLabConnection(ctx context.Context, clientset *kubernetes.Clientset, n } // ListGitLabConnections retrieves all GitLab connections from a ConfigMap -func ListGitLabConnections(ctx context.Context, clientset *kubernetes.Clientset, namespace string) ([]*types.GitLabConnection, error) { +func ListGitLabConnections(ctx context.Context, clientset kubernetes.Interface, namespace string) ([]*types.GitLabConnection, error) { configMapsClient := clientset.CoreV1().ConfigMaps(namespace) configMap, err := configMapsClient.Get(ctx, GitLabConnectionsConfigMapName, metav1.GetOptions{}) diff --git a/components/backend/k8s/secrets.go b/components/backend/k8s/secrets.go index 80105915a..151fc0c5c 100644 --- a/components/backend/k8s/secrets.go +++ b/components/backend/k8s/secrets.go @@ -18,7 +18,7 @@ const ( // StoreGitLabToken stores a GitLab Personal Access Token in Kubernetes Secrets // Uses optimistic concurrency control with retry to handle concurrent updates -func StoreGitLabToken(ctx context.Context, clientset *kubernetes.Clientset, namespace, userID, token string) error { +func StoreGitLabToken(ctx context.Context, clientset kubernetes.Interface, namespace, userID, token string) error { secretsClient := clientset.CoreV1().Secrets(namespace) // Retry up to 3 times with exponential backoff @@ -88,7 +88,7 @@ func StoreGitLabToken(ctx context.Context, clientset *kubernetes.Clientset, name } // GetGitLabToken retrieves a GitLab Personal Access Token from Kubernetes Secrets -func GetGitLabToken(ctx context.Context, clientset *kubernetes.Clientset, namespace, userID string) (string, error) { +func GetGitLabToken(ctx context.Context, clientset kubernetes.Interface, namespace, userID string) (string, error) { secretsClient := clientset.CoreV1().Secrets(namespace) secret, err := secretsClient.Get(ctx, GitLabTokensSecretName, metav1.GetOptions{}) @@ -109,7 +109,7 @@ func GetGitLabToken(ctx context.Context, clientset *kubernetes.Clientset, namesp // DeleteGitLabToken removes a GitLab Personal Access Token from Kubernetes Secrets // Uses optimistic concurrency control with retry to handle concurrent updates -func DeleteGitLabToken(ctx context.Context, clientset *kubernetes.Clientset, namespace, userID string) error { +func DeleteGitLabToken(ctx context.Context, clientset kubernetes.Interface, namespace, userID string) error { secretsClient := clientset.CoreV1().Secrets(namespace) // Retry up to 3 times with exponential backoff @@ -155,7 +155,7 @@ func DeleteGitLabToken(ctx context.Context, clientset *kubernetes.Clientset, nam } // HasGitLabToken checks if a user has a GitLab token stored -func HasGitLabToken(ctx context.Context, clientset *kubernetes.Clientset, namespace, userID string) (bool, error) { +func HasGitLabToken(ctx context.Context, clientset kubernetes.Interface, namespace, userID string) (bool, error) { secretsClient := clientset.CoreV1().Secrets(namespace) secret, err := secretsClient.Get(ctx, GitLabTokensSecretName, metav1.GetOptions{}) diff --git a/components/backend/tests/integration/gitlab/gitlab_integration_test.go b/components/backend/tests/integration/gitlab/gitlab_integration_test.go index 3a26fec26..d638cea14 100644 --- a/components/backend/tests/integration/gitlab/gitlab_integration_test.go +++ b/components/backend/tests/integration/gitlab/gitlab_integration_test.go @@ -4,7 +4,6 @@ import ( "context" "os" "testing" - "time" "ambient-code-backend/git" "ambient-code-backend/gitlab" @@ -14,7 +13,6 @@ import ( "github.com/stretchr/testify/assert" "github.com/stretchr/testify/require" - corev1 "k8s.io/api/core/v1" metav1 "k8s.io/apimachinery/pkg/apis/meta/v1" "k8s.io/client-go/kubernetes/fake" ) @@ -48,8 +46,9 @@ func TestGitLabIntegrationEnd2End(t *testing.T) { result, err := gitlab.ValidateGitLabToken(ctx, gitlabToken, "https://gitlab.com") require.NoError(t, err, "Token validation should succeed") assert.True(t, result.Valid, "Token should be valid") - assert.NotEmpty(t, result.Username, "Username should be populated") - assert.NotEmpty(t, result.GitLabUserID, "GitLab user ID should be populated") + assert.NotNil(t, result.User, "User should be populated") + assert.NotEmpty(t, result.User.Username, "Username should be populated") + assert.NotZero(t, result.User.ID, "GitLab user ID should be populated") }) // Test token storage @@ -362,7 +361,7 @@ func TestGitLabClientLogging(t *testing.T) { client := gitlab.NewClient("https://gitlab.com/api/v4", token) // Make a simple API request - resp, err := client.GetCurrentUser(ctx) + resp, err := gitlab.GetCurrentUser(ctx, client) require.NoError(t, err, "API call should succeed") assert.NotNil(t, resp, "Response should not be nil") From 301865bb2260994bcd5a325508c7e3c03010bb4f Mon Sep 17 00:00:00 2001 From: sallyom Date: Tue, 25 Nov 2025 23:43:08 -0500 Subject: [PATCH 2/3] Fix golangci-lint errors in GitLab integration code MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses linting errors flagged by golangci-lint CI checks: 1. git/operations.go: - Use tagged switch instead of if-else chains for provider checks (QF1003) - Lowercase error message prefixes per Go conventions (ST1005) - Merge conditional assignments into variable declaration (QF1007) 2. handlers/gitlab_auth.go: - Apply De Morgan's law to simplify character validation logic (QF1001) These changes improve code quality and follow Go best practices without changing functionality. Related: PR #339 (GitLab integration) 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-Authored-By: Claude Signed-off-by: sallyom --- components/backend/git/operations.go | 43 +++++++++++----------- components/backend/handlers/gitlab_auth.go | 8 ++-- 2 files changed, 25 insertions(+), 26 deletions(-) diff --git a/components/backend/git/operations.go b/components/backend/git/operations.go index 3ebf9ad46..0b3c84e6d 100644 --- a/components/backend/git/operations.go +++ b/components/backend/git/operations.go @@ -817,40 +817,44 @@ func DetectPushError(repoURL, stderr, stdout string) error { // Check for authentication/permission errors if strings.Contains(combined, "403") || strings.Contains(combined, "forbidden") { - if provider == types.ProviderGitLab { - return fmt.Errorf("GitLab push failed: Insufficient permissions. Ensure your GitLab token has 'write_repository' scope. You can update your token by reconnecting your GitLab account with the required permissions") - } else if provider == types.ProviderGitHub { - return fmt.Errorf("GitHub push failed: Insufficient permissions. Check that your GitHub App installation has write access to this repository") + switch provider { + case types.ProviderGitLab: + return fmt.Errorf("GitLab push failed: insufficient permissions. Ensure your GitLab token has 'write_repository' scope. You can update your token by reconnecting your GitLab account with the required permissions") + case types.ProviderGitHub: + return fmt.Errorf("GitHub push failed: insufficient permissions. Check that your GitHub App installation has write access to this repository") + default: + return fmt.Errorf("push failed: insufficient permissions (403 Forbidden)") } - return fmt.Errorf("Push failed: Insufficient permissions (403 Forbidden)") } // Check for authentication failures if strings.Contains(combined, "401") || strings.Contains(combined, "unauthorized") || strings.Contains(combined, "authentication failed") { - if provider == types.ProviderGitLab { - return fmt.Errorf("GitLab push failed: Authentication failed. Your GitLab token may be invalid or expired. Please reconnect your GitLab account") - } else if provider == types.ProviderGitHub { - return fmt.Errorf("GitHub push failed: Authentication failed. Check your GitHub App installation") + switch provider { + case types.ProviderGitLab: + return fmt.Errorf("GitLab push failed: authentication failed. Your GitLab token may be invalid or expired. Please reconnect your GitLab account") + case types.ProviderGitHub: + return fmt.Errorf("GitHub push failed: authentication failed. Check your GitHub App installation") + default: + return fmt.Errorf("push failed: authentication failed (401 Unauthorized)") } - return fmt.Errorf("Push failed: Authentication failed (401 Unauthorized)") } // Check for network errors if strings.Contains(combined, "could not resolve host") || strings.Contains(combined, "connection refused") { - return fmt.Errorf("Push failed: Unable to connect to %s. Check network connectivity", extractHostFromURL(repoURL)) + return fmt.Errorf("push failed: unable to connect to %s. Check network connectivity", extractHostFromURL(repoURL)) } // Check for rate limiting if strings.Contains(combined, "429") || strings.Contains(combined, "rate limit") { if provider == types.ProviderGitLab { - return fmt.Errorf("GitLab push failed: Rate limit exceeded. Please wait a few minutes before retrying") + return fmt.Errorf("GitLab push failed: rate limit exceeded. Please wait a few minutes before retrying") } - return fmt.Errorf("Push failed: API rate limit exceeded. Please wait before retrying") + return fmt.Errorf("push failed: API rate limit exceeded. Please wait before retrying") } // Check for repository not found if strings.Contains(combined, "404") || strings.Contains(combined, "not found") || strings.Contains(combined, "repository not found") { - return fmt.Errorf("Push failed: Repository not found. Verify the repository URL: %s", repoURL) + return fmt.Errorf("push failed: repository not found. Verify the repository URL: %s", repoURL) } // Return original error if no pattern matched @@ -861,7 +865,7 @@ func DetectPushError(repoURL, stderr, stdout string) error { if len(errMsg) > 500 { errMsg = errMsg[:500] + "..." } - return fmt.Errorf("Push failed: %s", errMsg) + return fmt.Errorf("push failed: %s", errMsg) } // extractHostFromURL extracts the host from a git URL for error messages @@ -1491,13 +1495,8 @@ func validateGitLabPushAccess(ctx context.Context, repoURL, gitlabToken string) // GitLab access levels: 10=Guest, 20=Reporter, 30=Developer, 40=Maintainer, 50=Owner // Need at least Developer (30) to push - hasAccess := false - if projectInfo.Permissions.ProjectAccess != nil && projectInfo.Permissions.ProjectAccess.AccessLevel >= 30 { - hasAccess = true - } - if projectInfo.Permissions.GroupAccess != nil && projectInfo.Permissions.GroupAccess.AccessLevel >= 30 { - hasAccess = true - } + hasAccess := (projectInfo.Permissions.ProjectAccess != nil && projectInfo.Permissions.ProjectAccess.AccessLevel >= 30) || + (projectInfo.Permissions.GroupAccess != nil && projectInfo.Permissions.GroupAccess.AccessLevel >= 30) if !hasAccess { return fmt.Errorf("you don't have push access to %s. You need at least Developer (30) access level. Please check your permissions in GitLab", repoURL) diff --git a/components/backend/handlers/gitlab_auth.go b/components/backend/handlers/gitlab_auth.go index 193d2ab1b..1d8436909 100644 --- a/components/backend/handlers/gitlab_auth.go +++ b/components/backend/handlers/gitlab_auth.go @@ -85,10 +85,10 @@ func validateGitLabInput(instanceURL, token string) error { // Validate token contains only valid characters (alphanumeric and some special chars) // GitLab tokens use: a-z, A-Z, 0-9, -, _ for _, char := range token { - if !((char >= 'a' && char <= 'z') || - (char >= 'A' && char <= 'Z') || - (char >= '0' && char <= '9') || - char == '-' || char == '_') { + if (char < 'a' || char > 'z') && + (char < 'A' || char > 'Z') && + (char < '0' || char > '9') && + char != '-' && char != '_' { return fmt.Errorf("token contains invalid characters") } } From 80438f53e6650a1eee034a17230dc0bf3a6294bd Mon Sep 17 00:00:00 2001 From: sallyom Date: Wed, 26 Nov 2025 00:16:33 -0500 Subject: [PATCH 3/3] Fix exported function/type comment formatting Addresses remaining golangci-lint staticcheck errors: - types/gitlab.go: Fix ParsedGitLabRepo comment format (ST1021) - types/common.go: Fix BoolPtr, StringPtr, IntPtr comment format (ST1020) Per Go conventions, comments on exported types and functions must start with the exported identifier name. --- components/backend/types/common.go | 4 +++- components/backend/types/gitlab.go | 2 +- 2 files changed, 4 insertions(+), 2 deletions(-) diff --git a/components/backend/types/common.go b/components/backend/types/common.go index 1d86bb2a1..930046f18 100644 --- a/components/backend/types/common.go +++ b/components/backend/types/common.go @@ -80,15 +80,17 @@ type FileContent struct { SHA string `json:"sha,omitempty"` } -// Helper functions for pointer types +// BoolPtr returns a pointer to the given bool value. func BoolPtr(b bool) *bool { return &b } +// StringPtr returns a pointer to the given string value. func StringPtr(s string) *string { return &s } +// IntPtr returns a pointer to the given int value. func IntPtr(i int) *int { return &i } diff --git a/components/backend/types/gitlab.go b/components/backend/types/gitlab.go index 78dfe3830..6532c71b5 100644 --- a/components/backend/types/gitlab.go +++ b/components/backend/types/gitlab.go @@ -11,7 +11,7 @@ type GitLabConnection struct { UpdatedAt time.Time `json:"updatedAt"` // Last connection update } -// GitLabRepository extends GitRepository for GitLab-specific attributes +// ParsedGitLabRepo extends GitRepository for GitLab-specific attributes. // Internal parsed representation (not persisted to CRD) type ParsedGitLabRepo struct { Host string // "gitlab.com" or "gitlab.example.com"