Sq 8 2+azure devops #202
Sq 8 2+azure devops #202
Conversation
Sonarqube 8.2 introduced a new `ProjectDto` with associated changes in `ProjectAlmSettingsDao` to require this new class in various methods, as well as changes in `ComponentFinder` to allow retrieval of instances of a project. This change makes use of these new classes and methods to allow the Web Services for setting up ALM bindings to operate. As this class and the associated methods did not exist in previous versions of Sonarqube, this change breaks backwards compatibility, so means the plugin will now only support Sonarqube 8.2. Sonarqube 8.2 also provides the ability to set the optional URL parameter on a Gitlab project so that scans run outside of Gitlab CI operate properly, so the additional parameter is now included in the appopriate WebServices and the Gitlab decorator.
|
I can confirm this build works well in sonarqube version 8.3.1.34397 |
|
UPD: I figured out what the problem was. Target branch should have been analyzed before creating the PR. Problem solved. I get the following error on version 8.3.1.34397 with on-premise Azure DevOps. Any ideas? Sonar Properties: {
"sonar.host.url": "http://sonarqube.dev.local/",
"sonar.login": "*******",
"sonar.projectKey": "*******",
"sonar.projectName": "*******",
"sonar.projectVersion": "1.0.0+7ddb00c1e6b4de01bc65fb6a0647b30e858d20f6",
"sonar.pullrequest.key": "661",
"sonar.pullrequest.base": "develop",
"sonar.pullrequest.branch": "azure-pipelines-pull-requests",
"sonar.pullrequest.provider": "vsts",
"sonar.pullrequest.vsts.instanceUrl": "https://azure.dev.local/tfs/DefaultCollection/",
"sonar.pullrequest.vsts.project": "*******",
"sonar.pullrequest.vsts.repository": "*******",
"sonar.scanner.metadataFilePath": "/azp/agent/_work/_temp/sonar/20200703.10/321f1354-82cf-599f-2c3f-9959d0134674/report-task.txt"
}Compute Engine logs |
|
Now I can confirm, this build works fine with SonarQube 8.3.1.34397 and Azure on-premise with REST API 5.1-preview. |
...1arke/sonarqube/plugin/ce/pullrequest/azuredevops/AzureDevOpsServerPullRequestDecorator.java
Outdated
Show resolved
Hide resolved
|
Throwing error on I think should fix spaces on project names or repository names. If having spaces it throws errors. |
|
@alekssako f203253 allows for spaces in the URI - it should encode the full URI before sending the request. Thanks for the reviews. |
@jcuzzi Keep it up with the good work you`ve done. I found out that in sonarqube "See the PR" link is referencing to the API instead of the PR itself on the azure devops.
Can't find it in the code, but its something like : Instead it should be something like : |
There was a problem hiding this comment.
Thanks for the contribution. I've given it an initial review although haven't tried executing it yet so may come back with further comments.
A few points not necessarily covered in my comments:
- Formatting: please ensure opening curly braces are part of the preceding line rather than being on their own line. Your change has some of these correct but seems to switch for some classes
- Please squash your commits and ensure your commit message explains why you've done things you have (i.e. if there's anything you want people to understand why you've done something then put it in the commit message)
- Your decorator class needs to be immutable for thread safety. The rest of the classes in the plugin also aim for immutability for consistency, so please aim for this too.
If you have any questions about any of my comments or disagree with any of them then please shout!
| LOGGER.info("Found " + properties.size() + " scanner properties..."); | ||
|
|
||
| for (Map.Entry<String,String> entry : properties.entrySet()) | ||
| LOGGER.debug("Key = " + entry.getKey() + ", Value = " + entry.getValue()); |
There was a problem hiding this comment.
Please put curly braces around this to make it clear what the limits of the for loop are
| LOGGER.info("Found " + properties.size() + " scanner properties..."); | ||
|
|
||
| for (Map.Entry<String,String> entry : properties.entrySet()) | ||
| LOGGER.debug("Key = " + entry.getKey() + ", Value = " + entry.getValue()); |
There was a problem hiding this comment.
Could we use String.format rather than String concatenation please?
| .getIssues(); | ||
| /*.stream() | ||
| .filter(i -> OPEN_ISSUE_STATUSES.contains(i.getIssue().getStatus())) | ||
| .collect(Collectors.toList());*/ |
There was a problem hiding this comment.
Is this code relevant to something? I suspect you do actually need to filter, but if you don't then please remove commented out code
| .count() > 0 ) { | ||
|
|
||
| if(!issue.getIssue().getStatus().equals(Issue.STATUS_OPEN) | ||
| && azureThread.getStatus().equals(CommentThreadStatus.ACTIVE)) { |
There was a problem hiding this comment.
Use == and != for enum comparison
| private Boolean deleted; | ||
| @JsonProperty("_links") | ||
| private ReferenceLinks links; | ||
| public CommentThread(){}; |
There was a problem hiding this comment.
Where is this constructor used? In theory, all the fields above it should be final, but this constructor prevents that.
| projectConfiguration.get(GitlabServerPullRequestDecorator.PULLREQUEST_GITLAB_INSTANCE_URL).ifPresent(v -> sensorContext | ||
| .addContextProperty(GitlabServerPullRequestDecorator.PULLREQUEST_GITLAB_INSTANCE_URL, v)); | ||
| projectConfiguration.get(GitlabServerPullRequestDecorator.PULLREQUEST_GITLAB_PROJECT_ID).ifPresent(v -> sensorContext | ||
| .addContextProperty(GitlabServerPullRequestDecorator.PULLREQUEST_GITLAB_PROJECT_ID, v)); | ||
| projectConfiguration.get(GitlabServerPullRequestDecorator.PULLREQUEST_GITLAB_PROJECT_URL).ifPresent(v -> sensorContext | ||
| .addContextProperty(GitlabServerPullRequestDecorator.PULLREQUEST_GITLAB_PROJECT_URL, v)); | ||
| projectConfiguration.get(GitlabServerPullRequestDecorator.PULLREQUEST_GITLAB_PIPELINE_ID).ifPresent(v -> sensorContext | ||
| .addContextProperty(GitlabServerPullRequestDecorator.PULLREQUEST_GITLAB_PIPELINE_ID, v)); | ||
|
|
There was a problem hiding this comment.
Why have these been added? They seem unrelated to the intent of your change
| .addContextProperty(AzureDevOpsServerPullRequestDecorator.PULLREQUEST_AZUREDEVOPS_PULLREQUEST_ID, v)); | ||
|
|
||
|
|
||
| Optional.ofNullable(system2.property(AzureDevOpsServerPullRequestDecorator.PULLREQUEST_AZUREDEVOPS_INSTANCE_URL)).ifPresent( |
There was a problem hiding this comment.
Should these variables not be the ones that are injected by Azure DevOps, rather than custom-named ones?
| public Map<String,String> getScannerProperties() { | ||
| return scannerContext.getProperties(); | ||
| } | ||
|
|
There was a problem hiding this comment.
This only seems to have been introduced to allow logging of all the properties, which doesn't seem like a good use case. The logging should either happen in the PostAnalysisTask that triggers decoration, or be removed.
| private String authorizationHeader; | ||
| private static final Logger LOGGER = Loggers.get(AzureDevOpsServerPullRequestDecorator.class); | ||
| public static final String API_VERSION_PREFIX = "?api-version="; |
There was a problem hiding this comment.
Please ensure you're following the Java Language Specification field ordering:
public static final
private static final
private final
private
| private String apiVersion = "6.0-preview.1"; | ||
| private String azureUrl = ""; | ||
| private String baseBranch = ""; | ||
| private String branch = ""; | ||
| private String pullRequestId = ""; | ||
| private String azureRepositoryName = ""; | ||
| private String azureProjectId = ""; |
There was a problem hiding this comment.
These should all be local variables that get passed to any required methods, otherwise it will cause a race condition if multiple decorators are executed at the same time
|
Sorry, I've just merged the 8.2 support branch which has auto-closed your MR. Could you open a new one pointing at master please? |
|
Yeah, once I have time to make the requested changes, I'll create a new PR to master. |
This should be fixed in #218 |
Manual merge of @Iloer's Azure DevOps PR into @mc1arke's sq-8_2-support branch (with some additions to get it all working).
Notes:
The following should be set in the scanner properties:
sonar.pullrequest.vsts.instanceUrl=https://[organization].visualstudio.com/
sonar.pullrequest.vsts.project=[Project]
sonar.pullrequest.vsts.repository=[Repository]
Ensure that the following option is unchecked in the Azure DevOps Branch Policy configuration, otherwise an exception will be thrown:
"Reset status whenever there are new changes"