JetBrains / TeamCity.SonarQubePlugin

A plugin for TeamCity server allowing you to configure and run SonarQube analysis from the CI
Apache License 2.0
53 stars 31 forks source link

Fix #69 : Build feature "SonarQube Branches & Pull Requests" support #77

Closed axel3rd closed 4 years ago

axel3rd commented 4 years ago

In addition part/fix #76 : Plugin packaging is failing (some revert of previous commit, see issue #76 for details ; not sure if it is a correct way for fix).

It is my first "from scratch TeamCity plugin feature" => feel free for any remarks, code style convention (I'm an Eclipse IDE user), ...

I have validate behavior locally. I will do more tests on our development platform after a first review.

axel3rd commented 4 years ago

Tested on our company TeamCity DEV platform (v2019.2.1), it works fine as expected.

But log "SonarQube plugin detects [pull-request|branch], 'env.SONARQUBE_SCANNER_PARAMS' set with ..." is displayed 3 times ... I should understand why 😒

axel3rd commented 4 years ago

But log "SonarQube plugin detects [pull-request|branch], 'env.SONARQUBE_SCANNER_PARAMS' set with ..." is displayed 3 times ... I should understand why 😒

Fixed by Execute process once (and not 4 each steps). It was because ParametersPreprocessor is executed for each steps of build.


From my side, no more improvments. Waiting any remarks/review.

The produced log is visible at head, and perhaps little long in case of Pull-Request:

18:32:35 The build is removed from the queue to be prepared for the start
18:32:35 Collecting changes in 1 VCS root
18:32:36 Starting the build on the agent "CLOUD_XXX"
18:32:37 SonarQube plugin detects pull-request, 'env.SONARQUBE_SCANNER_PARAMS' set with '{"sonar.pullrequest.key":"4","sonar.pullrequest.branch":"Update README.md","sonar.pullrequest.base":"master","sonar.pullrequest.provider":"github","sonar.pullrequest.github.repository":"orga/repo"}'
18:32:37 Updating tools for build
18:32:37 Clearing temporary directory: /xxx/teamcityBuildAgent/temp/buildTmp
18:32:37 Publishing internal artifacts
18:32:37 Checkout directory: /xxx/teamcityBuildAgent/work/763e09e4819fd742
18:32:37 Updating sources: agent side checkout
Linfar commented 4 years ago

Overall the feature seems to be fine. Yet after discussing with colleagues of mine I have one question - is a separate build feature (BranchesAndPullRequestsBuildFeature) needed at all? May be it's ok just to look for Pull Requests build feature and to add branch info by default?

axel3rd commented 4 years ago

👋 @Linfar,

Thanks about feedback.

May be it's ok just to look for Pull Requests build feature and to add branch info by default?

This is a good remark, two highlights about it, before taking a decision:

Pull request case

Currently there is no feedback on How to retrieve the "from" branch in a pull-request build ?, so current implementation supports only GitHub. I don't found some way to retrieve and be sure the VCS is GitHub, no parameter about that, and checking github in VCS URL is a little poor ; perhaps it exist a way to get information from PR-Support feature. All that to say if SONARQUBE_SCANNER_PARAMS is filled and used with another Git platform, it could have an impact on behavior.

Branch case

Due to SONAR-11155, if sonar.branch.name is provided on first analysis with SonarQube v7.9 (current LTS), it fails. Perhaps teamcity.build.branch.is_default could be used to check that, but a potential automatic feature should be very robust. Tha capacity to "disable it" (Due to a configurable Build Feature) is perhaps to keep/important in a first time.

Linfar commented 4 years ago

@karanagai May be you can suggest something?

karanagai commented 4 years ago

Hi @axel3rd, I am not sure what you mean saying there is no feedback. Pull Requests plugin should output the pull request information into parameters for all supported systems (GitHub, Bitbucket Server and GitLab). Could you clarify please?

axel3rd commented 4 years ago

👋 @karanagai ,

Could you clarify please?

The SONARQUBE_SCANNER_PARAMS sysenv should contain the sonar.pullrequest.provider (ex: GitHub) and some dedicated provider properties like sonar.pullrequest.github.repository.

The Git provider is not part of Pull Requests plugin (like teamcity.pullRequest.provider), due to memory optimizations (See Anton explanation, second part of TW-61490 thread) (all PR infos like number/title/etc are here and helpful).

=> How to determine Git provider (if not defined in a Build Feature) ? To fill or not correct SonarQube scanner properties.

Perhaps could be retrieved by "calling" the Pull Requests plugin directly (Spring bean, ...), but not via TeamCity parameter at build start. And the impact in this case is perhaps to "closely link" the TeamCity SonarQube plugin to TeamCity v2019.2 (currently even if this implementation feature is enabled with previous TeamCity version, there is an error message but build is continuing).

And once again, the SONARQUBE_SCANNER_PARAMS is not an official way to manage SonarQube scanner (not documented), even if works fine => enabling "automatically" (without any configuration or user decision) a third-party tools integration in this case is perhaps dangerous IMO.

But it is just a feeling, I'm so newbie in TeamCity plugins development to take final decision 😁.

karanagai commented 4 years ago

@alex3rd you don't have to call the pull requests plugin code to determine the provider. What you can do is to extract respective build feature descriptor (via getBuildFeaturesOfType) and take its providerType property, which must be one of the following: "github", "gitlab" or "bitbucketServer" (also most likely "azureDevOps" in the coming 2020.1 version of TeamCity).

As for the target repository, you are parsing its URL anyway. I assume you do not need the source repository of the pull request for your purposes?

Although a user being able to disable your build feature sounds like a valid reason to have a separate build feature.

axel3rd commented 4 years ago

What you can do is to extract respective build feature descriptor (via getBuildFeaturesOfType) and take its providerType property,

Definitely newbie in TeamCity plugin development 😢 ; good catch. Provider can be retrieved from here.

@Linfar:

  1. Did you want a PR update in this way ?
  2. Or current is acceptable for a first try ?

I promote the [2]. Not because I would be lazy, but if dedicated Build Feature has an interest ("Although a user being able to disable your build feature sounds like a valid reason to have a separate build feature"), the fact to have the "provider list" (currently only GitHub) in the Build Feature, it is expliciting which provider are supported.


As for the target repository, you are parsing its URL anyway. I assume you do not need the source repository of the pull request for your purposes?

@karanagai : yes & no 😁.

Linfar commented 4 years ago

Sorry, it seems I've forgot to post comment :( I think it's ok to begin with this approach, I'll merge the pull request and will integrate it to release branch

axel3rd commented 4 years ago

@Linfar : Many thanks ! 😗