Feat bitbucket support #1890
Feat bitbucket support #1890
Conversation
WalkthroughThis pull request reorganizes several components across the codebase. In the backend, import statements are reordered, and API configurations are enhanced by replacing a single middleware with dual authentication layers while adding new VCS connection endpoints. Model and storage functions migrate from GitHub-specific structures to a generalized VCSConnection including Bitbucket credentials. Utility functions, test migrations, and dependency definitions are updated accordingly. Additionally, new Bitbucket API integrations are added in both the CI and EE sections, with corresponding updates in CLI and lib packages. Changes
Sequence Diagram(s)VCS Connection API FlowsequenceDiagram
participant C as Client
participant API as API Server
participant MW as Auth Middleware
participant CTRL as VCS Controller
participant DB as Database
C->>API: Request to /vcs/...
API->>MW: InternalApiAuth check
MW-->>API: Authentication passed
API->>MW: HeadersApiAuth check
MW-->>API: Headers validated
API->>CTRL: Forward request
CTRL->>DB: Query VCS Connection
DB-->>CTRL: Return connection data
CTRL-->>API: Send JSON response
API-->>C: Return response
Bitbucket Webhook & Pipeline FlowsequenceDiagram
participant BB as Bitbucket
participant API as EE API Server
participant CTRL as BitbucketWebhookHandler
participant DB as Database
participant CI as CI Pipeline
BB->>API: POST /bitbucket-webhook with payload
API->>CTRL: Process webhook event
CTRL->>DB: Retrieve & decrypt VCS connection info
CTRL->>CI: Trigger pipeline workflow if applicable
CI-->>CTRL: Pipeline trigger result
CTRL-->>API: Respond with status
API-->>BB: HTTP 200 OK
Possibly related PRs
Poem
✨ Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 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 (
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
There was a problem hiding this comment.
PR Summary
Your free trial has ended. If you'd like to continue receiving code reviews, you can add a payment method here: app.greptile.com/review/github.
💡 (5/5) You can turn off certain types of comments like style here!
42 file(s) reviewed, no comment(s)
Edit PR Review Bot Settings | Greptile
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🔭 Outside diff range comments (1)
backend/models/storage.go (1)
441-462: 🛠️ Refactor suggestionValidate inputs and handle encryption carefully in
CreateVCSConnection.While you store multiple encrypted fields, ensure inputs are properly sanitized or validated. Also, consider whether logs or errors might inadvertently reveal sensitive information.
func (db *Database) CreateVCSConnection( name string, ... ) (*VCSConnection, error) { ... + // Suggest validating non-empty fields and sanitizing input where needed. result := db.GormDB.Save(&app) ... }
🧹 Nitpick comments (20)
ee/drift/utils/github.go (1)
115-115: Function call signature update needs documentation.An empty string parameter has been added to the
CloneGitRepoAndDoActionfunction call. While this appears to be part of the broader changes for supporting Bitbucket, the purpose of this parameter isn't clear from the code context.Consider adding a comment explaining what this parameter represents and why it's being passed as an empty string here.
backend/models/github.go (1)
1-28: Consider renaming the file to match its more generic purposeThe file is still named "github.go" even though it now contains a more generic VCS connection model. Consider renaming it to something like "vcs.go" or "vcs_connection.go" to better reflect the broader functionality it now provides.
ee/cli/pkg/utils/github.go (2)
19-19: Consider adding documentation for the new parameterThe new
tokenUsernameparameter lacks documentation explaining its purpose and expected values for different VCS providers (e.g., what to pass for GitHub vs. Bitbucket).Add a comment explaining that this parameter is typically empty for GitHub authentication but required for Bitbucket, to help future developers understand the correct usage.
1-35: Consider renaming the file to reflect its broader functionalitySimilar to the model file, this utility file is still named "github.go" even though it now contains functionality that works with multiple VCS providers. Consider renaming it to something more generic like "vcs.go" or "git.go" to better represent its purpose.
backend/utils/gitshell.go (1)
130-130: Added blank lineThis is a minor formatting change with no functional impact.
backend/utils/crypt.go (2)
79-92: Consider secure handling of sensitive fields.The newly added fields
BitbucketAccessTokenandBitbucketWebhookSecretstore sensitive tokens in memory. Ensure these strings are managed safely (e.g., minimized retention in memory, possibly cleared when no longer required).
96-103: Validategparameter for nil.You might want to add a quick check to ensure that
gis notnilbefore attempting to decrypt, thereby preventing potential nil-pointer dereferences if the caller passes anilreference.func DecryptConnection(g *models.VCSConnection, key []byte) (*DecryptedVCSConnection, error) { + if g == nil { + return nil, fmt.Errorf("input VCSConnection is nil") + } decrypted := &DecryptedVCSConnection{ ... } ... }libs/ci/bitbucket/bitbucket.go (4)
7-9: Use of “io/ioutil” is deprecated since Go 1.19.You’re importing
io/ioutil, which is deprecated in favor ofiooros. Please consider refactoring to useio.ReadAllinstead ofioutil.ReadAll.🧰 Tools
🪛 golangci-lint (1.62.2)
8-8: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os], and those implementations should be preferred in new code. See the specific function documentation for details.
(staticcheck)
69-69: Debug log statement is fine.The explicit format string
%vis correct. Consider removing this log or lowering its verbosity if it’s only meant for temporary debugging.
130-135: Migrate away fromioutil.ReadAll.Side note: As with your import,
ioutil.ReadAllis deprecated. Replace it withio.ReadAllto follow modern Go conventions.- body, err := ioutil.ReadAll(resp.Body) + body, err := io.ReadAll(resp.Body)
520-599: Large struct forPipelineResponse.All fields are mapped in detail. If only some fields are used, consider a partial struct to reduce maintenance overhead. Otherwise, no immediate issues.
backend/controllers/connections.go (2)
38-44: Dynamic VCS identification instead of hard-coded "bitbucket"
Currently, thevcsin the returned JSON is set to"bitbucket"for every connection, which prevents future extension to other VCS types. Consider mapping the connection to its actualvcstype (e.g.,connection.VCSType) instead of using a hard-coded value."vcs": "bitbucket", --- + "vcs": c.VCSType, // or the relevant field from models.VCSConnection
75-79: Potential extensibility for multiple VCS providers
The checkif request.VCS != "bitbucket"leads to rejection of other types. If you anticipate supporting multiple providers in the future, consider a more flexible design to handle different VCS types or a plugin architecture that can be extended easily.libs/ci/bitbucket/bitbucket_service.go (1)
130-131: Consider supporting additional commands
The code currently supports only a small set of Digger commands (plan, apply, unlock, lock). If you plan to handle other commands (e.g., destroy, revert, custom commands), you may want to expand this list or implement a more generic command resolution system.backend/utils/bitbucket.go (2)
29-45: Remove or clarify commented-out code
The code in lines 29-45 is commented out and might confuse contributors if it’s no longer needed. Consider removing it or adding a comment explaining why it’s left in place for future reference.
21-24: Custom error handling for empty tokens
TheNewClientmethod returns a client even if the token is empty. Consider validating the token first and returning an error if it's empty to avoid unexpected behavior or to ensure strong authentication.+ if token == "" { + return nil, fmt.Errorf("bitbucket token is empty or unset") + } client := bitbucket.NewOAuthbearerToken(token)ee/backend/controllers/bitbucket.go (1)
33-37: Clarify or remove the placeholderTriggerPipelinefunction.The function currently has a placeholder comment but no implementation. Either provide meaningful pipeline triggering logic or remove the function if it’s not needed.
func TriggerPipeline(pipelineType string) error { - // Implement pipeline triggering logic + // TODO: Implement or remove this function to avoid confusion return nil }backend/models/storage.go (3)
464-472: Return a more descriptive error inGetVCSConnectionById.If a record is not found, consider returning a clear, user-friendly error or nil. This helps callers differentiate between “record not found” and actual database errors.
result := db.GormDB.Where("id = ?", id).Find(&app) if result.Error != nil { log.Printf("Failed to find GitHub App for id: %v, error: %v\n", id, result.Error) - return nil, result.Error + if errors.Is(result.Error, gorm.ErrRecordNotFound) { + return nil, fmt.Errorf("VCS connection with id '%s' not found", id) + } return nil, result.Error }
475-483: Rename method or clarify comments.Despite the name
GetVCSConnection, the method specifically searches bygithub_id. Either rename it toGetVCSConnectionByGithubIdor add clarifying comments to minimize confusion.func (db *Database) GetVCSConnection(gitHubAppId any) (*VCSConnection, error) { + // Consider renaming to GetVCSConnectionByGithubId or clarifying doc since we query "github_id = ?" ... }
930-949: Ensure queries maintain principle of least privilege.The JOIN logic in
GetJobsByRepoNameis effectively enumerating jobs across multiple tables. Double-check that the user is permitted to retrieve these records and no sensitive data is leaked.Would you like me to generate a shell script to verify references to these queries or check for potential data exposure?
📜 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!**/*.sumgo.work.sumis excluded by!**/*.sum
📒 Files selected for processing (39)
backend/bootstrap/main.go(2 hunks)backend/controllers/bitbucket.go(0 hunks)backend/controllers/cache.go(1 hunks)backend/controllers/connections.go(1 hunks)backend/controllers/github.go(1 hunks)backend/controllers/github_test.go(1 hunks)backend/go.mod(2 hunks)backend/migrations/20250226185150.sql(1 hunks)backend/models/github.go(1 hunks)backend/models/scheduler.go(1 hunks)backend/models/scheduler_test.go(1 hunks)backend/models/storage.go(5 hunks)backend/models/storage_test.go(1 hunks)backend/queries/queries.go(1 hunks)backend/tasks/runs_test.go(1 hunks)backend/utils/bitbucket.go(1 hunks)backend/utils/crypt.go(2 hunks)backend/utils/github.go(1 hunks)backend/utils/github_test.go(2 hunks)backend/utils/gitlab.go(2 hunks)backend/utils/gitshell.go(4 hunks)cli/cmd/digger/root.go(1 hunks)ee/backend/ci_backends/bitbucket_pipeline.go(1 hunks)ee/backend/controllers/bitbucket.go(1 hunks)ee/backend/controllers/bitbucket_utils.go(1 hunks)ee/backend/controllers/github.go(3 hunks)ee/backend/controllers/gitlab.go(4 hunks)ee/backend/main.go(1 hunks)ee/backend/providers/github/providers.go(2 hunks)ee/cli/cmd/digger/root.go(1 hunks)ee/cli/pkg/policy/policy.go(1 hunks)ee/cli/pkg/utils/github.go(1 hunks)ee/drift/tasks/github.go(1 hunks)ee/drift/utils/github.go(1 hunks)libs/ci/bitbucket/bitbucket.go(7 hunks)libs/ci/bitbucket/bitbucket_service.go(1 hunks)libs/spec/providers.go(2 hunks)next/controllers/github.go(1 hunks)next/services/config.go(1 hunks)
💤 Files with no reviewable changes (1)
- backend/controllers/bitbucket.go
✅ Files skipped from review due to trivial changes (4)
- backend/queries/queries.go
- cli/cmd/digger/root.go
- backend/migrations/20250226185150.sql
- ee/cli/cmd/digger/root.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
libs/ci/bitbucket/bitbucket.go
8-8: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os], and those implementations should be preferred in new code. See the specific function documentation for details.
(staticcheck)
646-646: printf: non-constant format string in call to log.Printf
(govet)
ee/backend/controllers/bitbucket.go
120-120: Error return value is not checked
(errcheck)
268-268: Error return value of fmt.Errorf is not checked
(errcheck)
260-260: ineffectual assignment to err
(ineffassign)
⏰ Context from checks skipped due to timeout of 90000ms (3)
- GitHub Check: Build
- GitHub Check: Build
- GitHub Check: Build
🔇 Additional comments (49)
backend/models/scheduler.go (1)
23-23: Adding Bitbucket VCS type looks good!The constant is properly defined following the same pattern as existing VCS types, supporting the PR's goal of adding Bitbucket integration.
backend/models/storage_test.go (1)
39-39: Migration updated for generalized VCS connections.Great job replacing GitHub-specific model with the more general
VCSConnectionmodel in the database migration. This change supports the goal of handling multiple VCS providers in a consistent way.backend/tasks/runs_test.go (1)
53-53: Model migration updated to support multi-VCS systems.The change from
&models.GithubAppConnection{}to&models.VCSConnection{}in the database migration aligns with the PR's objective to add Bitbucket support. This generalization allows the system to handle multiple VCS providers using a unified model.ee/backend/providers/github/providers.go (2)
47-47: Method call updated to use generalized VCS connection model.The change from
GetGithubAppConnectiontoGetVCSConnectionsupports the multi-VCS system architecture being implemented in this PR.
111-111: Method call updated for VCS generalization.Similar to the previous change, this update maintains consistency across the codebase by using the generalized VCS connection model.
backend/controllers/cache.go (1)
70-70:✅ Verification successful
Added parameter to support multi-VCS authentication.
The addition of an empty string parameter to
CloneGitRepoAndDoActionappears to be for supporting Bitbucket authentication alongside GitHub. Since this is a GitHub-specific call, the parameter is empty.
🏁 Script executed:
#!/bin/bash # Check if CloneGitRepoAndDoAction has been updated consistently across the codebase # Look for the function definition to understand the new parameter echo "Looking for CloneGitRepoAndDoAction function definition:" rg -A 3 "func CloneGitRepoAndDoAction" --type goLength of output: 881
Review Approved: Multi-VCS Commit Hash Parameter Added
After verifying the function definitions:
- In backend/utils/github.go, the updated signature now accepts a commit hash parameter (as the third argument), which aligns with the call in backend/controllers/cache.go passing an empty string.
- This extra parameter is indeed intended to facilitate multi-VCS (e.g. Bitbucket) authentication while remaining a no-op for GitHub-specific calls.
No further changes are necessary here.
next/services/config.go (1)
42-42:❓ Verification inconclusive
Added parameter for multi-VCS compatibility.
Similar to changes in other files, an empty string parameter has been added to support the new function signature that accommodates multiple VCS systems.
🏁 Script executed:
#!/bin/bash # Check if the new parameter is being used with non-empty values for Bitbucket connections # Search for non-empty string parameters to CloneGitRepoAndDoAction echo "Checking for non-empty username parameters to CloneGitRepoAndDoAction:" rg "CloneGitRepoAndDoAction\(.*,.*,.*,.*,[^\"\'].*\)" --type go # Look for Bitbucket-related code that might use this parameter echo "Looking for Bitbucket-related code using this parameter:" rg "CloneGitRepoAndDoAction.*bitbucket" -A 3 --type goLength of output: 552
#!/bin/bash echo "Checking for non-empty username parameters passed as the 5th argument to CloneGitRepoAndDoAction:" # This regex looks for a call with four comma-separated arguments followed by a non-empty string literal (i.e. not ""). rg -N 'CloneGitRepoAndDoAction\([^,]+,[^,]+,[^,]+,[^,]+,"[^"]+"' --type go echo "Searching for Bitbucket-related usage invoking CloneGitRepoAndDoAction:" rg -N 'CloneGitRepoAndDoAction.*bitbucket' --type goPlease run the above script to manually verify whether any invocations of
CloneGitRepoAndDoActionsupply a non-empty 5th parameter (which might indicate Bitbucket-specific logic) or if the empty string is consistently used for multi-VCS compatibility. This verification is essential because the previous attempt encountered a regex parsing error that prevented conclusive results.backend/controllers/github.go (1)
598-598: New parameter added to CloneGitRepoAndDoAction function.The function call now includes an additional empty string parameter to support the new function signature. This change is part of the Bitbucket integration effort, allowing for username authentication which is needed for some VCS systems.
backend/utils/github_test.go (4)
17-17: Test updated to match new function signature.The CloneGitRepoAndDoAction function signature has been updated to include an additional parameter for token username to support Bitbucket integration. The test correctly passes an empty string as this parameter.
24-24: Test updated with new parameter.This test case has been properly updated to match the new CloneGitRepoAndDoAction function signature with an empty string for the tokenUsername parameter.
35-35: Private repo test updated to match new signature.The test for private repositories has been correctly updated with the new parameter structure.
42-42: Invalid branch test updated with new parameter.This test has been properly updated to include the empty string parameter needed for the new function signature.
ee/backend/main.go (2)
47-47: Added BitbucketProvider to DiggerEEController.A BitbucketProvider has been added to the DiggerEEController, enabling Bitbucket integration within the enterprise edition controller.
53-53:✅ Verification successful
Added Bitbucket webhook handler route.
A new route has been added to handle Bitbucket webhook events. This complements the existing GitHub and GitLab webhook handlers, providing complete VCS integration.
🏁 Script executed:
#!/bin/bash # Check if the BitbucketWebhookHandler is defined in the controller rg "func.*BitbucketWebhookHandler" --type goLength of output: 153
Verified Bitbucket Webhook Integration:
- The new POST route in
ee/backend/main.go(line 53) correctly maps toeeController.BitbucketWebhookHandler.- Verification confirms that
BitbucketWebhookHandleris defined inee/backend/controllers/bitbucket.go.- This integration complements the existing GitHub and GitLab webhook handlers as intended.
backend/go.mod (2)
13-13: New dependency added: github.com/davecgh/go-spewThe
go-spewlibrary is being added, which is commonly used for debugging by providing deep pretty printing of Go data structures. This is likely to help with debugging the new Bitbucket integration.
24-24: New dependency added: github.com/ktrysmt/go-bitbucketThis is a key dependency for the Bitbucket support feature. The
go-bitbucketpackage provides client functionality for interacting with the Bitbucket API, which aligns perfectly with the PR objective of adding Bitbucket support.backend/controllers/github_test.go (1)
592-592: Model migration updated for VCS generalizationThe AutoMigrate call has been updated to use
&models.VCSConnection{}instead of the previous GitHub-specific model. This change is consistent with the transition from GitHub-specific connections to a more generalized VCS connection model supporting multiple providers.backend/models/github.go (2)
7-22: Renamed model from GithubAppConnection to VCSConnectionThe model has been renamed to make it provider-agnostic, with additional fields added for Bitbucket credentials. This is a good architectural change that supports multiple VCS providers while maintaining backward compatibility with existing GitHub functionality.
24-28: TODO for table migrationThere's a noted TODO about creating a migration to rename the database table from "github_app_connections" to "vcs_connections". Consider creating this migration as a follow-up task to complete the transition to a fully provider-agnostic model.
Is there a reason the table migration wasn't included in this PR? This creates a slight mismatch between the code model name and the database table name.
ee/cli/pkg/utils/github.go (1)
19-21: Function signature updated to support different VCS providersThe
CloneGitRepoAndDoActionfunction has been modified to include atokenUsernameparameter and the call toNewGitShellWithTokenAuthhas been updated accordingly. This change enables support for Bitbucket's authentication, which requires a username along with the token.ee/cli/pkg/policy/policy.go (1)
65-65: Function signature adaptation looks good.The code has been updated to accommodate the new signature of
CloneGitRepoAndDoActionwhich now requires atokenUsernameparameter. Passing an empty string is appropriate here if no specific username is needed for this context.libs/spec/providers.go (1)
14-14: Bitbucket import added correctly.The import for the Bitbucket package has been properly added to support the new Bitbucket integration.
ee/backend/ci_backends/bitbucket_pipeline.go (2)
10-15: BitbucketPipelineCI struct is well-defined.The struct contains all necessary fields for interacting with Bitbucket pipelines, including the client, repository details, and branch.
17-39: TriggerWorkflow implementation is thorough.The method correctly:
- Serializes the spec to JSON
- Constructs the necessary pipeline variables
- Triggers the pipeline with proper error handling
Good practices observed:
- Error is properly wrapped with context
- Variables are well-structured for Bitbucket pipeline requirements
backend/utils/github.go (2)
31-31: Function signature expansion accommodates multi-VCS support.The addition of the
tokenUsernameparameter toCloneGitRepoAndDoActionenhances flexibility for different VCS providers that might require username authentication alongside tokens.
33-33: Updated authentication handling.The call to
NewGitShellWithTokenAuthhas been correctly updated to pass the newtokenUsernameparameter, ensuring consistent authentication across the codebase.backend/utils/gitlab.go (2)
49-49: Function renamed to be more specific to GitLabThe function has been renamed from
GetDiggerConfigForBranchtoGetDiggerConfigForBranchGitlab, making it explicitly GitLab-specific. This is likely part of the Bitbucket support refactoring to create provider-specific implementations.
65-65:✅ Verification successful
Parameter added to CloneGitRepoAndDoAction
An empty string parameter has been added to the
CloneGitRepoAndDoActioncall. This aligns with changes to the function signature to support multiple VCS providers.
🏁 Script executed:
#!/bin/bash # Check how the CloneGitRepoAndDoAction function is called in other files rg "CloneGitRepoAndDoAction\(" --type goLength of output: 2146
Parameter addition verification for CloneGitRepoAndDoAction
The empty string parameter in the call within
backend/utils/gitlab.gois consistent with how the function is invoked in other parts of the codebase (e.g., inbackend/controllers/cache.go,backend/utils/bitbucket.go, and the tests inbackend/utils/github_test.go). This new parameter accommodates the updated function signature that supports multiple VCS providers. No further changes are needed.backend/utils/gitshell.go (3)
15-19: Added TokenUsername field for Bitbucket supportA new
TokenUsernamefield has been added to theGitAuthstruct specifically to support Bitbucket's authentication which usesx-token-authinstead of GitHub'sx-access-token.
45-51: Updated constructor to support Bitbucket authenticationThe
NewGitShellWithTokenAuthfunction now accepts atokenUsernameparameter to support different authentication mechanisms for various VCS providers.
69-74: Enhanced authentication URL formatting for BitbucketThe logic now conditionally uses either the provided
TokenUsernameor defaults tox-access-token, allowing for compatibility with both GitHub and Bitbucket authentication mechanisms.next/controllers/github.go (1)
541-541: Updated CloneGitRepoAndDoAction call with new parameterAn empty string parameter has been added to the
CloneGitRepoAndDoActioncall to align with the updated function signature. This change is consistent with the implementation of Bitbucket support across the codebase.ee/backend/controllers/gitlab.go (4)
33-33: Added BitbucketProvider fieldThe
BitbucketProviderfield has been added to theDiggerEEControllerstruct to enable Bitbucket integration. This is a key part of the Bitbucket support feature.
160-160: Updated CloneGitRepoAndDoAction call with new parameterAn empty string parameter has been added to the
CloneGitRepoAndDoActioncall to align with the updated function signature, consistent with changes in other files.
222-222: Using GitLab-specific configuration functionChanged from
GetDiggerConfigForBranchtoGetDiggerConfigForBranchGitlab, aligning with the renaming that separates provider-specific implementations.
411-411: Using GitLab-specific configuration functionChanged from
GetDiggerConfigForBranchtoGetDiggerConfigForBranchGitlab, maintaining consistency with the provider-specific implementation approach.backend/utils/crypt.go (1)
94-94: Function signature looks good.Accepting a
*models.VCSConnectionand returning a pointer to the decrypted struct aligns with the broader transition to a general VCS connection. The approach is consistent and clarifies the scope of data being decrypted.ee/backend/controllers/github.go (3)
106-106: Reviewed broader connection type usage.Replacing
[]models.GithubAppConnectionwith[]models.VCSConnectionproperly aligns the data model with the new, generalized structure.
195-195: Ensure organization ID and extra params match new create logic.The updated call to
CreateVCSConnectionincludes new parameters. Verify upstream calls and user input to confirm that these empty-string placeholders (for Bitbucket) safely apply.
218-218: Renaming tomodels.VCSConnectionis consistent.Deletion logic looks straightforward. Just confirm that the record’s new Bitbucket-related fields do not require any special cleanup.
backend/bootstrap/main.go (3)
15-17: No issues spotted with import changes.These reordered imports do not raise concerns, and they appear consistent with typical Golang standards.
219-219: Layered authentication for API endpoints.Using both
middleware.InternalApiAuth()andmiddleware.HeadersApiAuth()increases security coverage. Make sure to confirm these middlewares don’t conflict or introduce redundancy.
229-233: New VCS endpoints look good.This consolidated group for VCS connections is coherent and follows REST best practices (
GET,POST,DELETE). Please confirm that proper input validation and request body checks are included in the respective controller methods.libs/ci/bitbucket/bitbucket.go (3)
14-15: Imports for CI and configuration packages.No issues: referencing
github.com/diggerhq/digger/libs/ciandconfiguration "github.com/diggerhq/digger/libs/digger_config"is consistent with your code’s structure.
136-142: JSON unmarshaling approach is valid.Reading the response into
commentResponseis fine, and the error handling is clear.
143-148: Converting the Bitbucket comment to a generic comment structure.Mapping fields looks correct. The
ci.Commentfields match the Bitbucket data well.backend/controllers/connections.go (1)
22-28: Ensure consistent error responses for organization not found
When the query fails to retrieve the organization record, the code logs an error and returns a 404 status. This is appropriate if the record truly doesn't exist. However, if there is a database connectivity error or another upstream failure, consider returning a 500 (Internal Server Error) to distinguish between "not found" vs. "database error."libs/ci/bitbucket/bitbucket_service.go (1)
50-55: Add safeguard for missing Pull Request ID
If the environment variable for the pull request ID is not set, an error is returned. Ensure that your CI environment reliably populates this variable. It might also help to log the environment variable name or provide more diagnostic details for easier debugging.ee/backend/controllers/bitbucket_utils.go (1)
1-209: Struct definitions look generally well-organized.The new data structures offer a robust schema for Bitbucket events. Value objects are clear, using
omitemptyin non-essential fields. This is a solid foundation for further integration.
| @@ -42,7 +42,7 @@ func LoadProjectsFromGithubRepo(gh utils2.GithubClientProvider, installationId s | |||
| return fmt.Errorf("error getting github service") | |||
| } | |||
|
|
|||
| err = utils3.CloneGitRepoAndDoAction(cloneUrl, branch, "", *token, func(dir string) error { | |||
| err = utils3.CloneGitRepoAndDoAction(cloneUrl, branch, "", *token, "", func(dir string) error { | |||
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Function call updated for consistent VCS handling.
The function call has been modified consistently with other similar calls in the codebase, adding an empty string parameter which is likely related to supporting multiple VCS types.
Could you verify that all calls to CloneGitRepoAndDoAction across the codebase have been updated to match the new signature? This would help ensure there are no missed instances that could cause runtime errors.
🏁 Script executed:
#!/bin/bash
# Find all calls to CloneGitRepoAndDoAction to ensure consistent parameter usage
rg "CloneGitRepoAndDoAction\(" --type goLength of output: 2146
Inconsistent Parameter Usage for CloneGitRepoAndDoAction
The updated call in ee/drift/tasks/github.go now correctly uses the new 6-parameter signature (including an explicit empty string for the commit hash and token username). However, the following instances were detected with only 5 parameters, which may lead to runtime inconsistencies with the intended VCS handling:
-
ee/cli/pkg/policy/policy.go
Uses:err := utils.CloneGitRepoAndDoAction(p.ManagementRepoUrl, "main", p.GitToken, "", func(basePath string) error {
Expected update: Add an empty string for the commit hash as the third parameter.
-
ee/drift/utils/github.go
Uses:err = utils2.CloneGitRepoAndDoAction(cloneUrl, branch, "", *token, func(dir string) error {
Expected update: Include the missing token username parameter (likely as an empty string) to align with the new signature.
Please update these calls (if they are not meant to use an alternative overload) to ensure consistent 6-parameter usage across the codebase for uniform VCS handling.
| err = gdb.AutoMigrate(&Policy{}, &Organisation{}, &Repo{}, &Project{}, &Token{}, | ||
| &User{}, &ProjectRun{}, &GithubAppInstallation{}, &GithubAppConnection{}, &GithubAppInstallationLink{}, | ||
| &User{}, &ProjectRun{}, &GithubAppInstallation{}, &VCSConnection{}, &GithubAppInstallationLink{}, | ||
| &GithubDiggerJobLink{}, &DiggerJob{}, &DiggerJobParentLink{}) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Model migration updated to use VCSConnection instead of GithubAppConnection.
The database migration has been modified to use the generalized VCSConnection model instead of the GitHub-specific GithubAppConnection. This is a good architectural change that supports the multi-VCS provider approach. Ensure all related code that previously used GithubAppConnection has been updated accordingly.
🏁 Script executed:
#!/bin/bash
# Check for any remaining references to GithubAppConnection that might have been missed
rg "GithubAppConnection" --type goLength of output: 744
Attention: Update All References from GithubAppConnection to VCSConnection
The migration code in the test file has been correctly updated. However, our search confirms that several parts of the codebase still reference “GithubAppConnection” (or use similar naming in function names and endpoints). Please review and update the following:
- File:
ee/backend/controllers/github.go- Functions:
GithubAppConnections(),GithubAppConnectionsConfirm(), andGithubAppConnectionsDelete()
- Functions:
- File:
ee/backend/main.go- Endpoints mapping to the above controller functions
Ensure these are either refactored to use the generalized VCSConnection nomenclature or validated if they are intentionally retained for legacy compatibility. This alignment is important to support the multi-VCS provider approach throughout the codebase.
| case "bitbucket": | ||
| token := os.Getenv("DIGGER_BITBUCKET_ACCESS_TOKEN") | ||
| if token == "" { | ||
| return nil, fmt.Errorf("failed to get bitbucket service: GITLAB_TOKEN not specified") | ||
| } | ||
| return bitbucket.BitbucketAPI{ | ||
| AuthToken: token, | ||
| HttpClient: http.Client{}, | ||
| RepoWorkspace: vcsSpec.RepoOwner, | ||
| RepoName: vcsSpec.RepoName, | ||
| }, nil |
There was a problem hiding this comment.
Fix error message to reference the correct environment variable.
The error message mentions "GITLAB_TOKEN" when it should reference "DIGGER_BITBUCKET_ACCESS_TOKEN" to match the environment variable being checked on line 200.
- return nil, fmt.Errorf("failed to get bitbucket service: GITLAB_TOKEN not specified")
+ return nil, fmt.Errorf("failed to get bitbucket service: DIGGER_BITBUCKET_ACCESS_TOKEN not specified")📝 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.
| case "bitbucket": | |
| token := os.Getenv("DIGGER_BITBUCKET_ACCESS_TOKEN") | |
| if token == "" { | |
| return nil, fmt.Errorf("failed to get bitbucket service: GITLAB_TOKEN not specified") | |
| } | |
| return bitbucket.BitbucketAPI{ | |
| AuthToken: token, | |
| HttpClient: http.Client{}, | |
| RepoWorkspace: vcsSpec.RepoOwner, | |
| RepoName: vcsSpec.RepoName, | |
| }, nil | |
| case "bitbucket": | |
| token := os.Getenv("DIGGER_BITBUCKET_ACCESS_TOKEN") | |
| if token == "" { | |
| return nil, fmt.Errorf("failed to get bitbucket service: DIGGER_BITBUCKET_ACCESS_TOKEN not specified") | |
| } | |
| return bitbucket.BitbucketAPI{ | |
| AuthToken: token, | |
| HttpClient: http.Client{}, | |
| RepoWorkspace: vcsSpec.RepoOwner, | |
| RepoName: vcsSpec.RepoName, | |
| }, nil |
| if g.BitbucketAccessTokenEncrypted != "" { | ||
| bitbucketAccessToken, err := AESDecrypt(key, g.BitbucketAccessTokenEncrypted) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to decrypt private key base64: %w", err) | ||
| } | ||
| decrypted.BitbucketAccessToken = bitbucketAccessToken | ||
| } |
There was a problem hiding this comment.
Fix incorrect error message when decrypting Bitbucket access token.
Currently, the error message says "failed to decrypt private key base64" even though it’s decrypting BitbucketAccessTokenEncrypted.
- return nil, fmt.Errorf("failed to decrypt private key base64: %w", err)
+ return nil, fmt.Errorf("failed to decrypt Bitbucket access token: %w", err)📝 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.
| if g.BitbucketAccessTokenEncrypted != "" { | |
| bitbucketAccessToken, err := AESDecrypt(key, g.BitbucketAccessTokenEncrypted) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to decrypt private key base64: %w", err) | |
| } | |
| decrypted.BitbucketAccessToken = bitbucketAccessToken | |
| } | |
| if g.BitbucketAccessTokenEncrypted != "" { | |
| bitbucketAccessToken, err := AESDecrypt(key, g.BitbucketAccessTokenEncrypted) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to decrypt Bitbucket access token: %w", err) | |
| } | |
| decrypted.BitbucketAccessToken = bitbucketAccessToken | |
| } |
| if g.BitbucketWebhookSecretEncrypted != "" { | ||
| bitbucketWebhookSecret, err := AESDecrypt(key, g.BitbucketWebhookSecretEncrypted) | ||
| if err != nil { | ||
| return nil, fmt.Errorf("failed to decrypt private key base64: %w", err) | ||
| } | ||
| decrypted.BitbucketWebhookSecret = bitbucketWebhookSecret | ||
| } |
There was a problem hiding this comment.
Fix incorrect error message when decrypting Bitbucket webhook secret.
Similarly, this block also displays "failed to decrypt private key base64" instead of referencing the Bitbucket webhook secret.
- return nil, fmt.Errorf("failed to decrypt private key base64: %w", err)
+ return nil, fmt.Errorf("failed to decrypt Bitbucket webhook secret: %w", err)📝 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.
| if g.BitbucketWebhookSecretEncrypted != "" { | |
| bitbucketWebhookSecret, err := AESDecrypt(key, g.BitbucketWebhookSecretEncrypted) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to decrypt private key base64: %w", err) | |
| } | |
| decrypted.BitbucketWebhookSecret = bitbucketWebhookSecret | |
| } | |
| if g.BitbucketWebhookSecretEncrypted != "" { | |
| bitbucketWebhookSecret, err := AESDecrypt(key, g.BitbucketWebhookSecretEncrypted) | |
| if err != nil { | |
| return nil, fmt.Errorf("failed to decrypt Bitbucket webhook secret: %w", err) | |
| } | |
| decrypted.BitbucketWebhookSecret = bitbucketWebhookSecret | |
| } |
| log.Printf("") | ||
| } | ||
|
|
There was a problem hiding this comment.
Proper error handling for connection creation failures
If an error occurs while creating the VCS connection, log.Printf("") is empty and the code does not return a response. This silently fails and leaves the client hanging. Ensure you return a relevant error or message to the client.
if err != nil {
- log.Printf("")
+ log.Printf("Failed to create VCS connection: %v", err)
+ c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to 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.
| log.Printf("") | |
| } | |
| if err != nil { | |
| log.Printf("Failed to create VCS connection: %v", err) | |
| c.JSON(http.StatusInternalServerError, gin.H{"error": "Failed to create VCS connection"}) | |
| return | |
| } |
ee/backend/controllers/bitbucket.go
Outdated
| log.Printf("error parsing pullrequest:comment_created event: %v", err) | ||
| log.Printf("error parsing pullrequest:comment_created event: %v", err) | ||
| } | ||
| go handleIssueCommentEventBB(ee.BitbucketProvider, &pullRequestCommentCreated, ee.CiBackendProvider, orgId, bitbucketAccessToken) |
There was a problem hiding this comment.
Capture or handle the error returned by handleIssueCommentEventBB.
When calling the function in a goroutine, its error return value is lost. Consider logging or otherwise handling this error inside the goroutine to prevent silent failures.
-go handleIssueCommentEventBB(ee.BitbucketProvider, &pullRequestCommentCreated, ee.CiBackendProvider, orgId, bitbucketAccessToken)
+go func() {
+ if err := handleIssueCommentEventBB(ee.BitbucketProvider, &pullRequestCommentCreated, ee.CiBackendProvider, orgId, bitbucketAccessToken); err != nil {
+ log.Printf("handleIssueCommentEventBB returned error: %v", err)
+ }
+}()📝 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.
| go handleIssueCommentEventBB(ee.BitbucketProvider, &pullRequestCommentCreated, ee.CiBackendProvider, orgId, bitbucketAccessToken) | |
| go func() { | |
| if err := handleIssueCommentEventBB(ee.BitbucketProvider, &pullRequestCommentCreated, ee.CiBackendProvider, orgId, bitbucketAccessToken); err != nil { | |
| log.Printf("handleIssueCommentEventBB returned error: %v", err) | |
| } | |
| }() |
🧰 Tools
🪛 golangci-lint (1.62.2)
120-120: Error return value is not checked
(errcheck)
ee/backend/controllers/bitbucket.go
Outdated
| func (ee DiggerEEController) BitbucketWebhookHandler(c *gin.Context) { | ||
| eventKey := c.GetHeader("X-Event-Key") | ||
| if eventKey == "" { | ||
| log.Printf("unknown event") | ||
| return | ||
| } | ||
|
|
||
| connectionId := c.GetHeader("DIGGER_CONNECTION_ID") | ||
| connectionEncrypted, err := models.DB.GetVCSConnectionById(connectionId) | ||
| if err != nil { | ||
| log.Printf("failed to fetch connection: %v", err) | ||
| c.String(http.StatusInternalServerError, "error while processing connection") | ||
| return | ||
| } | ||
|
|
||
| secret := os.Getenv("DIGGER_ENCRYPTION_SECRET") | ||
| if secret == "" { | ||
| log.Printf("ERROR: no encryption secret specified, please specify DIGGER_ENCRYPTION_SECRET as 32 bytes base64 string") | ||
| c.String(http.StatusInternalServerError, "secret not specified") | ||
| return | ||
| } | ||
| connectionDecrypted, err := utils.DecryptConnection(connectionEncrypted, []byte(secret)) | ||
| if err != nil { | ||
| log.Printf("ERROR: could not perform decryption: %v", err) | ||
| c.String(http.StatusInternalServerError, "unexpected error while fetching connection") | ||
| return | ||
| } | ||
| bitbucketAccessToken := connectionDecrypted.BitbucketAccessToken | ||
|
|
||
| orgId := connectionDecrypted.OrganisationID | ||
| bitbucketWebhookSecret := connectionDecrypted.BitbucketWebhookSecret | ||
|
|
||
| if bitbucketWebhookSecret == "" { | ||
| log.Printf("ERROR: no encryption secret specified, please specify DIGGER_ENCRYPTION_SECRET as 32 bytes base64 string") | ||
| c.String(http.StatusInternalServerError, "unexpected error while fetching connection") | ||
| return | ||
| } | ||
|
|
||
| var pullRequestCommentCreated = BitbucketCommentCreatedEvent{} | ||
| var repoPush = BitbucketPushEvent{} | ||
| var pullRequestCreated = BitbucketPullRequestCreatedEvent{} | ||
|
|
||
| bodyBytes, err := io.ReadAll(c.Request.Body) | ||
| if err != nil { | ||
| c.String(http.StatusBadRequest, "Error reading request body", err) | ||
| return | ||
| } | ||
|
|
||
| verifySignature(c, bodyBytes, bitbucketWebhookSecret) | ||
|
|
||
| switch eventKey { | ||
| case "pullrequest:comment_created": | ||
| err := json.Unmarshal(bodyBytes, &pullRequestCommentCreated) | ||
| if err != nil { | ||
| log.Printf("error parsing pullrequest:comment_created event: %v", err) | ||
| log.Printf("error parsing pullrequest:comment_created event: %v", err) | ||
| } | ||
| go handleIssueCommentEventBB(ee.BitbucketProvider, &pullRequestCommentCreated, ee.CiBackendProvider, orgId, bitbucketAccessToken) | ||
| case "pullrequest:created": | ||
| err := json.Unmarshal(bodyBytes, &pullRequestCreated) | ||
| if err != nil { | ||
| log.Printf("error parsing pullrequest:created event: %v", err) | ||
| } | ||
| log.Printf("pullrequest:created") | ||
| case "repo:push": | ||
| err := json.Unmarshal(bodyBytes, &repoPush) | ||
| if err != nil { | ||
| log.Printf("error parsing repo:push event: %v", err) | ||
| } | ||
| log.Printf("repo:push") | ||
| default: | ||
| log.Printf("unknown event key: %s", eventKey) | ||
| return | ||
| } | ||
|
|
||
| c.String(http.StatusAccepted, "ok") | ||
| } |
There was a problem hiding this comment.
Use the result of verifySignature for security checks.
Line 111 calls verifySignature but discards its return value, leaving the request unverified. This constitutes a security risk, as malicious requests could proceed unimpeded.
-verifySignature(c, bodyBytes, bitbucketWebhookSecret)
+isValidSig := verifySignature(c, bodyBytes, bitbucketWebhookSecret)
+if !isValidSig {
+ log.Printf("ERROR: invalid Bitbucket webhook signature")
+ c.String(http.StatusUnauthorized, "invalid signature")
+ 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.
| func (ee DiggerEEController) BitbucketWebhookHandler(c *gin.Context) { | |
| eventKey := c.GetHeader("X-Event-Key") | |
| if eventKey == "" { | |
| log.Printf("unknown event") | |
| return | |
| } | |
| connectionId := c.GetHeader("DIGGER_CONNECTION_ID") | |
| connectionEncrypted, err := models.DB.GetVCSConnectionById(connectionId) | |
| if err != nil { | |
| log.Printf("failed to fetch connection: %v", err) | |
| c.String(http.StatusInternalServerError, "error while processing connection") | |
| return | |
| } | |
| secret := os.Getenv("DIGGER_ENCRYPTION_SECRET") | |
| if secret == "" { | |
| log.Printf("ERROR: no encryption secret specified, please specify DIGGER_ENCRYPTION_SECRET as 32 bytes base64 string") | |
| c.String(http.StatusInternalServerError, "secret not specified") | |
| return | |
| } | |
| connectionDecrypted, err := utils.DecryptConnection(connectionEncrypted, []byte(secret)) | |
| if err != nil { | |
| log.Printf("ERROR: could not perform decryption: %v", err) | |
| c.String(http.StatusInternalServerError, "unexpected error while fetching connection") | |
| return | |
| } | |
| bitbucketAccessToken := connectionDecrypted.BitbucketAccessToken | |
| orgId := connectionDecrypted.OrganisationID | |
| bitbucketWebhookSecret := connectionDecrypted.BitbucketWebhookSecret | |
| if bitbucketWebhookSecret == "" { | |
| log.Printf("ERROR: no encryption secret specified, please specify DIGGER_ENCRYPTION_SECRET as 32 bytes base64 string") | |
| c.String(http.StatusInternalServerError, "unexpected error while fetching connection") | |
| return | |
| } | |
| var pullRequestCommentCreated = BitbucketCommentCreatedEvent{} | |
| var repoPush = BitbucketPushEvent{} | |
| var pullRequestCreated = BitbucketPullRequestCreatedEvent{} | |
| bodyBytes, err := io.ReadAll(c.Request.Body) | |
| if err != nil { | |
| c.String(http.StatusBadRequest, "Error reading request body", err) | |
| return | |
| } | |
| verifySignature(c, bodyBytes, bitbucketWebhookSecret) | |
| switch eventKey { | |
| case "pullrequest:comment_created": | |
| err := json.Unmarshal(bodyBytes, &pullRequestCommentCreated) | |
| if err != nil { | |
| log.Printf("error parsing pullrequest:comment_created event: %v", err) | |
| log.Printf("error parsing pullrequest:comment_created event: %v", err) | |
| } | |
| go handleIssueCommentEventBB(ee.BitbucketProvider, &pullRequestCommentCreated, ee.CiBackendProvider, orgId, bitbucketAccessToken) | |
| case "pullrequest:created": | |
| err := json.Unmarshal(bodyBytes, &pullRequestCreated) | |
| if err != nil { | |
| log.Printf("error parsing pullrequest:created event: %v", err) | |
| } | |
| log.Printf("pullrequest:created") | |
| case "repo:push": | |
| err := json.Unmarshal(bodyBytes, &repoPush) | |
| if err != nil { | |
| log.Printf("error parsing repo:push event: %v", err) | |
| } | |
| log.Printf("repo:push") | |
| default: | |
| log.Printf("unknown event key: %s", eventKey) | |
| return | |
| } | |
| c.String(http.StatusAccepted, "ok") | |
| } | |
| func (ee DiggerEEController) BitbucketWebhookHandler(c *gin.Context) { | |
| eventKey := c.GetHeader("X-Event-Key") | |
| if eventKey == "" { | |
| log.Printf("unknown event") | |
| return | |
| } | |
| connectionId := c.GetHeader("DIGGER_CONNECTION_ID") | |
| connectionEncrypted, err := models.DB.GetVCSConnectionById(connectionId) | |
| if err != nil { | |
| log.Printf("failed to fetch connection: %v", err) | |
| c.String(http.StatusInternalServerError, "error while processing connection") | |
| return | |
| } | |
| secret := os.Getenv("DIGGER_ENCRYPTION_SECRET") | |
| if secret == "" { | |
| log.Printf("ERROR: no encryption secret specified, please specify DIGGER_ENCRYPTION_SECRET as 32 bytes base64 string") | |
| c.String(http.StatusInternalServerError, "secret not specified") | |
| return | |
| } | |
| connectionDecrypted, err := utils.DecryptConnection(connectionEncrypted, []byte(secret)) | |
| if err != nil { | |
| log.Printf("ERROR: could not perform decryption: %v", err) | |
| c.String(http.StatusInternalServerError, "unexpected error while fetching connection") | |
| return | |
| } | |
| bitbucketAccessToken := connectionDecrypted.BitbucketAccessToken | |
| orgId := connectionDecrypted.OrganisationID | |
| bitbucketWebhookSecret := connectionDecrypted.BitbucketWebhookSecret | |
| if bitbucketWebhookSecret == "" { | |
| log.Printf("ERROR: no encryption secret specified, please specify DIGGER_ENCRYPTION_SECRET as 32 bytes base64 string") | |
| c.String(http.StatusInternalServerError, "unexpected error while fetching connection") | |
| return | |
| } | |
| var pullRequestCommentCreated = BitbucketCommentCreatedEvent{} | |
| var repoPush = BitbucketPushEvent{} | |
| var pullRequestCreated = BitbucketPullRequestCreatedEvent{} | |
| bodyBytes, err := io.ReadAll(c.Request.Body) | |
| if err != nil { | |
| c.String(http.StatusBadRequest, "Error reading request body", err) | |
| return | |
| } | |
| isValidSig := verifySignature(c, bodyBytes, bitbucketWebhookSecret) | |
| if !isValidSig { | |
| log.Printf("ERROR: invalid Bitbucket webhook signature") | |
| c.String(http.StatusUnauthorized, "invalid signature") | |
| return | |
| } | |
| switch eventKey { | |
| case "pullrequest:comment_created": | |
| err := json.Unmarshal(bodyBytes, &pullRequestCommentCreated) | |
| if err != nil { | |
| log.Printf("error parsing pullrequest:comment_created event: %v", err) | |
| log.Printf("error parsing pullrequest:comment_created event: %v", err) | |
| } | |
| go handleIssueCommentEventBB(ee.BitbucketProvider, &pullRequestCommentCreated, ee.CiBackendProvider, orgId, bitbucketAccessToken) | |
| case "pullrequest:created": | |
| err := json.Unmarshal(bodyBytes, &pullRequestCreated) | |
| if err != nil { | |
| log.Printf("error parsing pullrequest:created event: %v", err) | |
| } | |
| log.Printf("pullrequest:created") | |
| case "repo:push": | |
| err := json.Unmarshal(bodyBytes, &repoPush) | |
| if err != nil { | |
| log.Printf("error parsing repo:push event: %v", err) | |
| } | |
| log.Printf("repo:push") | |
| default: | |
| log.Printf("unknown event key: %s", eventKey) | |
| return | |
| } | |
| c.String(http.StatusAccepted, "ok") | |
| } |
🧰 Tools
🪛 golangci-lint (1.62.2)
120-120: Error return value is not checked
(errcheck)
| // verifySignature verifies the X-Hub-Signature header | ||
| func verifySignature(c *gin.Context, body []byte, webhookSecret string) bool { | ||
| // Get the signature from the header | ||
| signature := c.GetHeader("X-Hub-Signature") | ||
| if signature == "" { | ||
| return false | ||
| } | ||
|
|
||
| // Remove the "sha256=" prefix if present | ||
| if len(signature) > 7 && signature[0:7] == "sha256=" { | ||
| signature = signature[7:] | ||
| } | ||
|
|
||
| // Create a new HMAC | ||
|
|
||
| mac := hmac.New(sha256.New, []byte(webhookSecret)) | ||
| mac.Write(body) | ||
| expectedMAC := mac.Sum(nil) | ||
| expectedSignature := hex.EncodeToString(expectedMAC) | ||
|
|
||
| return hmac.Equal([]byte(signature), []byte(expectedSignature)) | ||
| } |
There was a problem hiding this comment.
Return an HTTP 4xx response on signature mismatch.
Although the HMAC verification logic looks correct, the caller ignores its result. Consider returning an HTTP status code (e.g., 401 Unauthorized) if the signature check fails, to prevent further processing of invalid requests.
func verifySignature(c *gin.Context, body []byte, webhookSecret string) bool {
...
return hmac.Equal([]byte(signature), []byte(expectedSignature))
}
func (ee DiggerEEController) BitbucketWebhookHandler(c *gin.Context) {
+ if !verifySignature(c, bodyBytes, bitbucketWebhookSecret) {
+ log.Printf("ERROR: signature mismatch")
+ c.String(http.StatusUnauthorized, "invalid signature")
+ return
+ }
...
}Committable suggestion skipped: line range outside the PR's diff.
ee/backend/controllers/bitbucket.go
Outdated
|
|
||
| jobs, _, err := generic.ConvertIssueCommentEventToJobs(repoFullName, actor, issueNumber, commentBody, impactedProjects, requestedProject, config.Workflows, prBranchName, defaultBranch) | ||
| if err != nil { | ||
| log.Printf("Error converting event to jobs: %v", err) | ||
| utils.InitCommentReporter(bbService, issueNumber, fmt.Sprintf(":x: Error converting event to jobs: %v", err)) | ||
| return fmt.Errorf("error converting event to jobs") | ||
| } | ||
| log.Printf("GitHub IssueComment event converted to Jobs successfully\n") | ||
|
|
||
| err = utils.ReportInitialJobsStatus(commentReporter, jobs) | ||
| if err != nil { | ||
| log.Printf("Failed to comment initial status for jobs: %v", err) | ||
| utils.InitCommentReporter(bbService, issueNumber, fmt.Sprintf(":x: Failed to comment initial status for jobs: %v", err)) | ||
| return fmt.Errorf("failed to comment initial status for jobs") | ||
| } | ||
|
|
||
| if len(jobs) == 0 { | ||
| log.Printf("no projects impacated, succeeding") | ||
| // This one is for aggregate reporting | ||
| err = utils.SetPRStatusForJobs(bbService, issueNumber, jobs) | ||
| return nil | ||
| } | ||
|
|
||
| err = utils.SetPRStatusForJobs(bbService, issueNumber, jobs) | ||
| if err != nil { | ||
| log.Printf("error setting status for PR: %v", err) | ||
| utils.InitCommentReporter(bbService, issueNumber, fmt.Sprintf(":x: error setting status for PR: %v", err)) | ||
| fmt.Errorf("error setting status for PR: %v", err) | ||
| } | ||
|
|
||
| impactedProjectsMap := make(map[string]dg_configuration.Project) | ||
| for _, p := range impactedProjects { | ||
| impactedProjectsMap[p.Name] = p | ||
| } | ||
|
|
||
| impactedProjectsJobMap := make(map[string]scheduler.Job) | ||
| for _, j := range jobs { | ||
| impactedProjectsJobMap[j.ProjectName] = j | ||
| } | ||
|
|
||
| commentId64, err := strconv.ParseInt(commentReporter.CommentId, 10, 64) | ||
| if err != nil { | ||
| log.Printf("ParseInt err: %v", err) | ||
| return fmt.Errorf("parseint error: %v", err) | ||
| } | ||
| batchId, _, err := utils.ConvertJobsToDiggerJobs(*diggerCommand, models.DiggerVCSBitbucket, organisationId, impactedProjectsJobMap, impactedProjectsMap, projectsGraph, 0, branch, issueNumber, repoOwner, repoName, repoFullName, commitSha, commentId64, diggerYmlStr, 0, "", false) | ||
| if err != nil { | ||
| log.Printf("ConvertJobsToDiggerJobs error: %v", err) | ||
| utils.InitCommentReporter(bbService, issueNumber, fmt.Sprintf(":x: ConvertJobsToDiggerJobs error: %v", err)) | ||
| return fmt.Errorf("error convertingjobs") | ||
| } | ||
|
|
||
| if config.CommentRenderMode == dg_configuration.CommentRenderModeGroupByModule && | ||
| (*diggerCommand == scheduler.DiggerCommandPlan || *diggerCommand == scheduler.DiggerCommandApply) { | ||
|
|
||
| sourceDetails, err := comment_updater.PostInitialSourceComments(bbService, issueNumber, impactedProjectsSourceMapping) | ||
| if err != nil { | ||
| log.Printf("PostInitialSourceComments error: %v", err) | ||
| utils.InitCommentReporter(bbService, issueNumber, fmt.Sprintf(":x: PostInitialSourceComments error: %v", err)) | ||
| return fmt.Errorf("error posting initial comments") | ||
| } | ||
| batch, err := models.DB.GetDiggerBatch(batchId) | ||
| if err != nil { | ||
| log.Printf("GetDiggerBatch error: %v", err) | ||
| utils.InitCommentReporter(bbService, issueNumber, fmt.Sprintf(":x: PostInitialSourceComments error: %v", err)) | ||
| return fmt.Errorf("error getting digger batch") | ||
| } | ||
|
|
||
| batch.SourceDetails, err = json.Marshal(sourceDetails) | ||
| if err != nil { | ||
| log.Printf("sourceDetails, json Marshal error: %v", err) | ||
| utils.InitCommentReporter(bbService, issueNumber, fmt.Sprintf(":x: json Marshal error: %v", err)) | ||
| return fmt.Errorf("error marshalling sourceDetails") | ||
| } | ||
| err = models.DB.UpdateDiggerBatch(batch) | ||
| if err != nil { | ||
| log.Printf("UpdateDiggerBatch error: %v", err) | ||
| utils.InitCommentReporter(bbService, issueNumber, fmt.Sprintf(":x: UpdateDiggerBatch error: %v", err)) | ||
| return fmt.Errorf("error updating digger batch") | ||
| } | ||
| } | ||
|
|
||
| segment.Track(strconv.Itoa(int(organisationId)), "backend_trigger_job") | ||
|
|
||
| // hardcoded bitbucket ci backend for this controller | ||
| // TODO: making this configurable based on env variable and connection | ||
| ciBackend := ci_backends2.BitbucketPipelineCI{ | ||
| RepoName: repoName, | ||
| RepoOwner: repoOwner, | ||
| Branch: branch, | ||
| Client: bbService, | ||
| } | ||
| err = controllers.TriggerDiggerJobs(ciBackend, repoFullName, repoOwner, repoName, batchId, issueNumber, bbService, nil) | ||
| if err != nil { | ||
| log.Printf("TriggerDiggerJobs error: %v", err) | ||
| utils.InitCommentReporter(bbService, issueNumber, fmt.Sprintf(":x: TriggerDiggerJobs error: %v", err)) | ||
| return fmt.Errorf("error triggering Digger Jobs") | ||
| } | ||
| return nil | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
General concurrency and error-handling improvements.
• At line 260, you assign err = utils.SetPRStatusForJobs(bbService, issueNumber, jobs) but never check or return it, making the assignment ineffectual.
• At line 268, you call fmt.Errorf(...) but never use the returned error, so it’s never acted upon.
These issues may cause silent failures and complicate debugging. Consider logging or returning these errors to ensure they’re visible.
258 // This one is for aggregate reporting
259 ...
260- err = utils.SetPRStatusForJobs(bbService, issueNumber, jobs)
+ if err := utils.SetPRStatusForJobs(bbService, issueNumber, jobs); err != nil {
+ log.Printf("SetPRStatusForJobs error: %v", err)
+ return
+ }
267 ...
268- fmt.Errorf("error setting status for PR: %v", err)
+ if err != nil {
+ newErr := fmt.Errorf("error setting status for PR: %v", err)
+ log.Printf("Returning new error: %v", newErr)
+ return newErr
+ }Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 golangci-lint (1.62.2)
268-268: Error return value of fmt.Errorf is not checked
(errcheck)
260-260: ineffectual assignment to err
(ineffassign)
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (3)
ee/backend/controllers/gitlab.go (1)
510-511:⚠️ Potential issueFix unreturned error created by fmt.Errorf.
Similar to the issue in the Bitbucket controller, the error created by fmt.Errorf is never returned.
-fmt.Errorf("error setting status for PR: %v", err) +return fmt.Errorf("error setting status for PR: %v", err)backend/models/storage.go (2)
623-649: 💡 Verification agent🧩 Analysis chain
Added VCSConnectionId parameter but no schema validation
You've added
VCSConnectionIdas a pointer toCreateDiggerBatch, which is good for supporting multiple VCS types. However, there's no validation to ensure that ifvcsTypeis set to a Bitbucket type, a validVCSConnectionIdis provided.Consider adding validation to ensure the VCS type and connection ID are consistent.
Run the following to find usage patterns:
🏁 Script executed:
#!/bin/bash # Find calls to CreateDiggerBatch to see how VCSConnectionId is being used echo "Looking for calls to CreateDiggerBatch..." rg "CreateDiggerBatch\(" --type goLength of output: 2327
Action Required: Add VCS Type–Specific Validation for VCSConnectionId
It appears that all current call sites for
CreateDiggerBatch(primarily with GitHub types) pass anilforVCSConnectionId. However, to future-proof the function and prevent misuse when supporting other VCS types (e.g., Bitbucket), it’s recommended to add a check that ensures a validVCSConnectionIdis provided whenever the chosenvcsTyperequires it.Suggestions:
- Add a conditional validation at the beginning of
CreateDiggerBatchsuch as:if vcsType == DiggerVCSBitbucket && VCSConnectionId == nil { return nil, errors.New("VCSConnectionId must be provided for Bitbucket") }- Update the schema or documentation accordingly to reflect this requirement.
These changes will help avoid potential runtime issues when the function is used with VCS types that mandate a connection ID.
931-950: 💡 Verification agent🧩 Analysis chain
SQL query in
GetJobsByRepoNamemight need updating for non-GitHub VCSThe function now returns
[]queries.JobQueryResultinstead ofJobQueryResult, which is good for handling multiple results. However, the SQL query still joins only with GitHub-specific tables and doesn't account for Bitbucket or other VCS providers.Consider updating the SQL query to handle different VCS types based on your database schema changes. The current query will only work for GitHub connections.
🏁 Script executed:
#!/bin/bash # Search for other VCS-related tables in the codebase echo "Looking for VCS-related tables definitions in models package..." rg -A 10 "type.*struct" --type go --glob "*/models/*.go" | grep -E "(VCS|Bitbucket)"Length of output: 708
Action Required: Update SQL Query for Multi-VCS Support
The current implementation of
GetJobsByRepoNameretrieves multiple job results correctly, but it still relies on a join with the GitHub-specific tablegithub_app_installation_links. Our codebase now defines VCS connections for multiple providers (GitHub, GitLab, Bitbucket, etc.), as confirmed by the VCS-related model definitions (e.g., inbackend/models/orgs.goandbackend/models/scheduler.go). This means that for repositories using non-GitHub VCS providers, the query's join condition may be invalid or incomplete.Please consider:
- Updating the SQL query to dynamically join the correct VCS-related table or adjust the logic based on the repository's VCS type.
- Verifying that all supported VCS providers are adequately handled by ensuring that the appropriate fields are joined or filtered according to our current schema.
♻️ Duplicate comments (7)
libs/spec/providers.go (1)
193-203:⚠️ Potential issueFix error message to reference the correct environment variable.
The error message mentions "GITLAB_TOKEN" when it should reference "DIGGER_BITBUCKET_ACCESS_TOKEN" to match the environment variable being checked on line 195.
- return nil, fmt.Errorf("failed to get bitbucket service: GITLAB_TOKEN not specified") + return nil, fmt.Errorf("failed to get bitbucket service: DIGGER_BITBUCKET_ACCESS_TOKEN not specified")libs/ci/bitbucket/bitbucket.go (2)
627-631: 🛠️ Refactor suggestionUpdate ReadAll call to use io package instead of ioutil.
Following the package import change, you should also update the ReadAll function call.
- body, err := ioutil.ReadAll(resp.Body) + body, err := io.ReadAll(resp.Body)
126-136: 🛠️ Refactor suggestionUpdate ReadAll call to use io package instead of ioutil.
Following the package import change, you should also update the ReadAll function call.
- body, err := ioutil.ReadAll(resp.Body) + body, err := io.ReadAll(resp.Body)ee/backend/controllers/bitbucket.go (4)
120-120: 🛠️ Refactor suggestionCapture or handle the error returned by
handleIssueCommentEventBB.When calling the function in a goroutine, its error return value is lost. Consider logging or otherwise handling this error inside the goroutine.
-go handleIssueCommentEventBB(ee.BitbucketProvider, &pullRequestCommentCreated, ee.CiBackendProvider, orgId, &connectionEncrypted.ID, bitbucketAccessToken) +go func() { + if err := handleIssueCommentEventBB(ee.BitbucketProvider, &pullRequestCommentCreated, ee.CiBackendProvider, orgId, &connectionEncrypted.ID, bitbucketAccessToken); err != nil { + log.Printf("handleIssueCommentEventBB returned error: %v", err) + } +}()🧰 Tools
🪛 golangci-lint (1.62.2)
120-120: Error return value is not checked
(errcheck)
268-268:⚠️ Potential issueFix unreturned error created by fmt.Errorf.
The error created by fmt.Errorf is never returned or logged.
-fmt.Errorf("error setting status for PR: %v", err) +return fmt.Errorf("error setting status for PR: %v", err)🧰 Tools
🪛 golangci-lint (1.62.2)
268-268: Error return value of
fmt.Errorfis not checked(errcheck)
111-111:⚠️ Potential issueUse the result of
verifySignaturefor security checks.The function call to
verifySignatureis executed but its return value is ignored, which creates a security vulnerability as requests with invalid signatures will be processed.-verifySignature(c, bodyBytes, bitbucketWebhookSecret) +if !verifySignature(c, bodyBytes, bitbucketWebhookSecret) { + log.Printf("ERROR: invalid Bitbucket webhook signature") + c.String(http.StatusUnauthorized, "invalid signature") + return +}
260-260:⚠️ Potential issueFix ineffectual assignment to err variable.
The assignment of err is ineffectual as the error is never checked or returned.
-err = utils.SetPRStatusForJobs(bbService, issueNumber, jobs) +if err := utils.SetPRStatusForJobs(bbService, issueNumber, jobs); err != nil { + log.Printf("Error setting PR status for jobs: %v", err) +}🧰 Tools
🪛 golangci-lint (1.62.2)
260-260: ineffectual assignment to err
(ineffassign)
🧹 Nitpick comments (7)
backend/utils/graphs.go (1)
17-17: Function signature updated for VCS connection supportThe
ConvertJobsToDiggerJobsfunction has been modified to accept a newVCSConnectionIdparameter to support Bitbucket integration. This is a good change that generalizes the function to work with multiple VCS providers.The parameter name
VCSConnectionIddoesn't follow Go's standard naming convention, which typically uses camelCase for variables. Consider renaming it tovcsConnectionIdfor consistency with the rest of the codebase.-func ConvertJobsToDiggerJobs(jobType scheduler.DiggerCommand, vcsType models.DiggerVCSType, organisationId uint, jobsMap map[string]scheduler.Job, projectMap map[string]configuration.Project, projectsGraph graph.Graph[string, configuration.Project], githubInstallationId int64, branch string, prNumber int, repoOwner string, repoName string, repoFullName string, commitSha string, commentId int64, diggerConfigStr string, gitlabProjectId int, aiSummaryCommentId string, reportTerraformOutput bool, VCSConnectionId *uint) (*uuid.UUID, map[string]*models.DiggerJob, error) { +func ConvertJobsToDiggerJobs(jobType scheduler.DiggerCommand, vcsType models.DiggerVCSType, organisationId uint, jobsMap map[string]scheduler.Job, projectMap map[string]configuration.Project, projectsGraph graph.Graph[string, configuration.Project], githubInstallationId int64, branch string, prNumber int, repoOwner string, repoName string, repoFullName string, commitSha string, commentId int64, diggerConfigStr string, gitlabProjectId int, aiSummaryCommentId string, reportTerraformOutput bool, vcsConnectionId *uint) (*uuid.UUID, map[string]*models.DiggerJob, error) {backend/services/spec.go (3)
37-38: Fix typo in commentThere's a typo in the comment - "peice" should be "piece".
- // TODO: Refactor this peice into its own + // TODO: Refactor this piece into its own
39-62: Consider refactoring Bitbucket token retrieval into a helper functionThe Bitbucket token retrieval logic is quite lengthy and could be refactored into a separate helper function for better maintainability, especially since the comment already indicates this intention.
case models.DiggerVCSBitbucket: - // TODO: Refactor this peice into its own - if batch.VCSConnectionId == nil { - return nil, fmt.Errorf("connection ID not set, could not get vcs token") - } - log.Printf("vcs connection id %v", batch.VCSConnectionId) - connectionId := strconv.Itoa(int(*batch.VCSConnectionId)) - connectionEncrypted, err := models.DB.GetVCSConnectionById(connectionId) - if err != nil { - log.Printf("failed to fetch connection: %v", err) - return nil, fmt.Errorf("failed to fetch connection: %v", err) - } - - secret := os.Getenv("DIGGER_ENCRYPTION_SECRET") - if secret == "" { - log.Printf("ERROR: no encryption secret specified, please specify DIGGER_ENCRYPTION_SECRET as 32 bytes base64 string") - return nil, fmt.Errorf("ERROR: no encryption secret specified, please specify DIGGER_ENCRYPTION_SECRET as 32 bytes base64 string") - } - connectionDecrypted, err := utils.DecryptConnection(connectionEncrypted, []byte(secret)) - if err != nil { - log.Printf("ERROR: could not perform decryption: %v", err) - return nil, fmt.Errorf("ERROR: could not perform decryption: %v", err) - } - bitbucketAccessToken := connectionDecrypted.BitbucketAccessToken - token = bitbucketAccessToken + bitbucketToken, err := getBitbucketTokenFromConnection(batch.VCSConnectionId) + if err != nil { + return nil, err + } + token = bitbucketTokenAdd this helper function:
func getBitbucketTokenFromConnection(connectionId *int64) (string, error) { if connectionId == nil { return "", fmt.Errorf("connection ID not set, could not get vcs token") } log.Printf("vcs connection id %v", connectionId) connIdStr := strconv.Itoa(int(*connectionId)) connectionEncrypted, err := models.DB.GetVCSConnectionById(connIdStr) if err != nil { log.Printf("failed to fetch connection: %v", err) return "", fmt.Errorf("failed to fetch connection: %v", err) } secret := os.Getenv("DIGGER_ENCRYPTION_SECRET") if secret == "" { log.Printf("ERROR: no encryption secret specified, please specify DIGGER_ENCRYPTION_SECRET as 32 bytes base64 string") return "", fmt.Errorf("ERROR: no encryption secret specified, please specify DIGGER_ENCRYPTION_SECRET as 32 bytes base64 string") } connectionDecrypted, err := utils.DecryptConnection(connectionEncrypted, []byte(secret)) if err != nil { log.Printf("ERROR: could not perform decryption: %v", err) return "", fmt.Errorf("ERROR: could not perform decryption: %v", err) } return connectionDecrypted.BitbucketAccessToken, nil }
51-54: Improve error message about encryption secret formatThe error message mentions that the encryption secret should be a "32 bytes base64 string" but doesn't provide clear guidance on how to generate such a string. Consider adding more specific instructions.
- log.Printf("ERROR: no encryption secret specified, please specify DIGGER_ENCRYPTION_SECRET as 32 bytes base64 string") - return nil, fmt.Errorf("ERROR: no encryption secret specified, please specify DIGGER_ENCRYPTION_SECRET as 32 bytes base64 string") + errorMsg := "ERROR: no encryption secret specified, please specify DIGGER_ENCRYPTION_SECRET as 32 bytes base64 string. You can generate one with: openssl rand -base64 32" + log.Printf(errorMsg) + return nil, fmt.Errorf(errorMsg)backend/models/storage.go (3)
441-454: Function renamed but log message still refers to GitHub AppThe function has been correctly renamed from
CreateGithubAppConnectiontoCreateVCSConnectionand now includes Bitbucket parameters, but the log message on line 460 still references "CreateGithubApp" instead of using VCS-neutral terminology.- log.Printf("CreateGithubApp (name: %v, url: %v) has been created successfully\n", app.Name, app.GithubAppUrl) + log.Printf("CreateVCSConnection (name: %v, url: %v) has been created successfully\n", app.Name, app.GithubAppUrl)
464-472: Update error message to use VCS-neutral terminologyThe function has been renamed to
GetVCSConnectionById, but the error message on line 469 still refers to "GitHub App".- log.Printf("Failed to find GitHub App for id: %v, error: %v\n", id, result.Error) + log.Printf("Failed to find VCS Connection for id: %v, error: %v\n", id, result.Error)
475-483: Update function comment and error message for VCS terminologyThe function has been renamed from
GetGithubApptoGetVCSConnection, but both the comment on line 474 and the error message on line 479 still refer to "GitHub App".-// GetGithubApp return GithubApp by Id +// GetVCSConnection return VCSConnection by Id func (db *Database) GetVCSConnection(gitHubAppId any) (*VCSConnection, error) { app := VCSConnection{} result := db.GormDB.Where("github_id = ?", gitHubAppId).Find(&app) if result.Error != nil { - log.Printf("Failed to find GitHub App for id: %v, error: %v\n", gitHubAppId, result.Error) + log.Printf("Failed to find VCS Connection for id: %v, error: %v\n", gitHubAppId, result.Error) return nil, result.Error } return &app, nil }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
backend/migrations/atlas.sumis excluded by!**/*.sum
📒 Files selected for processing (15)
backend/controllers/github.go(3 hunks)backend/controllers/github_test.go(5 hunks)backend/migrations/20250302190926.sql(1 hunks)backend/models/scheduler.go(2 hunks)backend/models/storage.go(6 hunks)backend/models/storage_test.go(2 hunks)backend/services/spec.go(1 hunks)backend/tasks/runs_test.go(2 hunks)backend/utils/graphs.go(2 hunks)cli/pkg/spec/spec.go(1 hunks)ee/backend/controllers/bitbucket.go(1 hunks)ee/backend/controllers/gitlab.go(6 hunks)ee/backend/hooks/github.go(1 hunks)libs/ci/bitbucket/bitbucket.go(6 hunks)libs/spec/providers.go(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (4)
- backend/models/scheduler.go
- backend/controllers/github_test.go
- backend/models/storage_test.go
- backend/controllers/github.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
libs/ci/bitbucket/bitbucket.go
9-9: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os], and those implementations should be preferred in new code. See the specific function documentation for details.
(staticcheck)
ee/backend/controllers/bitbucket.go
120-120: Error return value is not checked
(errcheck)
268-268: Error return value of fmt.Errorf is not checked
(errcheck)
260-260: ineffectual assignment to err
(ineffassign)
🔇 Additional comments (5)
ee/backend/hooks/github.go (1)
154-154: Updated function call to support VCS connectionsThe call to
utils.ConvertJobsToDiggerJobshas been updated to include a new parameternilfor the VCS connection ID, which is part of the Bitbucket support feature. This is correct since this hook is GitHub-specific and uses the GitHub installation ID directly instead of a VCS connection ID.backend/utils/graphs.go (1)
46-46: Updated DB call to include VCS connection IDThe new parameter is correctly passed to the DB layer, allowing the batch to be associated with a VCS connection if provided.
cli/pkg/spec/spec.go (1)
102-102: LGTM: Added VCS type parameter to support multiple VCS providersThe addition of the VCS type parameter to the GetReporter method call aligns well with the goal of supporting multiple VCS providers, including Bitbucket.
backend/tasks/runs_test.go (2)
53-53: LGTM: Updated model in AutoMigrate to use VCSConnectionThis change correctly updates the AutoMigrate call to use the new
VCSConnectionmodel instead of the GitHub-specificGithubAppConnection, aligning with the transition to a generalized VCS connection model.
146-146:✅ Verification successful
Additional nil parameter added to CreateDiggerBatch
The added nil parameter corresponds to the new vcs_connection_id field in the DiggerBatch model. This change aligns with the migration that added this field.
🏁 Script executed:
#!/bin/bash # Check the CreateDiggerBatch method signature to verify the new parameter rg -A 5 "func.*CreateDiggerBatch" --type goLength of output: 1389
Review Update: Nil Parameter Correctly Added to CreateDiggerBatch
- The additional nil parameter in the test call aligns with the updated function signature in
backend/models/storage.go, where the newVCSConnectionId *uintparameter has been introduced.- The change correctly reflects the migration that added the
vcs_connection_idfield to theDiggerBatchmodel.- No further modifications are needed.
| -- Modify "digger_batches" table | ||
| ALTER TABLE "public"."digger_batches" ADD COLUMN "vcs_connection_id" bigint NULL, ADD CONSTRAINT "fk_digger_batches_vcs_connection" FOREIGN KEY ("vcs_connection_id") REFERENCES "public"."github_app_connections" ("id") ON UPDATE NO ACTION ON DELETE NO ACTION; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Potential inconsistency in foreign key constraint naming
The foreign key constraint references "github_app_connections" table but uses a more generic name "vcs_connection_id" for the column. While this is part of transitioning to a more generalized VCS model that supports Bitbucket, it might be confusing to have a GitHub-specific table referenced by a generically named column.
🏁 Script executed:
#!/bin/bash
# Check if there's a plan to rename the github_app_connections table
rg -A 3 -B 3 "github_app_connections" --type sqlLength of output: 2076
Action Required: Fix Inconsistent Naming in Foreign Key Constraint
The migration in backend/migrations/20250302190926.sql adds a generic column (vcs_connection_id) to the digger_batches table that still references the github_app_connections table. Our search confirms that the table name has not been updated in related migrations, which suggests that the intended move toward a more generalized VCS design (to support Bitbucket alongside GitHub) isn’t fully reflected. This naming discrepancy could lead to confusion regarding the purpose and scope of the connection.
- File:
backend/migrations/20250302190926.sql- The column is named
vcs_connection_idbut its foreign key constraint references"public"."github_app_connections".
- The column is named
- Observation: Searches in other migration files (e.g.,
20241107172343.sql,20250226185150.sql, and20241107163722.sql) show that the table name remains unchanged. - Recommendation:
- Either update the table name to reflect a more generalized VCS connection or rename the column (and its associated constraint) to make clear that it exclusively references GitHub connections.
- Ensure consistency across all migration scripts for clarity in your database schema.
| case "bitbucket": | ||
| token := os.Getenv("DIGGER_BITBUCKET_ACCESS_TOKEN") | ||
| if token == "" { | ||
| return nil, fmt.Errorf("failed to get bitbucket service: GITLAB_TOKEN not specified") | ||
| } | ||
| return bitbucket.BitbucketAPI{ | ||
| AuthToken: token, | ||
| HttpClient: http.Client{}, | ||
| RepoWorkspace: vcsSpec.RepoOwner, | ||
| RepoName: vcsSpec.RepoName, | ||
| }, nil |
There was a problem hiding this comment.
Fix error message and extract duplicated code.
- The error message incorrectly references "GITLAB_TOKEN" instead of "DIGGER_BITBUCKET_ACCESS_TOKEN".
- The Bitbucket service creation logic is duplicated between
GetPrServiceandGetOrgService.
- return nil, fmt.Errorf("failed to get bitbucket service: GITLAB_TOKEN not specified")
+ return nil, fmt.Errorf("failed to get bitbucket service: DIGGER_BITBUCKET_ACCESS_TOKEN not specified")Consider extracting the Bitbucket service creation logic into a helper function to avoid duplication:
func createBitbucketService(token string, repoWorkspace string, repoName string) (bitbucket.BitbucketAPI, error) {
if token == "" {
return bitbucket.BitbucketAPI{}, fmt.Errorf("failed to get bitbucket service: DIGGER_BITBUCKET_ACCESS_TOKEN not specified")
}
return bitbucket.BitbucketAPI{
AuthToken: token,
HttpClient: http.Client{},
RepoWorkspace: repoWorkspace,
RepoName: repoName,
}, nil
}| return "", fmt.Errorf("error parsing response: %v", err) | ||
| } | ||
|
|
||
| return "", nil |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Return the pipeline URL or ID instead of an empty string.
The function returns an empty string on success, which doesn't provide the caller with any useful information about the triggered pipeline.
- return "", nil
+ return triggerPipelineResponse.Links.HTML.Href, nil📝 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.
| return "", nil | |
| return triggerPipelineResponse.Links.HTML.Href, nil |
| @@ -6,6 +6,7 @@ | |||
| "fmt" | |||
| "github.com/diggerhq/digger/libs/ci" | |||
| configuration "github.com/diggerhq/digger/libs/digger_config" | |||
| "io/ioutil" | |||
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace deprecated "ioutil" package with "io" package.
The ioutil package is deprecated since Go 1.16. Use the equivalent functionality from the io package instead.
- "io/ioutil"
+ "io"📝 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.
| "io/ioutil" | |
| "io" |
🧰 Tools
🪛 golangci-lint (1.62.2)
9-9: SA1019: "io/ioutil" has been deprecated since Go 1.19: As of Go 1.16, the same functionality is now provided by package [io] or package [os], and those implementations should be preferred in new code. See the specific function documentation for details.
(staticcheck)
Summary by CodeRabbit
New Features
Enhancements
Database Updates