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.

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

Fix missing gitlabUserUsername definition for MR and Note triggers #1469

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 5 commits
Commits
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
Original file line number Diff line number Diff line change
Expand Up @@ -198,6 +198,8 @@ protected CauseData retrieveCauseData(MergeRequestHook hook) {
.withSourceBranch(hook.getObjectAttributes().getSourceBranch())
.withUserName(
hook.getObjectAttributes().getLastCommit().getAuthor().getName())
.withUserUsername(
hook.getObjectAttributes().getLastCommit().getAuthor().getUsername())
.withUserEmail(
hook.getObjectAttributes().getLastCommit().getAuthor().getEmail())
.withSourceRepoHomepage(hook.getObjectAttributes().getSource().getHomepage())
Expand Down Expand Up @@ -360,10 +362,10 @@ private boolean isForcedByAddedLabel(MergeRequestHook hook) {
changedLabels.getPrevious() != null ? changedLabels.getPrevious() : emptyList();

return current.stream()
.filter(currentLabel -> !previous.stream()
.anyMatch(previousLabel -> Objects.equals(currentLabel.getId(), previousLabel.getId())))
.map(label -> label.getTitle())
.anyMatch(label -> labelsThatForcesBuildIfAdded.contains(label));
.filter(currentLabel -> previous.stream()
.noneMatch(previousLabel -> Objects.equals(currentLabel.getId(), previousLabel.getId())))
.map(MergeRequestLabel::getTitle)
.anyMatch(labelsThatForcesBuildIfAdded::contains);
}

private boolean isNotSkipWorkInProgressMergeRequest(MergeRequestObjectAttributes objectAttributes) {
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -72,6 +72,8 @@ protected CauseData retrieveCauseData(NoteHook hook) {
.withBranch(hook.getMergeRequest().getSourceBranch())
.withSourceBranch(hook.getMergeRequest().getSourceBranch())
.withUserName(hook.getMergeRequest().getLastCommit().getAuthor().getName())
.withUserUsername(
hook.getMergeRequest().getLastCommit().getAuthor().getUsername())
.withUserEmail(
hook.getMergeRequest().getLastCommit().getAuthor().getEmail())
.withSourceRepoHomepage(hook.getMergeRequest().getSource().getHomepage())
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,6 @@
import com.dabsquared.gitlabjenkins.gitlab.hook.model.Repository;
import com.dabsquared.gitlabjenkins.gitlab.hook.model.State;
import com.dabsquared.gitlabjenkins.gitlab.hook.model.User;
import com.dabsquared.gitlabjenkins.gitlab.hook.model.builder.generated.CommitBuilder;
import com.dabsquared.gitlabjenkins.gitlab.hook.model.builder.generated.MergeRequestHookBuilder;
import com.dabsquared.gitlabjenkins.gitlab.hook.model.builder.generated.ProjectBuilder;
import com.dabsquared.gitlabjenkins.gitlab.hook.model.builder.generated.PushHookBuilder;
Expand All @@ -36,7 +35,7 @@
import hudson.model.Queue;
import java.io.File;
import java.io.IOException;
import java.util.Arrays;
import java.util.Collections;
import org.jenkinsci.plugins.workflow.job.WorkflowJob;
import org.junit.After;
import org.junit.Before;
Expand Down Expand Up @@ -76,7 +75,7 @@ public void clearQueue() {

@Test
public void projectCanBeConfiguredToSendPendingBuildStatusWhenTriggered() throws IOException {
Project project =
Project<?, ?> project =
freestyleProject("freestyleProject1", new GitLabCommitStatusPublisher(GITLAB_BUILD_NAME, false));

GitLabPushTrigger gitLabPushTrigger = gitLabPushTrigger(project);
Expand Down Expand Up @@ -118,7 +117,7 @@ public void workflowJobCanConfiguredToSendToPendingBuildStatusWhenTriggered() th

@Test
public void queuedMergeRequestBuildsCanBeCancelledOnMergeRequestUpdate() throws IOException {
Project project = freestyleProject("project1", new GitLabCommitStatusPublisher(GITLAB_BUILD_NAME, false));
Project<?, ?> project = freestyleProject("project1", new GitLabCommitStatusPublisher(GITLAB_BUILD_NAME, false));

GitLabPushTrigger gitLabPushTrigger = gitLabPushTrigger(project);
gitLabPushTrigger.setCancelPendingBuildsOnUpdate(true);
Expand Down Expand Up @@ -153,7 +152,7 @@ public void queuedMergeRequestBuildsCanBeCancelledOnMergeRequestUpdate() throws
assertThat(jenkins.getInstance().getQueue().getItems().length, is(3));
}

private GitLabPushTrigger gitLabPushTrigger(Project project) throws IOException {
private GitLabPushTrigger gitLabPushTrigger(Project<?, ?> project) throws IOException {
GitLabPushTrigger gitLabPushTrigger = gitLabPushTrigger();
project.addTrigger(gitLabPushTrigger);
gitLabPushTrigger.start(project, true);
Expand Down Expand Up @@ -212,6 +211,13 @@ private MergeRequestHook mergeRequestHook(int projectId, String branch, String c
.withProject(ProjectBuilder.project()
.withWebUrl("https://gitlab.org/test.git")
.build())
.withUser(user().withId(1)
.withName("User")
.withUsername("user")
.withEmail("[email protected]")
.withAvatarUrl(
"https://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon")
.build())
.build();
}

Expand All @@ -230,15 +236,17 @@ private PushHook pushHook(int projectId, String branch, String commitId) {
.withAfter(commitId)
.withRepository(new Repository())
.withProject(ProjectBuilder.project().withNamespace("namespace").build())
.withCommits(Arrays.asList(
CommitBuilder.commit().withId(commitId).withAuthor(user).build()))
.withCommits(Collections.singletonList(
commit().withId(commitId).withAuthor(user).build()))
.withRepository(repository)
.withObjectKind("push")
.withUserName("username")
.withUserName("User")
.withUserUsername("username")
.withUserEmail("[email protected]")
Comment on lines +243 to +245
Copy link
Member

Choose a reason for hiding this comment

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

I think these are the tests we need for this PR I guess, which I have added/updated.

.build();
}

private Project freestyleProject(String name, GitLabCommitStatusPublisher gitLabCommitStatusPublisher)
private Project<?, ?> freestyleProject(String name, GitLabCommitStatusPublisher gitLabCommitStatusPublisher)
throws IOException {
FreeStyleProject project = jenkins.createFreeStyleProject(name);
project.setQuietPeriod(5000);
Expand All @@ -248,7 +256,7 @@ private Project freestyleProject(String name, GitLabCommitStatusPublisher gitLab
}

private WorkflowJob workflowJob() throws IOException {
ItemGroup itemGroup = mock(ItemGroup.class);
ItemGroup<WorkflowJob> itemGroup = mock(ItemGroup.class);
when(itemGroup.getFullName()).thenReturn("parent");
when(itemGroup.getUrlChildPrefix()).thenReturn("prefix");

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@
import hudson.util.OneShotEvent;
import java.io.IOException;
import java.util.Arrays;
import java.util.List;
import java.util.concurrent.ExecutionException;
import org.eclipse.jgit.api.Git;
import org.eclipse.jgit.api.errors.GitAPIException;
Expand Down Expand Up @@ -429,6 +430,13 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
.withProject(project()
.withWebUrl("https://gitlab.org/test.git")
.build())
.withUser(user().withId(1)
.withName("User")
.withUsername("user")
.withEmail("[email protected]")
.withAvatarUrl(
"https://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon")
.build())
.build(),
true,
BranchFilterFactory.newBranchFilter(branchFilterConfig().build(BranchFilterType.All)),
Expand All @@ -452,6 +460,13 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
.withProject(project()
.withWebUrl("https://gitlab.org/test.git")
.build())
.withUser(user().withId(1)
.withName("User")
.withUsername("user")
.withEmail("[email protected]")
.withAvatarUrl(
"https://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon")
.build())
.build(),
true,
BranchFilterFactory.newBranchFilter(branchFilterConfig().build(BranchFilterType.All)),
Expand Down Expand Up @@ -541,6 +556,13 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
.withProject(project()
.withWebUrl("https://gitlab.org/test.git")
.build())
.withUser(user().withId(1)
.withName("User")
.withUsername("user")
.withEmail("[email protected]")
.withAvatarUrl(
"https://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon")
.build())
.build(),
true,
BranchFilterFactory.newBranchFilter(branchFilterConfig().build(BranchFilterType.All)),
Expand All @@ -564,7 +586,7 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
});
project.setQuietPeriod(0);
MergeRequestHookTriggerHandler mergeRequestHookTriggerHandler = new MergeRequestHookTriggerHandlerImpl(
Arrays.asList(State.opened, State.reopened), Arrays.asList(Action.approved), false, false, false);
Arrays.asList(State.opened, State.reopened), List.of(Action.approved), false, false, false);
mergeRequestHookTriggerHandler.handle(
project,
mergeRequestHook()
Expand All @@ -575,6 +597,13 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
.withId("testid")
.build())
.build())
.withUser(user().withId(1)
.withName("User")
.withUsername("user")
.withEmail("[email protected]")
.withAvatarUrl(
"https://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon")
.build())
.build(),
true,
BranchFilterFactory.newBranchFilter(branchFilterConfig().build(BranchFilterType.All)),
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -159,6 +159,13 @@ public boolean perform(AbstractBuild<?, ?> build, Launcher launcher, BuildListen
.withWebUrl("https://gitlab.org/test.git")
.build())
.build())
.withUser(user().withId(1)
.withName("User")
.withUsername("user")
.withEmail("[email protected]")
.withAvatarUrl(
"https://www.gravatar.com/avatar/e64c7d89f26bd1972efa854d13d7dd61?s=80&d=identicon")
.build())
.build(),
true,
BranchFilterFactory.newBranchFilter(branchFilterConfig().build(BranchFilterType.All)),
Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
package com.dabsquared.gitlabjenkins.webhook.build;

import static com.dabsquared.gitlabjenkins.cause.CauseDataBuilder.causeData;
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.notNullValue;
Expand Down Expand Up @@ -107,6 +108,9 @@ public void build() throws IOException {
verify(mockTrigger).onPost(pushHookArgumentCaptor.capture());
assertThat(pushHookArgumentCaptor.getValue().getProject(), is(notNullValue()));
assertThat(pushHookArgumentCaptor.getValue().getProject().getWebUrl(), is(notNullValue()));
assertThat(pushHookArgumentCaptor.getValue().getUser(), is(notNullValue()));
assertThat(pushHookArgumentCaptor.getValue().getUser().getName(), containsString("Administrator"));
assertThat(pushHookArgumentCaptor.getValue().getUser().getUsername(), containsString("root"));
krisstern marked this conversation as resolved.
Show resolved Hide resolved
Copy link
Member

Choose a reason for hiding this comment

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

Actually this only checks for when the User's name contains "Administrator" and the User's username is "root" by design. But there should be no harm done by adding this code snippet.

}
}

Expand Down
Original file line number Diff line number Diff line change
@@ -1,6 +1,10 @@
package com.dabsquared.gitlabjenkins.webhook.build;

import static com.dabsquared.gitlabjenkins.cause.CauseDataBuilder.causeData;
import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.MatcherAssert.assertThat;
import static org.hamcrest.Matchers.is;
import static org.hamcrest.Matchers.notNullValue;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.Mockito.verify;

Expand All @@ -14,6 +18,8 @@
import hudson.model.queue.QueueTaskFuture;
import hudson.plugins.git.GitSCM;
import java.io.IOException;
import java.nio.charset.StandardCharsets;
import java.util.Objects;
import java.util.concurrent.ExecutionException;
import org.apache.commons.io.IOUtils;
import org.eclipse.jgit.api.Git;
Expand All @@ -28,6 +34,7 @@
import org.jvnet.hudson.test.JenkinsRule;
import org.kohsuke.stapler.HttpResponses;
import org.kohsuke.stapler.StaplerResponse;
import org.mockito.ArgumentCaptor;
import org.mockito.Mock;
import org.mockito.junit.MockitoJUnitRunner;

Expand Down Expand Up @@ -68,13 +75,19 @@ public void setup() throws Exception {

@Test
public void build() throws IOException {
FreeStyleProject testProject = jenkins.createFreeStyleProject();
testProject.addTrigger(trigger);

exception.expect(HttpResponses.HttpResponseException.class);
new NoteBuildAction(testProject, getJson("NoteEvent.json"), null).execute(response);

verify(trigger).onPost(any(NoteHook.class));
try {
FreeStyleProject testProject = jenkins.createFreeStyleProject();
testProject.addTrigger(trigger);

exception.expect(HttpResponses.HttpResponseException.class);
new NoteBuildAction(testProject, getJson("NoteEvent.json"), null).execute(response);
} finally {
ArgumentCaptor<NoteHook> noteHookArgumentCaptor = ArgumentCaptor.forClass(NoteHook.class);
verify(trigger).onPost(noteHookArgumentCaptor.capture());
assertThat(noteHookArgumentCaptor.getValue().getUser(), is(notNullValue()));
assertThat(noteHookArgumentCaptor.getValue().getUser().getName(), containsString("Administrator"));
assertThat(noteHookArgumentCaptor.getValue().getUser().getUsername(), containsString("root"));
krisstern marked this conversation as resolved.
Show resolved Hide resolved
}
}

@Test
Expand All @@ -84,7 +97,9 @@ public void build_alreadyBuiltMR_alreadyBuiltMR() throws IOException, ExecutionE
testProject.setScm(new GitSCM(gitRepoUrl));
QueueTaskFuture<?> future = testProject.scheduleBuild2(
0, new ParametersAction(new StringParameterValue("gitlabTargetBranch", "master")));
future.get();
if (future != null) {
future.get();
}

exception.expect(HttpResponses.HttpResponseException.class);
new NoteBuildAction(testProject, getJson("NoteEvent_alreadyBuiltMR.json"), null).execute(response);
Expand Down Expand Up @@ -134,6 +149,7 @@ public void build_alreadyBuiltMR_differentTargetBranch()
}

private String getJson(String name) throws IOException {
return IOUtils.toString(getClass().getResourceAsStream(name)).replace("${commitSha1}", commitSha1);
return IOUtils.toString(Objects.requireNonNull(getClass().getResourceAsStream(name)), StandardCharsets.UTF_8)
.replace("${commitSha1}", commitSha1);
}
}