Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Don't fail GitLab pipeline on QualityGate failure - optional property #637

Closed
wants to merge 4 commits into from
Closed

Don't fail GitLab pipeline on QualityGate failure - optional property #637

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
82828a5
[#137][GitLab] Extra property to disable pipeline failing on failed a…
szpak Jul 1, 2022
087b5d9
[#137][GitLab] Simplify GitLab tests with junit-jupiter-params
szpak Jul 1, 2022
e23009c
[#137][GitLab] Tweaks after GitlabMergeRequestDecoratorTest migration…
szpak Jul 1, 2022
5baff3e
[#137][GitLab] Adjust property name to be passed to Sonar
szpak Jul 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,7 @@ dependencies {
implementation('com.squareup.okhttp3:logging-interceptor:4.10.0')
testImplementation(platform('org.junit:junit-bom:5.8.2'))
testImplementation('org.junit.jupiter:junit-jupiter')
testImplementation('org.junit.jupiter:junit-jupiter-params')
testImplementation('junit:junit:4.13.2')
testRuntimeOnly('org.junit.vintage:junit-vintage-engine')
}
Expand Down
8 changes: 7 additions & 1 deletion ...om/github/mc1arke/sonarqube/plugin/ce/pullrequest/gitlab/GitlabMergeRequestDecorator.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -52,6 +52,8 @@ public class GitlabMergeRequestDecorator extends DiscussionAwarePullRequestDecor
public static final String PULLREQUEST_GITLAB_PROJECT_URL = "sonar.pullrequest.gitlab.projectUrl";
public static final String PULLREQUEST_GITLAB_PIPELINE_ID =
"com.github.mc1arke.sonarqube.plugin.branch.pullrequest.gitlab.pipelineId";
public static final String PULLREQUEST_GITLAB_DONT_FAIL_PIPELINE =
"sonar.analysis.com.github.mc1arke.sonarqube.plugin.branch.pullrequest.gitlab.dontFailPipeline";

private final GitlabClientFactory gitlabClientFactory;
private final MarkdownFormatterFactory formatterFactory;
Expand Down Expand Up @@ -121,11 +123,15 @@ protected void submitPipelineStatus(GitlabClient gitlabClient, MergeRequest merg
Long pipelineId = analysis.getScannerProperty(PULLREQUEST_GITLAB_PIPELINE_ID)
.map(Long::parseLong)
.orElse(null);
boolean dontFailPipeline = analysis.getScannerProperty(PULLREQUEST_GITLAB_DONT_FAIL_PIPELINE)
.map(Boolean::parseBoolean)
.orElse(false);
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a facility we don't really want: someone who has access to submit scans can decide whether the pipeline passes on a Quality Gate failure, rather than it being clear that a passing pipeline is because of clean code. In the companies I work with, the decision is made up-front as to what rules must be adhered to, and it is assumed that a pipeline is passing because those rules have been met. If we start allowing people who control the scanner to decide whether they can bypass Quality Checks then we'd effectively be breaking the safeguard that Sonarqube is providing in these scenarios.


try {
PipelineStatus pipelineStatus = new PipelineStatus("SonarQube",
"SonarQube Status",
analysis.getQualityGateStatus() == QualityGate.Status.OK ? PipelineStatus.State.SUCCESS : PipelineStatus.State.FAILED,
analysis.getQualityGateStatus() == QualityGate.Status.OK || dontFailPipeline ?
PipelineStatus.State.SUCCESS : PipelineStatus.State.FAILED,
analysisSummary.getDashboardUrl(),
analysisSummary.getNewCoverage(),
pipelineId);
Expand Down
83 changes: 50 additions & 33 deletions ...ithub/mc1arke/sonarqube/plugin/ce/pullrequest/gitlab/GitlabMergeRequestDecoratorTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,11 @@
import com.github.mc1arke.sonarqube.plugin.ce.pullrequest.report.AnalysisIssueSummary;
import com.github.mc1arke.sonarqube.plugin.ce.pullrequest.report.AnalysisSummary;
import com.github.mc1arke.sonarqube.plugin.ce.pullrequest.report.ReportGenerator;
import org.junit.Before;
import org.junit.Test;
import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.junit.jupiter.params.ParameterizedTest;
import org.junit.jupiter.params.provider.Arguments;
import org.junit.jupiter.params.provider.MethodSource;
import org.mockito.ArgumentCaptor;
import org.sonar.api.ce.posttask.QualityGate;
import org.sonar.api.issue.Issue;
Expand All @@ -56,6 +59,7 @@
import java.util.Collections;
import java.util.Optional;
import java.util.stream.Collectors;
import java.util.stream.Stream;

import static org.assertj.core.api.Assertions.assertThat;
import static org.assertj.core.api.Assertions.assertThatThrownBy;
Expand All @@ -69,7 +73,7 @@
import static org.mockito.Mockito.verify;
import static org.mockito.Mockito.when;

public class GitlabMergeRequestDecoratorTest {
class GitlabMergeRequestDecoratorTest {

private static final long MERGE_REQUEST_IID = 123;
private static final long PROJECT_ID = 101;
Expand Down Expand Up @@ -101,8 +105,8 @@ public class GitlabMergeRequestDecoratorTest {

private final GitlabMergeRequestDecorator underTest = new GitlabMergeRequestDecorator(scmInfoRepository, gitlabClientFactory, reportGenerator, markdownFormatterFactory);

@Before
public void setUp() throws IOException {
@BeforeEach
void setUp() throws IOException {
when(analysisSummary.format(any())).thenReturn("Summary Comment");
when(reportGenerator.createAnalysisSummary(any())).thenReturn(analysisSummary);
AnalysisIssueSummary analysisIssueSummary = mock(AnalysisIssueSummary.class);
Expand Down Expand Up @@ -132,12 +136,12 @@ public void setUp() throws IOException {
}

@Test
public void shouldReturnCorrectDecoratorType() {
void shouldReturnCorrectDecoratorType() {
assertThat(underTest.alm()).containsOnly(ALM.GITLAB);
}

@Test
public void shouldThrowErrorWhenPullRequestKeyNotNumeric() {
void shouldThrowErrorWhenPullRequestKeyNotNumeric() {
when(analysisDetails.getPullRequestId()).thenReturn("non-MR-IID");

assertThatThrownBy(() -> underTest.decorateQualityGateStatus(analysisDetails, almSettingDto, projectAlmSettingDto))
Expand All @@ -146,7 +150,7 @@ public void shouldThrowErrorWhenPullRequestKeyNotNumeric() {
}

@Test
public void shouldThrowErrorWhenGitlabMergeRequestRetrievalFails() throws IOException {
void shouldThrowErrorWhenGitlabMergeRequestRetrievalFails() throws IOException {
when(gitlabClient.getMergeRequest(any(), anyLong())).thenThrow(new IOException("dummy"));

assertThatThrownBy(() -> underTest.decorateQualityGateStatus(analysisDetails, almSettingDto, projectAlmSettingDto))
Expand All @@ -155,7 +159,7 @@ public void shouldThrowErrorWhenGitlabMergeRequestRetrievalFails() throws IOExce
}

@Test
public void shouldThrowErrorWhenGitlabUserRetrievalFails() throws IOException {
void shouldThrowErrorWhenGitlabUserRetrievalFails() throws IOException {
when(gitlabClient.getCurrentUser()).thenThrow(new IOException("dummy"));

assertThatThrownBy(() -> underTest.decorateQualityGateStatus(analysisDetails, almSettingDto, projectAlmSettingDto))
Expand All @@ -164,7 +168,7 @@ public void shouldThrowErrorWhenGitlabUserRetrievalFails() throws IOException {
}

@Test
public void shouldThrowErrorWhenGitlabMergeRequestCommitsRetrievalFails() throws IOException {
void shouldThrowErrorWhenGitlabMergeRequestCommitsRetrievalFails() throws IOException {
when(gitlabClient.getMergeRequestCommits(anyLong(), anyLong())).thenThrow(new IOException("dummy"));

assertThatThrownBy(() -> underTest.decorateQualityGateStatus(analysisDetails, almSettingDto, projectAlmSettingDto))
Expand All @@ -173,7 +177,7 @@ public void shouldThrowErrorWhenGitlabMergeRequestCommitsRetrievalFails() throws
}

@Test
public void shouldThrowErrorWhenGitlabMergeRequestDiscussionRetrievalFails() throws IOException {
void shouldThrowErrorWhenGitlabMergeRequestDiscussionRetrievalFails() throws IOException {
when(gitlabClient.getMergeRequestDiscussions(anyLong(), anyLong())).thenThrow(new IOException("dummy"));

assertThatThrownBy(() -> underTest.decorateQualityGateStatus(analysisDetails, almSettingDto, projectAlmSettingDto))
Expand All @@ -182,7 +186,7 @@ public void shouldThrowErrorWhenGitlabMergeRequestDiscussionRetrievalFails() thr
}

@Test
public void shouldCloseDiscussionWithSingleResolvableNoteFromSonarqubeUserButNoIssueIdInBody() throws IOException {
void shouldCloseDiscussionWithSingleResolvableNoteFromSonarqubeUserButNoIssueIdInBody() throws IOException {
Note note = mock(Note.class);
when(note.getAuthor()).thenReturn(sonarqubeUser);
when(note.getBody()).thenReturn("Post with no issue ID");
Expand All @@ -203,7 +207,7 @@ public void shouldCloseDiscussionWithSingleResolvableNoteFromSonarqubeUserButNoI
assertThat(mergeRequestNoteArgumentCaptor.getValue()).isNotInstanceOf(CommitNote.class); }

@Test
public void shouldNotCloseDiscussionWithSingleNonResolvableNoteFromSonarqubeUserButNoIssueIdInBody() throws IOException {
void shouldNotCloseDiscussionWithSingleNonResolvableNoteFromSonarqubeUserButNoIssueIdInBody() throws IOException {
Note note = mock(Note.class);
when(note.getAuthor()).thenReturn(sonarqubeUser);
when(note.getBody()).thenReturn("Post with no issue ID");
Expand All @@ -221,7 +225,7 @@ public void shouldNotCloseDiscussionWithSingleNonResolvableNoteFromSonarqubeUser
}

@Test
public void shouldNotCloseDiscussionWithMultipleResolvableNotesFromSonarqubeUserButNoId() throws IOException {
void shouldNotCloseDiscussionWithMultipleResolvableNotesFromSonarqubeUserButNoId() throws IOException {
Note note = mock(Note.class);
when(note.getAuthor()).thenReturn(sonarqubeUser);
when(note.getBody()).thenReturn("Another post with no issue ID\nbut containing a new line");
Expand Down Expand Up @@ -249,7 +253,7 @@ public void shouldNotCloseDiscussionWithMultipleResolvableNotesFromSonarqubeUser
}

@Test
public void shouldCloseDiscussionWithResolvableNoteFromSonarqubeUserAndOnlySystemNoteFromOtherUser() throws IOException {
void shouldCloseDiscussionWithResolvableNoteFromSonarqubeUserAndOnlySystemNoteFromOtherUser() throws IOException {
User otherUser = mock(User.class);
when(otherUser.getUsername()).thenReturn("[email protected]");

Expand Down Expand Up @@ -279,7 +283,7 @@ public void shouldCloseDiscussionWithResolvableNoteFromSonarqubeUserAndOnlySyste
}

@Test
public void shouldNotAttemptCloseOfDiscussionWithMultipleResolvableNotesFromSonarqubeUserAndAnotherUserWithNoId() throws IOException {
void shouldNotAttemptCloseOfDiscussionWithMultipleResolvableNotesFromSonarqubeUserAndAnotherUserWithNoId() throws IOException {
User otherUser = mock(User.class);
when(otherUser.getUsername()).thenReturn("[email protected]");

Expand Down Expand Up @@ -310,7 +314,7 @@ public void shouldNotAttemptCloseOfDiscussionWithMultipleResolvableNotesFromSona
}

@Test
public void shouldNotCommentOrAttemptCloseOfDiscussionWithMultipleResolvableNotesFromSonarqubeUserAndACloseMessageWithNoId() throws IOException {
void shouldNotCommentOrAttemptCloseOfDiscussionWithMultipleResolvableNotesFromSonarqubeUserAndACloseMessageWithNoId() throws IOException {
Note note = mock(Note.class);
when(note.getAuthor()).thenReturn(sonarqubeUser);
when(note.getBody()).thenReturn("And another post with no issue ID\nNo View in SonarQube link");
Expand Down Expand Up @@ -339,7 +343,7 @@ public void shouldNotCommentOrAttemptCloseOfDiscussionWithMultipleResolvableNote
}

@Test
public void shouldCommentAboutCloseOfDiscussionWithMultipleResolvableNotesFromSonarqubeUserAndAnotherUserWithIssuedId() throws IOException {
void shouldCommentAboutCloseOfDiscussionWithMultipleResolvableNotesFromSonarqubeUserAndAnotherUserWithIssuedId() throws IOException {
User otherUser = mock(User.class);
when(otherUser.getUsername()).thenReturn("[email protected]");

Expand Down Expand Up @@ -371,7 +375,7 @@ public void shouldCommentAboutCloseOfDiscussionWithMultipleResolvableNotesFromSo
}

@Test
public void shouldThrowErrorIfUnableToCleanUpDiscussionOnGitlab() throws IOException {
void shouldThrowErrorIfUnableToCleanUpDiscussionOnGitlab() throws IOException {
User otherUser = mock(User.class);
when(otherUser.getUsername()).thenReturn("[email protected]");

Expand Down Expand Up @@ -406,7 +410,7 @@ public void shouldThrowErrorIfUnableToCleanUpDiscussionOnGitlab() throws IOExcep
}

@Test
public void shouldNotCommentOrAttemptCloseOfDiscussionWithMultipleResolvableNotesFromSonarqubeUserAndACloseMessageWithIssueId() throws IOException {
void shouldNotCommentOrAttemptCloseOfDiscussionWithMultipleResolvableNotesFromSonarqubeUserAndACloseMessageWithIssueId() throws IOException {
Note note = mock(Note.class);
when(note.getAuthor()).thenReturn(sonarqubeUser);
when(note.getBody()).thenReturn("And another post with an issue ID\n[View in SonarQube](url)");
Expand Down Expand Up @@ -435,7 +439,7 @@ public void shouldNotCommentOrAttemptCloseOfDiscussionWithMultipleResolvableNote
}

@Test
public void shouldNotCommentOrAttemptCloseOfDiscussionWithMultipleResolvableNotesFromSonarqubeWithOtherProjectId() throws IOException {
void shouldNotCommentOrAttemptCloseOfDiscussionWithMultipleResolvableNotesFromSonarqubeWithOtherProjectId() throws IOException {
Note note = mock(Note.class);
when(note.getAuthor()).thenReturn(sonarqubeUser);
when(note.getBody()).thenReturn("And another post with an issue ID\n[View in SonarQube](url)");
Expand Down Expand Up @@ -464,7 +468,7 @@ public void shouldNotCommentOrAttemptCloseOfDiscussionWithMultipleResolvableNote
}

@Test
public void shouldThrowErrorIfSubmittingNewIssueToGitlabFails() throws IOException {
void shouldThrowErrorIfSubmittingNewIssueToGitlabFails() throws IOException {
PostAnalysisIssueVisitor.LightIssue lightIssue = mock(PostAnalysisIssueVisitor.LightIssue.class);
when(lightIssue.key()).thenReturn("issueKey1");
when(lightIssue.getStatus()).thenReturn(Issue.STATUS_OPEN);
Expand Down Expand Up @@ -506,7 +510,7 @@ public void shouldThrowErrorIfSubmittingNewIssueToGitlabFails() throws IOExcepti
}

@Test
public void shouldStartNewDiscussionForNewIssueFromCommitInMergeRequest() throws IOException {
void shouldStartNewDiscussionForNewIssueFromCommitInMergeRequest() throws IOException {
PostAnalysisIssueVisitor.LightIssue lightIssue = mock(PostAnalysisIssueVisitor.LightIssue.class);
when(lightIssue.key()).thenReturn("issueKey1");
when(lightIssue.getStatus()).thenReturn(Issue.STATUS_OPEN);
Expand Down Expand Up @@ -545,7 +549,7 @@ public void shouldStartNewDiscussionForNewIssueFromCommitInMergeRequest() throws
}

@Test
public void shouldNotStartNewDiscussionForIssueWithExistingCommentFromCommitInMergeRequest() throws IOException {
void shouldNotStartNewDiscussionForIssueWithExistingCommentFromCommitInMergeRequest() throws IOException {
PostAnalysisIssueVisitor.LightIssue lightIssue = mock(PostAnalysisIssueVisitor.LightIssue.class);
when(lightIssue.key()).thenReturn("issueKey1");
when(lightIssue.getStatus()).thenReturn(Issue.STATUS_OPEN);
Expand Down Expand Up @@ -590,7 +594,7 @@ public void shouldNotStartNewDiscussionForIssueWithExistingCommentFromCommitInMe
}

@Test
public void shouldNotCreateCommentsForIssuesWithNoLineNumbers() throws IOException {
void shouldNotCreateCommentsForIssuesWithNoLineNumbers() throws IOException {
PostAnalysisIssueVisitor.LightIssue lightIssue = mock(PostAnalysisIssueVisitor.LightIssue.class);
when(lightIssue.key()).thenReturn("issueKey1");
when(lightIssue.getStatus()).thenReturn(Issue.STATUS_OPEN);
Expand Down Expand Up @@ -618,7 +622,7 @@ public void shouldNotCreateCommentsForIssuesWithNoLineNumbers() throws IOExcepti
}

@Test
public void shouldSubmitSuccessfulPipelineStatusAndResolvedSummaryCommentOnSuccessAnalysis() throws IOException {
void shouldSubmitSuccessfulPipelineStatusAndResolvedSummaryCommentOnSuccessAnalysis() throws IOException {
when(analysisDetails.getQualityGateStatus()).thenReturn(QualityGate.Status.OK);
when(analysisDetails.getCommitSha()).thenReturn("commitsha");

Expand Down Expand Up @@ -646,11 +650,15 @@ public void shouldSubmitSuccessfulPipelineStatusAndResolvedSummaryCommentOnSucce
PipelineStatus.State.SUCCESS, "https://sonarqube.dummy/dashboard?id=" + PROJECT_KEY + "&pullRequest=" + MERGE_REQUEST_IID, null, null));
}

@Test
public void shouldSubmitFailedPipelineStatusAndUnresolvedSummaryCommentOnFailedAnalysis() throws IOException {
@SuppressWarnings({"OptionalUsedAsFieldOrParameterType", "unused"})
@MethodSource
@ParameterizedTest //https://github.com/mc1arke/sonarqube-community-branch-plugin/issues/137
void shouldSubmitRequestedPipelineStatusBasedOnPropertiesAndUnresolvedSummaryCommentOnFailedAnalysis(
Optional<String> dontFailPipelinePropertyValue, PipelineStatus.State expectedSentPipelineStatus, String description) throws IOException {
when(analysisDetails.getQualityGateStatus()).thenReturn(QualityGate.Status.ERROR);
when(analysisDetails.getCommitSha()).thenReturn("other sha");
when(analysisDetails.getScannerProperty("com.github.mc1arke.sonarqube.plugin.branch.pullrequest.gitlab.pipelineId")).thenReturn(Optional.of("11"));
when(analysisDetails.getScannerProperty("sonar.analysis.com.github.mc1arke.sonarqube.plugin.branch.pullrequest.gitlab.dontFailPipeline")).thenReturn(dontFailPipelinePropertyValue);

when(analysisSummary.format(any())).thenReturn("Different Summary comment");
when(analysisSummary.getDashboardUrl()).thenReturn("https://sonarqube2.dummy/dashboard?id=projectKey&pullRequest=123");
Expand All @@ -674,11 +682,20 @@ public void shouldSubmitFailedPipelineStatusAndUnresolvedSummaryCommentOnFailedA
assertThat(pipelineStatusArgumentCaptor.getValue())
.usingRecursiveComparison()
.isEqualTo(new PipelineStatus("SonarQube", "SonarQube Status",
PipelineStatus.State.FAILED, "https://sonarqube2.dummy/dashboard?id=" + PROJECT_KEY + "&pullRequest=" + MERGE_REQUEST_IID, BigDecimal.TEN, 11L));
expectedSentPipelineStatus, "https://sonarqube2.dummy/dashboard?id=" + PROJECT_KEY + "&pullRequest=" + MERGE_REQUEST_IID, BigDecimal.TEN, 11L));
}

@SuppressWarnings("unused") //used by @ParameterizedTest
private static Stream<Arguments> shouldSubmitRequestedPipelineStatusBasedOnPropertiesAndUnresolvedSummaryCommentOnFailedAnalysis() {
return Stream.of(
Arguments.of(Optional.empty(), PipelineStatus.State.FAILED, "dontFailPipeline not defined"),
Arguments.of(Optional.of("true"), PipelineStatus.State.SUCCESS, "dontFailPipeline == true"),
Arguments.of(Optional.of("false"), PipelineStatus.State.FAILED, "dontFailPipeline == false")
);
}

@Test
public void shouldThrowErrorWhenSubmitPipelineStatusToGitlabFails() throws IOException {
void shouldThrowErrorWhenSubmitPipelineStatusToGitlabFails() throws IOException {
when(analysisDetails.getQualityGateStatus()).thenReturn(QualityGate.Status.ERROR);
when(analysisDetails.getCommitSha()).thenReturn("other sha");
when(analysisDetails.getScannerProperty("com.github.mc1arke.sonarqube.plugin.branch.pullrequest.gitlab.pipelineId")).thenReturn(Optional.of("11"));
Expand Down Expand Up @@ -712,7 +729,7 @@ public void shouldThrowErrorWhenSubmitPipelineStatusToGitlabFails() throws IOExc
}

@Test
public void shouldThrowErrorWhenSubmitAnalysisToGitlabFails() throws IOException {
void shouldThrowErrorWhenSubmitAnalysisToGitlabFails() throws IOException {
when(analysisDetails.getQualityGateStatus()).thenReturn(QualityGate.Status.ERROR);
when(analysisDetails.getCommitSha()).thenReturn("other sha");
when(analysisDetails.getScannerProperty("com.github.mc1arke.sonarqube.plugin.branch.pullrequest.gitlab.pipelineId")).thenReturn(Optional.of("11"));
Expand All @@ -739,14 +756,14 @@ public void shouldThrowErrorWhenSubmitAnalysisToGitlabFails() throws IOException
}

@Test
public void shouldReturnWebUrlFromMergeRequestIfScannerPropertyNotSet() {
void shouldReturnWebUrlFromMergeRequestIfScannerPropertyNotSet() {
assertThat(underTest.decorateQualityGateStatus(analysisDetails, almSettingDto, projectAlmSettingDto))
.usingRecursiveComparison()
.isEqualTo(DecorationResult.builder().withPullRequestUrl(MERGE_REQUEST_WEB_URL).build());
}

@Test
public void shouldReturnWebUrlFromScannerPropertyIfSet() {
void shouldReturnWebUrlFromScannerPropertyIfSet() {
when(analysisDetails.getScannerProperty("sonar.pullrequest.gitlab.projectUrl")).thenReturn(Optional.of(MERGE_REQUEST_WEB_URL + "/additional"));
assertThat(underTest.decorateQualityGateStatus(analysisDetails, almSettingDto, projectAlmSettingDto))
.usingRecursiveComparison()
Expand Down