-
Notifications
You must be signed in to change notification settings - Fork 33
feat(scm): Add Bitbucket Cloud SCM #699
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
Bundle ReportBundle size has no change ✅ |
|
Thanks @ushiradineth! Just so you know, the latest code fixes a security scan failure. :-) |
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #699 +/- ##
==========================================
- Coverage 55.70% 51.62% -4.09%
==========================================
Files 37 41 +4
Lines 3958 4271 +313
==========================================
Hits 2205 2205
- Misses 1460 1772 +312
- Partials 293 294 +1 ☔ View full report in Codecov by Sentry. |
4adfa9b to
fc9c919
Compare
Signed-off-by: Ushira Dineth <[email protected]>
Signed-off-by: Ushira Dineth <[email protected]>
Signed-off-by: Ushira Dineth <[email protected]>
Signed-off-by: Ushira Dineth <[email protected]>
Signed-off-by: Ushira Dineth <[email protected]>
Signed-off-by: Ushira Dineth <[email protected]>
Signed-off-by: Ushira Dineth <[email protected]>
Signed-off-by: Ushira Dineth <[email protected]>
Signed-off-by: Ushira Dineth <[email protected]>
- Replace type assertions with errors.As for wrapped error handling
- Replace interface{} with any for Go 1.18+ compatibility
- Add default cases to switch statements per revive rules
- Remove underscore receiver names per revive receiver-naming rule
- Merge variable declarations with assignments per staticcheck
- Replace fmt.Errorf with errors.New for static error messages
- Check exists before using map values to avoid unnecessary blank assignments
These changes address all 25 linter issues (6 errorlint, 17 revive, 2 staticcheck)
while maintaining the existing functionality of the Bitbucket SCM provider.
Signed-off-by: Ushira Dineth <[email protected]>
fc9c919 to
ddb91e7
Compare
|
Will also mentioned that you will probably want to enable webhook support as well, it's not technically required but it makes the user experience so much better that I feel it is really should be required. |
|
Thanks @crenshaw-dev! I've synced the PR with the main branch and the checks have succeeded now. |
@zachaller I did not notice this haha :D. I will go ahead implement this as well. |
Signed-off-by: Ushira Dineth <[email protected]>
Signed-off-by: Ushira Dineth <[email protected]>
Signed-off-by: Ushira Dineth <[email protected]>
43e2a44 to
8dcded3
Compare
|
@crenshaw-dev @zachaller I have added webhook support and checks are passing. Additionally, I renamed bitbucket to bitbucket_cloud (and other instances) incase of a future Bitbucket Server intergration. |
crenshaw-dev
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great work! Overall looking good, just did a first pass.
| // Workspace is the workspace of the repository. | ||
| // +kubebuilder:validation:Required | ||
| Workspace string `json:"workspace"` | ||
| // Repository is the repository of the repository. | ||
| // +kubebuilder:validation:Required | ||
| Repository string `json:"repository"` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is there any additional validation that we can apply to these fields? At minimum, I think we should have a min length of 1 char if they're both really required.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I mainly followed the validations defined on the other repo types, I've added MinLength=1.
| return github.NewGithubPullRequestProvider(ctx, r.Client, scmProvider, *secret, gitRepository.Spec.GitHub.Owner) //nolint:wrapcheck | ||
| case scmProvider.GetSpec().GitLab != nil: | ||
| return gitlab.NewGitlabPullRequestProvider(r.Client, *secret, scmProvider.GetSpec().GitLab.Domain) //nolint:wrapcheck | ||
| case scmProvider.GetSpec().Bitbucket != nil: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we go ahead and name it BitbucketCloud to disambiguate the implementations from the start?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have done a through check and have renamed all Bitbucket/bitbucket appearances to BitbucketCloud/bitbucket_cloud/bitbucketCloud.
| // Parse error message to determine status code | ||
| statusCode := http.StatusCreated | ||
| if err != nil { | ||
| statusCode = http.StatusInternalServerError | ||
| var bbErr *bitbucket.UnexpectedResponseStatusError | ||
| if errors.As(err, &bbErr) { | ||
| errMsg := bbErr.Error() | ||
| switch { | ||
| case strings.HasPrefix(errMsg, "401"): | ||
| statusCode = http.StatusUnauthorized | ||
| case strings.HasPrefix(errMsg, "404"): | ||
| statusCode = http.StatusNotFound | ||
| default: | ||
| statusCode = http.StatusInternalServerError | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe pull this into a helper function?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have abstracted the error code parsing into a util.
| } | ||
|
|
||
| // Extract state | ||
| state, _ := resultMap["state"].(string) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Any risk of a panic here? Maybe just go ahead and check the cast success?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
| commitStatus.Status.Sha = commitStatus.Spec.Sha | ||
|
|
||
| // Bitbucket doesn't return an ID for commit statuses, use key as identifier | ||
| commitStatus.Status.Id = commitStatus.Spec.Name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zachaller any concerns here? Seems maybe as good as any option?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It does look like bitbucket returns a uuid
{
"type": "<string>",
"links": {
"self": {
"href": "<string>",
"name": "<string>"
},
"commit": {
"href": "<string>",
"name": "<string>"
}
},
"uuid": "<string>",
"key": "<string>",
"refname": "<string>",
"url": "<string>",
"state": "FAILED",
"name": "<string>",
"description": "<string>",
"created_on": "<string>",
"updated_on": "<string>"
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sorry for overlooking this, I have fixed this now.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@zachaller @ushiradineth the doc https://developer.atlassian.com/cloud/bitbucket/rest/api-group-commit-statuses/#api-repositories-workspace-repo-slug-commit-commit-statuses-build-post say it returns the UUID as the base level but from what I can see running the code it is return in a repository object
Below you will see the output of the result object after being converted to json.
{
"commit": {
"hash": "0719fc1ee1174a385ccbc23da4430d4e579d1b47",
"links": {
"html": {
"href": "https://bitbucket.org/djnono8434/argocd-example-apps/commits/0719fc1ee1174a385ccbc23da4430d4e579d1b47"
},
"self": {
"href": "https://api.bitbucket.org/2.0/repositories/djnono8434/argocd-example-apps/commit/0719fc1ee1174a385ccbc23da4430d4e579d1b47"
}
},
"type": "commit"
},
"created_on": "2025-12-10T23:01:44.697804+00:00",
"description": "Waiting for previous environment's \"argocd-health\" commit status to be successful",
"key": "build",
"links": {
"commit": {
"href": "https://api.bitbucket.org/2.0/repositories/djnono8434/argocd-example-apps/commit/0719fc1ee1174a385ccbc23da4430d4e579d1b47"
},
"self": {
"href": "https://api.bitbucket.org/2.0/repositories/djnono8434/argocd-example-apps/commit/0719fc1ee1174a385ccbc23da4430d4e579d1b47/statuses/build/build"
}
},
"name": "nolan",
"refname": null,
"repository": {
"full_name": "djnono8434/argocd-example-apps",
"links": {
"avatar": {
"href": "https://bytebucket.org/ravatar/%7B1c25d6d7-7863-4cec-9986-12c7135e11d4%7D?ts=default"
},
"html": {
"href": "https://bitbucket.org/djnono8434/argocd-example-apps"
},
"self": {
"href": "https://api.bitbucket.org/2.0/repositories/djnono8434/argocd-example-apps"
}
},
"name": "argocd-example-apps",
"type": "repository",
"uuid": "{1c25d6d7-7863-4cec-9986-12c7135e11d4}"
},
"state": "INPROGRESS",
"type": "build",
"updated_on": "2025-12-11T00:03:25.431922+00:00",
"url": "https://bitbucket.org/djnono8434/argocd-example-apps"
}
| // The Bitbucket client doesn't return HTTP response metadata, so we parse | ||
| // the error message to determine status codes (e.g., "400 Bad Request") | ||
| statusCode := http.StatusCreated | ||
| if err != nil { | ||
| statusCode = http.StatusInternalServerError | ||
| var bbErr *bitbucket.UnexpectedResponseStatusError | ||
| if errors.As(err, &bbErr) { | ||
| errMsg := bbErr.Error() | ||
| switch { | ||
| case strings.HasPrefix(errMsg, "400"): | ||
| statusCode = http.StatusBadRequest | ||
| case strings.HasPrefix(errMsg, "401"): | ||
| statusCode = http.StatusUnauthorized | ||
| default: | ||
| statusCode = http.StatusInternalServerError | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Share helper func across similar uses?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
|
|
||
| // Build query to find PRs matching source and target branches and state | ||
| // Bitbucket query syntax: https://developer.atlassian.com/cloud/bitbucket/rest/intro/#querying | ||
| query := fmt.Sprintf(`source.branch.name="%s" AND destination.branch.name="%s" AND state="OPEN"`, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The word "injection" comes to mind..... is there a way we can escape the inputs reliably? Maybe strconv.Quote?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Woah I did not notice it, I have fixed it.
Co-authored-by: Michael Crenshaw <[email protected]> Signed-off-by: Ushira Dineth <[email protected]>
Signed-off-by: Ushira Dineth <[email protected]>
Signed-off-by: Ushira Dineth <[email protected]>
Signed-off-by: Ushira Dineth <[email protected]>
Signed-off-by: Ushira Dineth <[email protected]>
Signed-off-by: Ushira Dineth <[email protected]>
Signed-off-by: Ushira Dineth <[email protected]>
Signed-off-by: Ushira Dineth <[email protected]>
|
@ushiradineth from what I see you, I think you are missing this https://github.com/ushiradineth/gitops-promoter/pull/2/files |
| commitStatusOptions := &bitbucket.CommitStatusOptions{ | ||
| State: phaseToBuildState(commitStatus.Spec.Phase), | ||
| Key: commitStatus.Spec.Name, | ||
| Url: commitStatus.Spec.Url, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ushiradineth
From what I saw testing it looks like the URL is empty and bitbucket does not allow that it is a required parameter
@zachaller does it need to be the url or having example.org will work just fine?
Fixes #179
Adds Bitbucket Cloud integration as an SCM provider