supporting gitlab connections #1921
Conversation
WalkthroughThis pull request updates the system's VCS connection handling. In the API, the dynamic value for VCS types is now returned instead of a hardcoded value, and the request payload for creating connections includes new fields for GitLab access tokens and webhook secrets. Database migration scripts add columns for encrypted GitLab tokens, secrets, and a new VCS type column with a default value. Model and storage functions are updated to accommodate these parameters, and adjustments are made in the EE controller to pass the appropriate VCS type. An indirect dependency has also been added to the module. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant API_Controller
participant DB_Store
Client->>API_Controller: POST /createVCSConnection with VCS and GitLab details
API_Controller->>DB_Store: Call CreateVCSConnection(name, vcsType, tokens, secrets)
DB_Store-->>API_Controller: Return VCSConnection object or error
API_Controller-->>Client: Respond with connection details
sequenceDiagram
participant EE_Client
participant EE_Controller
participant DB_Store
EE_Client->>EE_Controller: Trigger GithubAppConnectionsConfirm
EE_Controller->>DB_Store: Call CreateVCSConnection (vcsType = Github, ...)
DB_Store-->>EE_Controller: Return result or error
EE_Controller-->>EE_Client: Return confirmation response
Possibly related PRs
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
backend/migrations/20250325134924.sql (1)
1-2: Consider renaming the table to better reflect its purpose.The table name
github_app_connectionsis now storing connections for multiple VCS types (including GitLab and Bitbucket), but its name still suggests it's specific to GitHub. This could lead to confusion for developers maintaining the code in the future.Consider renaming the table to something more generic like
vcs_connectionsin a future migration. If renaming is too disruptive at this stage, adding a descriptive comment in the migration would be helpful.backend/models/storage.go (1)
441-458: Good implementation of GitLab support in CreateVCSConnectionThe function signature and implementation have been updated to properly handle GitLab connections. The function now takes additional parameters for GitLab credentials and properly initializes the VCSConnection struct with the new fields.
One minor issue is that the log message on line 463 still refers to "CreateGithubApp" even though the function now handles multiple VCS types.
Consider updating the log message to be more generic:
- log.Printf("CreateGithubApp (name: %v, url: %v) has been created successfully\n", app.Name, app.GithubAppUrl) + log.Printf("CreateVCSConnection (name: %v, type: %v) has been created successfully\n", app.Name, app.VCSType)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (3)
backend/go.sumis excluded by!**/*.sumbackend/migrations/atlas.sumis excluded by!**/*.sumee/backend/go.sumis excluded by!**/*.sum
📒 Files selected for processing (7)
backend/controllers/connections.go(4 hunks)backend/migrations/20250325115901.sql(1 hunks)backend/migrations/20250325134924.sql(1 hunks)backend/models/github.go(1 hunks)backend/models/storage.go(2 hunks)ee/backend/controllers/github.go(1 hunks)ee/backend/go.mod(1 hunks)
🧰 Additional context used
🧬 Code Definitions (1)
backend/models/github.go (1)
backend/models/scheduler.go (1)
DiggerVCSType(19-19)
🔇 Additional comments (8)
ee/backend/go.mod (1)
26-26: LGTM: New dependency added for database operations.The addition of
ariga.io/atlas-provider-gormas an indirect dependency is properly formatted and appears to support the database operations needed for the GitLab integration.backend/migrations/20250325134924.sql (1)
2-2:❓ Verification inconclusive
Verify consistency of default VCS type.
The default value for
vcs_typeis set to 'bitbucket', but based on the controller code, GitHub connections use 'github'. Ensure this default value is intentional and consistent with the application's logic.
🏁 Script executed:
#!/bin/bash # Check for consistency in VCS type handling across the codebase # Look for all references to VCS types in the codebase echo "Searching for VCS type constants and references..." rg -A 2 -B 2 "DiggerVCS|vcs_type" --type go # Check for any references to the value "bitbucket" in the code echo "Searching for bitbucket references..." rg -A 2 -B 2 "bitbucket" --type goLength of output: 37939
Action: Verify default VCS type consistency
The migration in
backend/migrations/20250325134924.sqlcurrently sets the default value forvcs_typeto'bitbucket', yet several parts of the codebase—such as the GitHub controllers and constants (e.g.,DiggerVCSGithub)—expect GitHub connections to use'github'. Notably:
- In
backend/models/github.go, theVCSTypefield is defined with a GORM default of'bitbucket'.- In
backend/models/orgs.go, a default value of'github'is specified.- Multiple controllers (e.g., in
controllers/github_after_merge.goandcontrollers/github.go) explicitly use the constant for GitHub ("github").Please verify whether the default of
'bitbucket'in the migration is intentional for GitHub app connections or if it should be updated to'github'to align with the controller logic. If the current default is deliberate, consider adding documentation to clarify the rationale for this discrepancy.backend/migrations/20250325115901.sql (1)
1-2: Properly structured migration for GitLab support.Adding encrypted columns for GitLab tokens is a good security practice. The nullable constraint is appropriate since these fields would only be used for GitLab connections.
ee/backend/controllers/github.go (1)
195-195:❓ Verification inconclusive
Verify empty string parameters for GitLab fields.
The function call now passes empty strings ("") for the GitLab access token and webhook secret parameters. Ensure this is the expected behavior and won't cause issues in the backend when processing GitHub connections.
Consider whether
nilor another default value would be more appropriate for these parameters when creating GitHub connections.
🏁 Script executed:
#!/bin/bash # Check the implementation of CreateVCSConnection to understand param handling # Find the CreateVCSConnection implementation echo "Looking for CreateVCSConnection implementation:" rg -A 20 "func.*CreateVCSConnection" --type go # Check how empty GitLab params are handled echo "Checking empty param handling:" rg -A 10 -B 10 "gitlab.*token.*empty|gitlab.*secret.*empty" --type goLength of output: 3815
Verify handling of empty GitLab (and Bitbucket) fields for GitHub connections
The current call to
CreateVCSConnectioninee/backend/controllers/github.go(line 195) passes empty strings for the GitLab token and webhook secret (as well as for the Bitbucket fields). Please verify that defaulting these parameters to empty strings is intentional for GitHub connections and that no downstream validation or processing issues occur. Consider whether usingnilor a more explicit default value might be more appropriate to improve clarity and maintainability.
- File:
ee/backend/controllers/github.go(Line 195)- Diff snippet:
_, err = models.DB.CreateVCSConnection(cfg.GetName(), models.DiggerVCSGithub, cfg.GetID(), cfg.GetClientID(), clientSecretEnc, webhookSecretEnc, PEMEnc, PEM64Enc, *cfg.Owner.Login, cfg.GetHTMLURL(), "", "", "", "", orgId)backend/models/github.go (1)
20-22: Good addition of fields to support GitLab connectionsThese new fields properly extend the VCSConnection struct to support GitLab alongside the existing GitHub and Bitbucket implementations. The default value for VCSType is set to "bitbucket", which suggests a backward compatibility approach.
backend/controllers/connections.go (3)
41-41: Good update to use dynamic VCS typeReplacing the hardcoded value with
c.VCSTypeproperly returns the actual VCS type from the connection data.
67-68: Fields added for GitLab supportThese fields appropriately extend the request structure to support GitLab connections.
77-78: Expanded VCS type validation to support GitLabThe validation logic now correctly checks for both Bitbucket and GitLab VCS types.
| connection, err := models.DB.CreateVCSConnection(request.Name, models.DiggerVCSType(request.VCS), 0, "", "", "", "", "", "", "", bitbucketAccessTokenEncrypted, bitbucketWebhookSecretEncrypted, gitlabWebhookSecret, gitlabAccessTokenEncrypted, org.ID) | ||
| if err != nil { | ||
| log.Printf("") | ||
| log.Printf("failed to create vcs connection") | ||
| } |
There was a problem hiding this comment.
Missing error handling for CreateVCSConnection
The error handling for CreateVCSConnection is incomplete. A log message is created if an error occurs, but the error isn't returned to the client, and the code continues execution as if the operation succeeded.
Apply this fix:
connection, err := models.DB.CreateVCSConnection(request.Name, models.DiggerVCSType(request.VCS), 0, "", "", "", "", "", "", "", bitbucketAccessTokenEncrypted, bitbucketWebhookSecretEncrypted, gitlabWebhookSecret, gitlabAccessTokenEncrypted, org.ID)
if err != nil {
log.Printf("failed to create vcs connection")
+ c.JSON(http.StatusInternalServerError, gin.H{"error": "Could not create VCS connection"})
+ return
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| connection, err := models.DB.CreateVCSConnection(request.Name, models.DiggerVCSType(request.VCS), 0, "", "", "", "", "", "", "", bitbucketAccessTokenEncrypted, bitbucketWebhookSecretEncrypted, gitlabWebhookSecret, gitlabAccessTokenEncrypted, org.ID) | |
| if err != nil { | |
| log.Printf("") | |
| log.Printf("failed to create vcs connection") | |
| } | |
| connection, err := models.DB.CreateVCSConnection(request.Name, models.DiggerVCSType(request.VCS), 0, "", "", "", "", "", "", "", bitbucketAccessTokenEncrypted, bitbucketWebhookSecretEncrypted, gitlabWebhookSecret, gitlabAccessTokenEncrypted, org.ID) | |
| if err != nil { | |
| log.Printf("failed to create vcs connection") | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Could not create VCS connection"}) | |
| return | |
| } |
| log.Printf("could not encrypt bitbucket access token: %v", err) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Could not encrypt bitbucket access token"}) | ||
| return | ||
| } | ||
|
|
||
| bitbucketWebhookSecretEncrypted, err := utils.AESEncrypt([]byte(secret), request.BitbucketWebhookSecret) | ||
| if err != nil { | ||
| log.Printf("could not encrypt webhook secret: %v", err) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Could not encrypt webhook secret"}) | ||
| log.Printf("could not encrypt bitbucket webhook secret: %v", err) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Could not encrypt bitbucket webhook secret"}) | ||
| return | ||
| } | ||
|
|
||
| gitlabAccessTokenEncrypted, err := utils.AESEncrypt([]byte(secret), request.GitlabAccessToken) | ||
| if err != nil { | ||
| log.Printf("could not encrypt gitlab access token: %v", err) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Could not encrypt gitlab access token"}) | ||
| return | ||
| } | ||
|
|
||
| gitlabWebhookSecret, err := utils.AESEncrypt([]byte(secret), request.GitlabWebhookSecret) | ||
| if err != nil { | ||
| log.Printf("could not encrypt gitlab webhook secret: %v", err) | ||
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Could not encrypt gitlab access token"}) | ||
| return |
There was a problem hiding this comment.
Error message inconsistency in GitLab webhook secret encryption error
There's an inconsistency in the error message for GitLab webhook secret encryption. The error message reports "Could not encrypt gitlab access token" instead of "Could not encrypt gitlab webhook secret".
Apply this fix:
- c.JSON(http.StatusInternalServerError, gin.H{"error": "Could not encrypt gitlab access token"})
+ c.JSON(http.StatusInternalServerError, gin.H{"error": "Could not encrypt gitlab webhook secret"})📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| log.Printf("could not encrypt bitbucket access token: %v", err) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Could not encrypt bitbucket access token"}) | |
| return | |
| } | |
| bitbucketWebhookSecretEncrypted, err := utils.AESEncrypt([]byte(secret), request.BitbucketWebhookSecret) | |
| if err != nil { | |
| log.Printf("could not encrypt webhook secret: %v", err) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Could not encrypt webhook secret"}) | |
| log.Printf("could not encrypt bitbucket webhook secret: %v", err) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Could not encrypt bitbucket webhook secret"}) | |
| return | |
| } | |
| gitlabAccessTokenEncrypted, err := utils.AESEncrypt([]byte(secret), request.GitlabAccessToken) | |
| if err != nil { | |
| log.Printf("could not encrypt gitlab access token: %v", err) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Could not encrypt gitlab access token"}) | |
| return | |
| } | |
| gitlabWebhookSecret, err := utils.AESEncrypt([]byte(secret), request.GitlabWebhookSecret) | |
| if err != nil { | |
| log.Printf("could not encrypt gitlab webhook secret: %v", err) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Could not encrypt gitlab access token"}) | |
| return | |
| log.Printf("could not encrypt bitbucket access token: %v", err) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Could not encrypt bitbucket access token"}) | |
| return | |
| } | |
| bitbucketWebhookSecretEncrypted, err := utils.AESEncrypt([]byte(secret), request.BitbucketWebhookSecret) | |
| if err != nil { | |
| log.Printf("could not encrypt bitbucket webhook secret: %v", err) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Could not encrypt bitbucket webhook secret"}) | |
| return | |
| } | |
| gitlabAccessTokenEncrypted, err := utils.AESEncrypt([]byte(secret), request.GitlabAccessToken) | |
| if err != nil { | |
| log.Printf("could not encrypt gitlab access token: %v", err) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Could not encrypt gitlab access token"}) | |
| return | |
| } | |
| gitlabWebhookSecret, err := utils.AESEncrypt([]byte(secret), request.GitlabWebhookSecret) | |
| if err != nil { | |
| log.Printf("could not encrypt gitlab webhook secret: %v", err) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Could not encrypt gitlab webhook secret"}) | |
| return |
Summary by CodeRabbit
New Features
Chores