-
-
Notifications
You must be signed in to change notification settings - Fork 520
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
Implemented PullRequest decoration to AzureDevOps server (issue/102 8 1support) #123
Conversation
Sonarqube 8.1 removed the concept of short and long lived branches, instead simplifying the setup to either `BRANCH` or `PULL_REQUEST`. This release of Sonarqube also moved the management of Pull Request decoration into a standard User Interface calling a set of 'ALM' services that provide the management of decorators, and the binding of each project to these decorators. However the implementation of these services is not included in the Community Edition of Sonarqube, although the UI is made visible based on the presence of the branch management components provided by this plugin. This change therefore introduces the services required to support UI components for the management of Pull Request decoration, as well as updating the handling of the configuration and loading of branches to support the removal of the SHORT/LONG branch constructs. As these changes require the reference of classes that were not present in older version of Sonarqube (namely `AlmSettingsDao` and `ProjectAlmSettingsDao`), the compatibility interfaces for these versions have been removed, as well as any methods and classes that existed purely to allow these versions to be supported by this plugin. Given the standard UI provides a restricted set of fields for creating and binding each Pull Request decorator, some of the patterns that were previously used by this plugin for configuring the decoration of Pull Requests do not fit into this UI, and were awkward to find/manage when moved to another screen in the UI. This results in the configuration for disabling the addition and removing of comments on Merge Requests being removed from the plugin, with the Gitlab decorator removing the deleting of old comments since this was leading to discussion boxes being shown in Gitlab with no content, and the BitBucket decorator removing since it can't work out which user posted comments based purely on the configuration provided by Sonarqube. To ensure the UI works for all configuration options, the services for configuring and binding Azure DevOps configuration have been included in this change-set, although no decorator currently exists for this ALM, so any project attempting to use this configuration will get a warning appear in the CE logs when attempting to use this decorator. Similarly, as the UI does not provide any options for configuring the URL for the Gitlab API, or the Slug/name of the project on the binding, a `Sensor` has been added into the scanner to detect these properties from injection by the Gitlab CI Runner, as well as injection directly from scanner arguments of `com.github.mc1arke.sonarqube.plugin.branch.pullrequest.gitlab.url` and `com.github.mc1arke.sonarqube.plugin.branch.pullrequest.gitlab.repositorySlug` for the repository URL and repository-name configuration entries.
The Github ALM Binding Web Service uses the `AlmRepo` field to store the repository name, but the Github decorator was using `AlmSlug` to try and retrieve the repository name, so was getting a `null` value back and failing to find a matching repository. Switching to using `AlmRepo` in the decorator overcomes this issues.
Initial commit with test
add getContextProperty method
fix test
Add POST PR Status
mc1arke#102 add Azure object class and small refactoring
mc1arke#102 add azure thread with issue
mc1arke#102 add: closed issue in sonar, resolve in Azure
mc1arke#102 add unit tests
mc1arke#102 small refactoring
mc1arke#102 Fix unit test
remove unused code
remove star import
...ain/java/com/github/mc1arke/sonarqube/plugin/ce/pullrequest/PullRequestPostAnalysisTask.java
Outdated
Show resolved
Hide resolved
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.
Thanks for your contribution!
I've put a few comments on specific lines that generally apply to your whole change. Could we aim for immutable classes, and low visibility methods and fields where possible.
I've not gone through and functionally reviewed your changes yet given there are quite a few classes will need modified to meet the above ruquest.
Could you also squash your commits to a single commit per feature (i.e. this PR should probably be a single commit), and ensure your commit message follows https://github.com/RomuloOliveira/commit-messages-guide.
/** | ||
* The ID of the parent comment. This is used for replies. | ||
*/ | ||
public void setParentCommentId(Integer value){ |
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.
Can we make this object immutable?
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.
Resolved
import java.io.Serializable; | ||
|
||
public class CommentPosition implements Serializable { | ||
private Integer line; |
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.
int
rather than Integer
, or is this null
alble?
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.
Resolved
|
||
public CommentPosition(Integer line, Integer offset){ | ||
this.line = line; | ||
this.offset = offset + 1; |
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.
Why + 1
?
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.
Char numbering in line in Azure PullRequest starts at "0", while in SonarQube starts at "1"
import com.fasterxml.jackson.annotation.JsonProperty; | ||
|
||
public class CommentThreadResponse { | ||
private CommentThread[] value; |
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.
final
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.
Resolved
/** | ||
* ID of the iteration to associate status with. Minimum value is 1. | ||
*/ | ||
public Integer iterationId = null; |
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.
These should be private, and access through getters, with initialisation in the constructor
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.
Resolved
@@ -0,0 +1,5 @@ | |||
package com.github.mc1arke.sonarqube.plugin.ce.pullrequest.azuredevops.model; | |||
|
|||
public class PropertiesCollection { |
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.
What's the intent of this class? It contains no fields or methods
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.
Resolved
/** | ||
* The thread status is unknown. | ||
*/ | ||
unknown, |
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.
Naming conventions are to have enum fields in upper-case. Is there any reason that can't be followed here?
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.
Resolved
import com.github.mc1arke.sonarqube.plugin.ce.pullrequest.azuredevops.model.enums.CommentThreadStatus; | ||
import org.sonar.api.issue.Issue; | ||
|
||
import static org.sonar.api.issue.Issue.*; |
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.
Please don't use star imports
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.
Resolved
import static org.sonar.api.issue.Issue.*; | ||
|
||
|
||
public class CommentThreadStatusMapper { |
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.
final
if the constructor is private
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.
Resolved
import com.github.mc1arke.sonarqube.plugin.ce.pullrequest.azuredevops.model.enums.GitStatusState; | ||
import org.sonar.api.ce.posttask.QualityGate; | ||
|
||
public class GitStatusStateMapper { |
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.
final
if the constructor is private
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.
Resolved
mc1arke#102 comment fixes
…qube-community-branch-plugin into issue/102-8_1support
…qube-community-branch-plugin into issue/102-8_1support
@mc1arke I am tried rebase, but something went wrong, please help squash the commits |
mc1arke#102 enum to UPPERCASE
Can it be used also with TFS on premise? |
Yes, tested on AzureDevOps server 2019 (on premise) on API version 5.0-preview.1 |
@mc1arke please see this pullrequets |
mc1arke#102 attempt to close comment in AzureDevOps by issue.key
a34a666
to
bf136e4
Compare
What about this PR? This is my dream PR 🥇 |
11eac55
to
9b3aedf
Compare
2d029de
to
89b5982
Compare
#102 Implemented PullRequest decoration to AzureDevOps server
(only Git Repository, without TFVC)