Skip to content

Commit

Permalink
Merge pull request #52 from VijayKumarMidde/master
Browse files Browse the repository at this point in the history 
Avoid infinite sleep loop if multiple fork names come back from GitHub API
  • Loading branch information
@justinharringa
justinharringa authored Jun 3, 2019
2 parents 51a6d6b + 6615b4c commit a6e1ce7
Show file tree
Hide file tree
Showing 4 changed files with 122 additions and 18 deletions.
16 changes: 8 additions & 8 deletions ...image-update/src/main/java/com/salesforce/dockerfileimageupdate/subcommands/impl/All.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -95,7 +95,7 @@ protected void forkRepositoriesFound(Multimap<String, String> pathToDockerfilesI
PagedSearchIterable<GHContent> contentsWithImage,
String image) throws IOException {
log.info("Forking {} repositories...", contentsWithImage.getTotalCount());
List<String> parentReposAlreadyChecked = new ArrayList<>();
List<String> parentReposForked = new ArrayList<>();
GHRepository parent;
String parentRepoName = null;
for (GHContent c : contentsWithImage) {
Expand All @@ -112,17 +112,17 @@ protected void forkRepositoriesFound(Multimap<String, String> pathToDockerfilesI
log.warn("Skipping {} because it's a fork already. Sending a PR to a fork is unsupported at the moment.",
parentRepoName);
} else {
pathToDockerfilesInParentRepo.put(parentRepoName, c.getPath());
imagesFoundInParentRepo.put(parentRepoName, image);

// fork the parent if not already forked or we couldn't fork
if (!parentReposAlreadyChecked.contains(parentRepoName)) {
// fork the parent if not already forked
if (parentReposForked.contains(parentRepoName) == false) {
GHRepository fork = dockerfileGitHubUtil.closeOutdatedPullRequestAndFork(parent);
if (fork == null) {
log.info("Could not fork {}", parentRepoName);
pathToDockerfilesInParentRepo.remove(parentRepoName, c.getPath());
} else {
// Add repos to pathToDockerfilesInParentRepo and imagesFoundInParentRepo only if we forked it successfully.
pathToDockerfilesInParentRepo.put(parentRepoName, c.getPath());
imagesFoundInParentRepo.put(parentRepoName, image);
parentReposForked.add(parentRepoName);
}
parentReposAlreadyChecked.add(parentRepoName);
}
}
}
Expand Down
13 changes: 7 additions & 6 deletions ...ge-update/src/main/java/com/salesforce/dockerfileimageupdate/subcommands/impl/Parent.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -110,7 +110,7 @@ protected PagedSearchIterable<GHContent> getGHContents(String org, String img)
protected Multimap<String, String> forkRepositoriesFoundAndGetPathToDockerfiles(PagedSearchIterable<GHContent> contentsWithImage) throws IOException {
log.info("Forking repositories...");
Multimap<String, String> pathToDockerfilesInParentRepo = HashMultimap.create();
List<String> parentReposAlreadyChecked = new ArrayList<>();
List<String> parentReposForked = new ArrayList<>();
GHRepository parent;
String parentRepoName = null;
for (GHContent c : contentsWithImage) {
Expand All @@ -127,16 +127,17 @@ protected Multimap<String, String> forkRepositoriesFoundAndGetPathToDockerfiles(
log.warn("Skipping {} because it's a fork already. Sending a PR to a fork is unsupported at the moment.",
parentRepoName);
} else {
pathToDockerfilesInParentRepo.put(parentRepoName, c.getPath());
// fork the parent if not already forked or we couldn't fork
if (!parentReposAlreadyChecked.contains(parentRepoName)) {
// fork the parent if not already forked
if (parentReposForked.contains(parentRepoName) == false) {
log.info("Forking {}", parentRepoName);
GHRepository fork = dockerfileGitHubUtil.closeOutdatedPullRequestAndFork(parent);
if (fork == null) {
log.info("Could not fork {}", parentRepoName);
pathToDockerfilesInParentRepo.remove(parentRepoName, c.getPath());
} else {
// Add repos to pathToDockerfilesInParentRepo only if we forked it successfully.
pathToDockerfilesInParentRepo.put(parentRepoName, c.getPath());
parentReposForked.add(parentRepoName);
}
parentReposAlreadyChecked.add(parentRepoName);
}
}
}
Expand Down
59 changes: 55 additions & 4 deletions ...e-update/src/test/java/com/salesforce/dockerfileimageupdate/subcommands/impl/AllTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -25,10 +25,9 @@
import java.util.Map;
import java.util.Set;

import static org.mockito.Matchers.any;
import static org.mockito.Matchers.anyString;
import static org.mockito.Matchers.eq;
import static org.mockito.Matchers.*;
import static org.mockito.Mockito.*;
import static org.testng.Assert.assertEquals;
import static org.testng.Assert.assertNotNull;

/**
Expand Down Expand Up @@ -61,7 +60,59 @@ public void testForkRepositoriesFound() throws Exception {
all.loadDockerfileGithubUtil(dockerfileGitHubUtil);
all.forkRepositoriesFound(ArrayListMultimap.create(), ArrayListMultimap.create(), contentsWithImage, "image");

Mockito.verify(dockerfileGitHubUtil, times(1)).closeOutdatedPullRequestAndFork(any());
Mockito.verify(dockerfileGitHubUtil, times(3)).closeOutdatedPullRequestAndFork(any());
}

@Test
public void testForkRepositoriesFound_unableToforkRepo() throws Exception {
/**
* Suppose we have multiple dockerfiles that need to updated in a repo and we fail to fork such repo,
* we should not add those repos to pathToDockerfilesInParentRepo.
*/
DockerfileGitHubUtil dockerfileGitHubUtil = mock(DockerfileGitHubUtil.class);

GHRepository contentRepo1 = mock(GHRepository.class);
when(contentRepo1.getFullName()).thenReturn("1");

GHRepository contentRepo2 = mock(GHRepository.class);
// Say we have multiple dockerfiles to be updated in repo "1"
when(contentRepo2.getFullName()).thenReturn("1");

GHRepository contentRepo3 = mock(GHRepository.class);
when(contentRepo3.getFullName()).thenReturn("2");

GHContent content1 = mock(GHContent.class);
when(content1.getOwner()).thenReturn(contentRepo1);
when(content1.getPath()).thenReturn("1"); // path to 1st dockerfile in repo "1"

GHContent content2 = mock(GHContent.class);
when(content2.getOwner()).thenReturn(contentRepo2);
when(content2.getPath()).thenReturn("2"); // path to 2st dockerfile in repo "1"

GHContent content3 = mock(GHContent.class);
when(content3.getOwner()).thenReturn(contentRepo3);
when(content3.getPath()).thenReturn("3");

PagedSearchIterable<GHContent> contentsWithImage = mock(PagedSearchIterable.class);

PagedIterator<GHContent> contentsWithImageIterator = mock(PagedIterator.class);
when(contentsWithImageIterator.hasNext()).thenReturn(true, true, true, false);
when(contentsWithImageIterator.next()).thenReturn(content1, content2, content3, null);
when(contentsWithImage.iterator()).thenReturn(contentsWithImageIterator);
when(dockerfileGitHubUtil.closeOutdatedPullRequestAndFork(contentRepo1)).thenReturn(null); // repo1 is unforkable
when(dockerfileGitHubUtil.closeOutdatedPullRequestAndFork(contentRepo2)).thenReturn(null); // repo1 is unforkable
when(dockerfileGitHubUtil.closeOutdatedPullRequestAndFork(contentRepo3)).thenReturn(new GHRepository());

All all = new All();
all.loadDockerfileGithubUtil(dockerfileGitHubUtil);
Multimap<String, String> pathToDockerfilesInParentRepo = ArrayListMultimap.create();
Multimap<String, String> imagesFoundInParentRepo = ArrayListMultimap.create();
all.forkRepositoriesFound(pathToDockerfilesInParentRepo, imagesFoundInParentRepo, contentsWithImage, "image");

// Since repo "1" is unforkable, we only added repo "2" to pathToDockerfilesInParentRepo
assertEquals(pathToDockerfilesInParentRepo.size(), 1);
assertEquals(imagesFoundInParentRepo.size(), 1);
Mockito.verify(dockerfileGitHubUtil, times(3)).closeOutdatedPullRequestAndFork(any());
}

@Test
Expand Down
52 changes: 52 additions & 0 deletions ...pdate/src/test/java/com/salesforce/dockerfileimageupdate/subcommands/impl/ParentTest.java
View file
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,58 @@ public void testForkRepositoriesFound() throws Exception {
assertEquals(repoMap.size(), 3);
}

@Test
public void forkRepositoriesFoundAndGetPathToDockerfiles_unableToforkRepo() throws Exception {
/**
* Suppose we have multiple dockerfiles that need to updated in a repo and we fail to fork such repo,
* we should not add those repos to pathToDockerfilesInParentRepo.
*
* Note: Sometimes GitHub search API returns the same result twice. This test covers such cases as well.
*/
DockerfileGitHubUtil dockerfileGitHubUtil = mock(DockerfileGitHubUtil.class);

GHRepository contentRepo1 = mock(GHRepository.class);
when(contentRepo1.getFullName()).thenReturn("1");

GHRepository duplicateContentRepo1 = mock(GHRepository.class);
// Say we have multiple dockerfiles to be updated in repo "1"
// Or sometimes GitHub search API returns same result twice.
when(duplicateContentRepo1.getFullName()).thenReturn("1");

GHRepository contentRepo2 = mock(GHRepository.class);
when(contentRepo2.getFullName()).thenReturn("2");

GHContent content1 = mock(GHContent.class);
when(content1.getOwner()).thenReturn(contentRepo1);
when(content1.getPath()).thenReturn("1"); // path to 1st dockerfile in repo "1"

GHContent content2 = mock(GHContent.class);
when(content2.getOwner()).thenReturn(duplicateContentRepo1);
when(content2.getPath()).thenReturn("2"); // path to 2st dockerfile in repo "1"

GHContent content3 = mock(GHContent.class);
when(content3.getOwner()).thenReturn(contentRepo2);
when(content3.getPath()).thenReturn("3");

PagedSearchIterable<GHContent> contentsWithImage = mock(PagedSearchIterable.class);

PagedIterator<GHContent> contentsWithImageIterator = mock(PagedIterator.class);
when(contentsWithImageIterator.hasNext()).thenReturn(true, true, true, false);
when(contentsWithImageIterator.next()).thenReturn(content1, content2, content3, null);
when(contentsWithImage.iterator()).thenReturn(contentsWithImageIterator);
when(dockerfileGitHubUtil.closeOutdatedPullRequestAndFork(contentRepo1)).thenReturn(null); // repo1 is unforkable
when(dockerfileGitHubUtil.closeOutdatedPullRequestAndFork(duplicateContentRepo1)).thenReturn(null); // repo1 is unforkable
when(dockerfileGitHubUtil.closeOutdatedPullRequestAndFork(contentRepo2)).thenReturn(new GHRepository());

Parent parent = new Parent();
parent.loadDockerfileGithubUtil(dockerfileGitHubUtil);
Multimap<String, String> repoMap = parent.forkRepositoriesFoundAndGetPathToDockerfiles(contentsWithImage);

// Since repo "1" is unforkable, we will only try to update repo "2"
verify(dockerfileGitHubUtil, times(3)).closeOutdatedPullRequestAndFork(any());
assertEquals(repoMap.size(), 1);
}

@Test
public void testForkRepositoriesFound_forkRepoIsSkipped() throws Exception {
DockerfileGitHubUtil dockerfileGitHubUtil = mock(DockerfileGitHubUtil.class);
Expand Down

0 comments on commit a6e1ce7

Please sign in to comment.