-
Notifications
You must be signed in to change notification settings - Fork 228
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
Set up CI with Azure Pipelines #2826
Conversation
2704832
to
ac9b647
Compare
477fbdf
to
93c19e8
Compare
91149a2
to
80efda1
Compare
80efda1
to
6312349
Compare
@@ -19,6 +19,7 @@ | |||
<AllowedOutputExtensionsInPackageBuildOutputFolder>$(AllowedOutputExtensionsInPackageBuildOutputFolder);.pdb</AllowedOutputExtensionsInPackageBuildOutputFolder> | |||
<CodeAnalysisRuleSet>..\..\..\..\ValidationRuleset.ruleset</CodeAnalysisRuleSet> | |||
<ProjectGuid>025B9F93-7953-454B-AF29-E6A55B2AA58F</ProjectGuid> | |||
<DisableImplicitNuGetFallbackFolder>true</DisableImplicitNuGetFallbackFolder> |
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.
This was needed due to this issue: NuGet/Home#7414
@@ -0,0 +1,409 @@ | |||
{ |
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 packages.lock.json files were generated to enable caching on azure pipelines
stages: | ||
- template: stage-with-burgr-notifications.yml@commonTemplates | ||
parameters: | ||
burgrName: 'build' |
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.
@tomverin - are these names predefined or can we give custom names? it would be more readable to be analyzer build
inside burgr
@@ -110,17 +115,17 @@ public void projectIsAnalyzed() { | |||
|
|||
@Test | |||
public void linesAtProjectLevel() { | |||
assertThat(getProjectMeasureAsInt("lines")).isEqualTo(117); | |||
assertThat(getProjectMeasureAsInt("lines")).isEqualTo(118); |
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.
note: this test MetricsIncludeHeaderCommentTest.java
was not being run before on the QA (when I added it 6 months ago I forgot to update Tests.java
). These metrics are the same with the ones in Metrics.java
.
So these changes are fine
@@ -46,6 +46,7 @@ | |||
CasingAppTest.class, | |||
CoverageTest.class, | |||
DoNotAnalyzeTestFilesTest.class, | |||
MetricsIncludeHeaderCommentTest.class, |
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.
this wasn't run before on QA
its/src/test/java/com/sonar/it/vbnet/DoNotAnalyzeTestFilesTest.java
Outdated
Show resolved
Hide resolved
its/src/test/java/com/sonar/it/vbnet/DoNotAnalyzeTestFilesTest.java
Outdated
Show resolved
Hide resolved
String vstsSourcePath = VstsUtils.getSourcesDirectory(); | ||
baseDirectory = new File(vstsSourcePath); | ||
} | ||
TemporaryFolder folder = new TemporaryFolder(baseDirectory); |
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.
could directly return this
@@ -2,7 +2,7 @@ | |||
"issues": [ | |||
{ | |||
"id": "S1200", | |||
"message": "Split this class into smaller and more specialized ones to reduce its dependencies on other classes from 33 to the maximum authorized 30 or less.", | |||
"message": "Split this class into smaller and more specialized ones to reduce its dependencies on other classes from 34 to the maximum authorized 30 or less.", |
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.
this is related to #2854
@@ -50,6 +50,7 @@ public class PerformanceTests | |||
// dotnet test or vstest.console.exe: pass the following command line argument | |||
// --TestCaseFilter:"TestCategory!=Slow" | |||
[TestCategory("Slow")] | |||
[Ignore("Skip this one as it adds significant delays. It will be moved to the integration tests.")] |
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.
related to #2578
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.
almost there
@christophe-zurn-sonarsource - most of the thousands of lines of code are actually auto-generated json files needed by nuget caching (and they're at the end of the PR list), so it's not that bad as review experience |
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.
LGTM
displayName: 'Run Code Analysis - End Step' | ||
|
||
- task: SonarCloudPublish@1 | ||
displayName: 'Run Code Analysis - Publish QG' |
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.
FYI, the QG tab on burgr is missing for the new pipeline
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 @tomverin, do you know why the QG is missing?
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.
Ok, I've found out. When executing the SonarCloudPrepare task we have to specify the analysis parameters. These will be sent later to burgr to display the Quality Gate.
I've added a few commits with the required changes.
sqMavenPluginVersionChoice: 'latest' | ||
|
||
- task: SonarCloudPublish@1 | ||
displayName: 'Run Code Analysis - Publish QG' |
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.
We have 2 SonarCloudPublish
in the yml file. From our discussion, I get that we mostly care about .NET SonarAnalyzer QG, however, are we sure that it is always the correct QG that will be published to github ?
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.
Very good question. Due to a SonarCloud/Qube limitation, currently, only one QG is displayed on GitHub; the one which completes the last (it overwrites all the previous ones).
Since the .Net unit tests take longer that java ones (with about 4 minutes difference) their result will be the one displayed. This is a happy coincidence since this is what we want to display.
I think this could be good enough for now until the cloud team will address that limitation. What to you think?
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 think we should have a ticket to track this.
Do you know if the sonarcloud team is actively working to fix this?
(address code review comments)
e53b68b
to
fb227f3
Compare
- the dependencies are defined with stageDependencies
ddfac7d
to
b1967a9
Compare
9a90c26
to
3fd3c2e
Compare
3fd3c2e
to
7e694cb
Compare
- in order to match burger commit id
- was added for debugging purposes
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.
LGTM
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.
LGTM !
Fixes #2834