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

Try resolve commit from run first #27

Closed
wants to merge 1 commit into from
+83 −7
Closed

Try resolve commit from run first #27

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
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
37 changes: 31 additions & 6 deletions src/main/java/io/jenkins/plugins/checks/github/GitHubChecksContext.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,6 +7,7 @@
import jenkins.plugins.git.AbstractGitSCMSource;
import jenkins.scm.api.SCMHead;
import jenkins.scm.api.SCMRevision;
import org.apache.commons.lang3.StringUtils;
import org.jenkinsci.plugins.displayurlapi.DisplayURLProvider;
import org.jenkinsci.plugins.github_branch_source.GitHubAppCredentials;
import org.jenkinsci.plugins.github_branch_source.GitHubSCMSource;
Expand Down Expand Up @@ -75,7 +76,13 @@ class GitHubChecksContext {
* @return the commit sha of the run or null
Copy link
Member

Choose a reason for hiding this comment

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

If null is allowed (which I think is a bad choice) then we must mark the method with Nullable

*/
public String getHeadSha() {
return resolveHeadSha();
String commit = resolveCommit();
if (StringUtils.isEmpty(commit)) {
return resolveHeadSha();
}
else {
return commit;
}
}

/**
Expand Down Expand Up @@ -150,14 +157,32 @@ private String resolveHeadSha() {
getRepository(), head.get().getName()));
}

if (revision.get() instanceof AbstractGitSCMSource.SCMRevisionImpl) {
return ((AbstractGitSCMSource.SCMRevisionImpl) revision.get()).getHash();
return resolveCommit(revision.get());
}

private String resolveCommit() {
if (run == null) {
Copy link
Member

Choose a reason for hiding this comment

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

In my refactored version run is not a field anymore and thus cannot be null. Rather than setting run to null we should immediately initialize all fields in the constructor. Then we do not have any null checks for run anymore.

I'm not sure if I did understand the GitHub REST API correctly: is it valid to call it without a SHA? If not then we should not create a ChecksPublisher. This is the way I did it with Freestyle jobs and pipelines.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The reason we introduce the nullable run here is that when the job is still in the queue, the run is not available, so we can't get the url of the run (see #13 (comment)) and will return the url of the job view instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't find a better way to avoid nullable run, but maybe we can add another boolean field isLastBuild when constructing the context to indicate whether the run is the last build? So when isLastBuild == true, we get run by job.getLastBuild(), when isLastBuild == false, we use the run that was passed in the constructor.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I saw your refactoring on this problem, simple but great! Ignore the previous two comments :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not sure if I did understand the GitHub REST API correctly: is it valid to call it without a SHA? If not then we should not create a ChecksPublisher. This is the way I did it with Freestyle jobs and pipelines.

The SHA is required for a check, without it, the check cannot be created successfully.

return StringUtils.EMPTY;
Copy link
Member

Choose a reason for hiding this comment

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

returning an empty string is usually a codesmell, just return null if you can't find it, imo

Suggested change
return StringUtils.EMPTY;
return null;

Copy link
Member

Choose a reason for hiding this comment

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

Then we need to guard all methods that invoke the getSha.

Copy link
Member

Choose a reason for hiding this comment

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

This method is only called internally from one place?

}

Optional<SCMRevision> revision = scmFacade.findRevision(resolveSource(), run);
if (revision.isPresent()) {
return resolveCommit(revision.get());
}
else {
return StringUtils.EMPTY;
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
return StringUtils.EMPTY;
return null;

}
}

private String resolveCommit(final SCMRevision revision) {
if (revision instanceof AbstractGitSCMSource.SCMRevisionImpl) {
return ((AbstractGitSCMSource.SCMRevisionImpl) revision).getHash();
}
else if (revision.get() instanceof PullRequestSCMRevision) {
return ((PullRequestSCMRevision) revision.get()).getPullHash();
else if (revision instanceof PullRequestSCMRevision) {
return ((PullRequestSCMRevision) revision).getPullHash();
}
else {
throw new IllegalStateException("Unsupported revision type: " + revision.get().getClass().getName());
throw new IllegalStateException("Unsupported revision type: " + revision.getClass().getName());
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we catch that error before we create the publisher and return then a NullPublisher?

}
}
}
18 changes: 17 additions & 1 deletion src/main/java/io/jenkins/plugins/checks/github/GitHubSCMFacade.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@
import com.cloudbees.plugins.credentials.CredentialsMatchers;
import com.cloudbees.plugins.credentials.CredentialsProvider;
import hudson.model.Job;
import hudson.model.Run;
import hudson.security.ACL;
import jenkins.scm.api.SCMHead;
import jenkins.scm.api.SCMRevision;
import jenkins.scm.api.SCMRevisionAction;
import jenkins.scm.api.SCMSource;
import org.jenkinsci.plugins.github_branch_source.GitHubAppCredentials;
import org.jenkinsci.plugins.github_branch_source.GitHubSCMSource;
Expand Down Expand Up @@ -61,7 +63,7 @@ public Optional<SCMHead> findHead(final Job<?, ?> job) {
}

/**
* Fetch the current {@link SCMRevision} used by the {@code head} of the {@code source}.
* Fetch the current {@link SCMRevision} of the {@code source} and {@code head} remotely from GitHub.
*
* @param source
* the GitHub repository
Expand All @@ -78,4 +80,18 @@ public Optional<SCMRevision> findRevision(final GitHubSCMSource source, final SC
source.getRepoOwner() + "/" + source.getRepository(), head.getName()), e);
}
}

/**
* Find the current {@link SCMRevision} of the {@code source} and {@code run} locally through
* {@link jenkins.scm.api.SCMRevisionAction}.
*
* @param source
* the GitHub repository
* @param run
* the Jenkins run
* @return the found revision or empty
*/
public Optional<SCMRevision> findRevision(final GitHubSCMSource source, final Run<?, ?> run) {
return Optional.ofNullable(SCMRevisionAction.getRevision(source, run));
}
}
35 changes: 35 additions & 0 deletions src/test/java/io/jenkins/plugins/checks/github/GitHubChecksContextTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,41 @@
import static org.mockito.Mockito.when;

class GitHubChecksContextTest {
@Test
void shouldGetHeadShaWhenResolveRevisionFromRun() {
Job job = mock(Job.class);
Run run = mock(Run.class);
PullRequestSCMRevision revision = mock(PullRequestSCMRevision.class);
GitHubSCMSource source = mock(GitHubSCMSource.class);
GitHubSCMFacade scmFacade = mock(GitHubSCMFacade.class);

when(run.getParent()).thenReturn(job);
when(scmFacade.findGitHubSCMSource(job)).thenReturn(Optional.of(source));
when(scmFacade.findRevision(source, run)).thenReturn(Optional.of(revision));
when(revision.getPullHash()).thenReturn("a1b2c3");

assertThat(new GitHubChecksContext(run, scmFacade, null)
.getHeadSha())
.isEqualTo("a1b2c3");
}

@Test
void shouldGetHeadShaWhenResolveRevisionFromHead() {
Job job = mock(Job.class);
Run run = mock(Run.class);
SCMHead head = mock(SCMHead.class);
PullRequestSCMRevision revision = mock(PullRequestSCMRevision.class);
GitHubSCMSource source = mock(GitHubSCMSource.class);
GitHubSCMFacade scmFacade = createGitHubSCMFacadeWithRevision(job, source, head, revision);

when(run.getParent()).thenReturn(job);
when(revision.getPullHash()).thenReturn("a1b2c3");

assertThat(new GitHubChecksContext(run, scmFacade, null)
.getHeadSha())
.isEqualTo("a1b2c3");
}

@Test
void shouldGetHeadShaFromMasterBranch() {
Job job = mock(Job.class);
Expand Down