-
-
Notifications
You must be signed in to change notification settings - Fork 532
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
Add support for Bitbucket Code Insights #151
Conversation
@4n4n4s maybe you have some good input on this one? |
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.
Hi @goober
first of all cool that you're integrating the code insights. I didn't know that this exists already for so long.
Maybe the "code insights" implementation should completely replace the comments feature and we already should set it to deprecated as 5.15 of bitbucket server was realeased in 2018.
Similar to all the other files in the project please also add the copyright and other info on top of all your added files.
Added some remarks to files, didn't have the time yet to make a test drive on my local instance. For example would be good to know if #149 is also fixed with the annotations.
...rke/sonarqube/plugin/ce/pullrequest/bitbucket/insights/client/model/CreateReportRequest.java
Outdated
Show resolved
Hide resolved
...ithub/mc1arke/sonarqube/plugin/ce/pullrequest/bitbucket/insights/client/BitbucketClient.java
Outdated
Show resolved
Hide resolved
"SonarQube", | ||
analysisDetails.getAnalysisDate().toInstant(), | ||
analysisDetails.getDashboardUrl(), | ||
"https://www.sonarqube.org/favicon-152.png", |
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.
suggestion deliver this image also in the static resources so that instances of SQ that might not be able to access the internet still have an icon displayed see /common/bug.svg
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.
Fixed
...lugin/ce/pullrequest/bitbucket/insights/BitbucketServerCodeInsightsPullRequestDecorator.java
Outdated
Show resolved
Hide resolved
...lugin/ce/pullrequest/bitbucket/insights/BitbucketServerCodeInsightsPullRequestDecorator.java
Outdated
Show resolved
Hide resolved
...rke/sonarqube/plugin/ce/pullrequest/bitbucket/insights/client/model/CreateReportRequest.java
Outdated
Show resolved
Hide resolved
a2bf205
to
fe62f84
Compare
I agree with you, should I remove the comments implementation in the same PR or should we do it in a separate one? We have just upgraded to Bitbucket Server 7.1.1 and unfortunately, the comments with included images looks really awful.
Done
I have manually verified that this is not an issue with the Code Insights API |
I'd remove it in this PR. The delegate implementations currently implement the Decorator interface but don't really have a need to, so removing the old implementation should allow you to collapse code together and simplify the whole implementation. |
fe62f84
to
47ed592
Compare
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.
A few additional points from me. Could you also squash your commits (or confirm you're happy for me to squash on merge)?
...hub/mc1arke/sonarqube/plugin/ce/pullrequest/bitbucket/BitbucketPullRequestDecoratorTest.java
Outdated
Show resolved
Hide resolved
...hub/mc1arke/sonarqube/plugin/ce/pullrequest/bitbucket/BitbucketPullRequestDecoratorTest.java
Outdated
Show resolved
Hide resolved
.../com/github/mc1arke/sonarqube/plugin/ce/pullrequest/bitbucket/client/BitbucketException.java
Outdated
Show resolved
Hide resolved
...b/mc1arke/sonarqube/plugin/ce/pullrequest/bitbucket/BitbucketServerPullRequestDecorator.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/mc1arke/sonarqube/plugin/ce/pullrequest/AnalysisDetails.java
Outdated
Show resolved
Hide resolved
src/main/java/com/github/mc1arke/sonarqube/plugin/ce/pullrequest/AnalysisDetails.java
Outdated
Show resolved
Hide resolved
47ed592
to
4adf6a5
Compare
@mc1arke I have updated the PR and squashed the commits according to your comments. |
This change will add support for Bitbucket Code Insights in favor of regular comments when available. It will fall back on the comments strategy when the Code Insights is not available (it is supported in version 5.15 and later).
4adf6a5
to
1eed63f
Compare
SonarCloud Quality Gate failed. 0 Bugs |
@goober I've merged this with a couple of small modifications for the issues highlighted by Sonarqube and a few formatting tweaks, |
@goober @mc1arke While trying to get this to work for the cloud version I encountered a really strange behavior. Is there a chance that this PR broke the overrides for project specific parameters for bitbucket? Only Project and Repository can be overrriden anymore since those come from the UnifiyConfiguration object. All other properties (like URL and token) can now only be defined globally. Individual project settings (through GUI - not CLI) are no longer taken into account. Did someone test this?
I try to fix it but the code probably doesn't look that nice anymore after doing that since UnifyConfiguration is not on DI. |
@marvin-w I've not specifically tested this so can't comment. Since I've just merged the Sonarqube 8.1 changes into master, The latest plugin releases will currently only support over-riding configuration in the Gitlab decorator since most configuration is now driven through Sonarqube APIs |
This change will add support for Bitbucket Code Insights in favor of regular comments when available. It will fall back on the comments strategy when the Code Insights is not available (it is supported in version 5.15 and later).