Skip to content

Commit 11833b3

Browse files
VijayKumarMiddeVijayKumarMidde
committed
Do not add a repo to parentReposForked if unable to fork
1 parent 887213e commit 11833b3

File tree

4 files changed

+124
-20
lines changed

4 files changed

+124
-20
lines changed

‎dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/subcommands/impl/All.java

+9-9
Original file line numberDiff line numberDiff line change
@@ -95,7 +95,7 @@ protected void forkRepositoriesFound(Multimap<String, String> pathToDockerfilesI
9595
PagedSearchIterable<GHContent> contentsWithImage,
9696
String image) throws IOException {
9797
log.info("Forking {} repositories...", contentsWithImage.getTotalCount());
98-
List<String> parentReposAlreadyChecked = new ArrayList<>();
98+
List<String> parentReposForked = new ArrayList<>();
9999
GHRepository parent;
100100
String parentRepoName = null;
101101
for (GHContent c : contentsWithImage) {
@@ -112,17 +112,17 @@ protected void forkRepositoriesFound(Multimap<String, String> pathToDockerfilesI
112112
log.warn("Skipping {} because it's a fork already. Sending a PR to a fork is unsupported at the moment.",
113113
parentRepoName);
114114
} else {
115-
pathToDockerfilesInParentRepo.put(parentRepoName, c.getPath());
116-
imagesFoundInParentRepo.put(parentRepoName, image);
117-
118-
// fork the parent if not already forked or we couldn't fork
119-
if (!parentReposAlreadyChecked.contains(parentRepoName)) {
115+
// fork the parent if not already forked
116+
if (!parentReposForked.contains(parentRepoName)) {
120117
GHRepository fork = dockerfileGitHubUtil.closeOutdatedPullRequestAndFork(parent);
121-
if (fork == null) {
118+
if (fork != null) {
119+
// Add repos to pathToDockerfilesInParentRepo and imagesFoundInParentRepo only if we forked it successfully.
120+
pathToDockerfilesInParentRepo.put(parentRepoName, c.getPath());
121+
imagesFoundInParentRepo.put(parentRepoName, image);
122+
parentReposForked.add(parentRepoName);
123+
} else {
122124
log.info("Could not fork {}", parentRepoName);
123-
pathToDockerfilesInParentRepo.remove(parentRepoName, c.getPath());
124125
}
125-
parentReposAlreadyChecked.add(parentRepoName);
126126
}
127127
}
128128
}

‎dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/subcommands/impl/Parent.java

+8-7
Original file line numberDiff line numberDiff line change
@@ -110,7 +110,7 @@ protected PagedSearchIterable<GHContent> getGHContents(String org, String img)
110110
protected Multimap<String, String> forkRepositoriesFoundAndGetPathToDockerfiles(PagedSearchIterable<GHContent> contentsWithImage) throws IOException {
111111
log.info("Forking repositories...");
112112
Multimap<String, String> pathToDockerfilesInParentRepo = HashMultimap.create();
113-
List<String> parentReposAlreadyChecked = new ArrayList<>();
113+
List<String> parentReposForked = new ArrayList<>();
114114
GHRepository parent;
115115
String parentRepoName = null;
116116
for (GHContent c : contentsWithImage) {
@@ -127,16 +127,17 @@ protected Multimap<String, String> forkRepositoriesFoundAndGetPathToDockerfiles(
127127
log.warn("Skipping {} because it's a fork already. Sending a PR to a fork is unsupported at the moment.",
128128
parentRepoName);
129129
} else {
130-
pathToDockerfilesInParentRepo.put(parentRepoName, c.getPath());
131-
// fork the parent if not already forked or we couldn't fork
132-
if (!parentReposAlreadyChecked.contains(parentRepoName)) {
130+
// fork the parent if not already forked
131+
if (!parentReposForked.contains(parentRepoName)) {
133132
log.info("Forking {}", parentRepoName);
134133
GHRepository fork = dockerfileGitHubUtil.closeOutdatedPullRequestAndFork(parent);
135-
if (fork == null) {
134+
if (fork != null) {
135+
// Add repos to pathToDockerfilesInParentRepo only if we forked it successfully.
136+
pathToDockerfilesInParentRepo.put(parentRepoName, c.getPath());
137+
parentReposForked.add(parentRepoName);
138+
} else {
136139
log.info("Could not fork {}", parentRepoName);
137-
pathToDockerfilesInParentRepo.remove(parentRepoName, c.getPath());
138140
}
139-
parentReposAlreadyChecked.add(parentRepoName);
140141
}
141142
}
142143
}

‎dockerfile-image-update/src/test/java/com/salesforce/dockerfileimageupdate/subcommands/impl/AllTest.java

+55-4
Original file line numberDiff line numberDiff line change
@@ -25,10 +25,9 @@
2525
import java.util.Map;
2626
import java.util.Set;
2727

28-
import static org.mockito.Matchers.any;
29-
import static org.mockito.Matchers.anyString;
30-
import static org.mockito.Matchers.eq;
28+
import static org.mockito.Matchers.*;
3129
import static org.mockito.Mockito.*;
30+
import static org.testng.Assert.assertEquals;
3231
import static org.testng.Assert.assertNotNull;
3332

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

64-
Mockito.verify(dockerfileGitHubUtil, times(1)).closeOutdatedPullRequestAndFork(any());
63+
Mockito.verify(dockerfileGitHubUtil, times(3)).closeOutdatedPullRequestAndFork(any());
64+
}
65+
66+
@Test
67+
public void testForkRepositoriesFound_unableToforkRepo() throws Exception {
68+
/**
69+
* Suppose we have multiple dockerfiles that need to updated in a repo and we fail to fork such repo,
70+
* we should not add those repos to pathToDockerfilesInParentRepo.
71+
*/
72+
DockerfileGitHubUtil dockerfileGitHubUtil = mock(DockerfileGitHubUtil.class);
73+
74+
GHRepository contentRepo1 = mock(GHRepository.class);
75+
when(contentRepo1.getFullName()).thenReturn("1");
76+
77+
GHRepository contentRepo2 = mock(GHRepository.class);
78+
// Say we have multiple dockerfiles to be updated in repo "1"
79+
when(contentRepo2.getFullName()).thenReturn("1");
80+
81+
GHRepository contentRepo3 = mock(GHRepository.class);
82+
when(contentRepo3.getFullName()).thenReturn("2");
83+
84+
GHContent content1 = mock(GHContent.class);
85+
when(content1.getOwner()).thenReturn(contentRepo1);
86+
when(content1.getPath()).thenReturn("1"); // path to 1st dockerfile in repo "1"
87+
88+
GHContent content2 = mock(GHContent.class);
89+
when(content2.getOwner()).thenReturn(contentRepo2);
90+
when(content2.getPath()).thenReturn("2"); // path to 2st dockerfile in repo "1"
91+
92+
GHContent content3 = mock(GHContent.class);
93+
when(content3.getOwner()).thenReturn(contentRepo3);
94+
when(content3.getPath()).thenReturn("3");
95+
96+
PagedSearchIterable<GHContent> contentsWithImage = mock(PagedSearchIterable.class);
97+
98+
PagedIterator<GHContent> contentsWithImageIterator = mock(PagedIterator.class);
99+
when(contentsWithImageIterator.hasNext()).thenReturn(true, true, true, false);
100+
when(contentsWithImageIterator.next()).thenReturn(content1, content2, content3, null);
101+
when(contentsWithImage.iterator()).thenReturn(contentsWithImageIterator);
102+
when(dockerfileGitHubUtil.closeOutdatedPullRequestAndFork(contentRepo1)).thenReturn(null); // repo1 is unforkable
103+
when(dockerfileGitHubUtil.closeOutdatedPullRequestAndFork(contentRepo2)).thenReturn(null); // repo1 is unforkable
104+
when(dockerfileGitHubUtil.closeOutdatedPullRequestAndFork(contentRepo3)).thenReturn(new GHRepository());
105+
106+
All all = new All();
107+
all.loadDockerfileGithubUtil(dockerfileGitHubUtil);
108+
Multimap<String, String> pathToDockerfilesInParentRepo = ArrayListMultimap.create();
109+
Multimap<String, String> imagesFoundInParentRepo = ArrayListMultimap.create();
110+
all.forkRepositoriesFound(pathToDockerfilesInParentRepo, imagesFoundInParentRepo, contentsWithImage, "image");
111+
112+
// Since repo "1" is unforkable, we only added repo "2" to pathToDockerfilesInParentRepo
113+
assertEquals(pathToDockerfilesInParentRepo.size(), 1);
114+
assertEquals(imagesFoundInParentRepo.size(), 1);
115+
Mockito.verify(dockerfileGitHubUtil, times(3)).closeOutdatedPullRequestAndFork(any());
65116
}
66117

67118
@Test

‎dockerfile-image-update/src/test/java/com/salesforce/dockerfileimageupdate/subcommands/impl/ParentTest.java

+52
Original file line numberDiff line numberDiff line change
@@ -109,6 +109,58 @@ public void testForkRepositoriesFound() throws Exception {
109109
assertEquals(repoMap.size(), 3);
110110
}
111111

112+
@Test
113+
public void forkRepositoriesFoundAndGetPathToDockerfiles_unableToforkRepo() throws Exception {
114+
/**
115+
* Suppose we have multiple dockerfiles that need to updated in a repo and we fail to fork such repo,
116+
* we should not add those repos to pathToDockerfilesInParentRepo.
117+
*
118+
* Note: Sometimes GitHub search API returns the same result twice. This test covers such cases as well.
119+
*/
120+
DockerfileGitHubUtil dockerfileGitHubUtil = mock(DockerfileGitHubUtil.class);
121+
122+
GHRepository contentRepo1 = mock(GHRepository.class);
123+
when(contentRepo1.getFullName()).thenReturn("1");
124+
125+
GHRepository duplicateContentRepo1 = mock(GHRepository.class);
126+
// Say we have multiple dockerfiles to be updated in repo "1"
127+
// Or sometimes GitHub search API returns same result twice.
128+
when(duplicateContentRepo1.getFullName()).thenReturn("1");
129+
130+
GHRepository contentRepo2 = mock(GHRepository.class);
131+
when(contentRepo2.getFullName()).thenReturn("2");
132+
133+
GHContent content1 = mock(GHContent.class);
134+
when(content1.getOwner()).thenReturn(contentRepo1);
135+
when(content1.getPath()).thenReturn("1"); // path to 1st dockerfile in repo "1"
136+
137+
GHContent content2 = mock(GHContent.class);
138+
when(content2.getOwner()).thenReturn(duplicateContentRepo1);
139+
when(content2.getPath()).thenReturn("2"); // path to 2st dockerfile in repo "1"
140+
141+
GHContent content3 = mock(GHContent.class);
142+
when(content3.getOwner()).thenReturn(contentRepo2);
143+
when(content3.getPath()).thenReturn("3");
144+
145+
PagedSearchIterable<GHContent> contentsWithImage = mock(PagedSearchIterable.class);
146+
147+
PagedIterator<GHContent> contentsWithImageIterator = mock(PagedIterator.class);
148+
when(contentsWithImageIterator.hasNext()).thenReturn(true, true, true, false);
149+
when(contentsWithImageIterator.next()).thenReturn(content1, content2, content3, null);
150+
when(contentsWithImage.iterator()).thenReturn(contentsWithImageIterator);
151+
when(dockerfileGitHubUtil.closeOutdatedPullRequestAndFork(contentRepo1)).thenReturn(null); // repo1 is unforkable
152+
when(dockerfileGitHubUtil.closeOutdatedPullRequestAndFork(duplicateContentRepo1)).thenReturn(null); // repo1 is unforkable
153+
when(dockerfileGitHubUtil.closeOutdatedPullRequestAndFork(contentRepo2)).thenReturn(new GHRepository());
154+
155+
Parent parent = new Parent();
156+
parent.loadDockerfileGithubUtil(dockerfileGitHubUtil);
157+
Multimap<String, String> repoMap = parent.forkRepositoriesFoundAndGetPathToDockerfiles(contentsWithImage);
158+
159+
// Since repo "1" is unforkable, we will only try to update repo "2"
160+
verify(dockerfileGitHubUtil, times(3)).closeOutdatedPullRequestAndFork(any());
161+
assertEquals(repoMap.size(), 1);
162+
}
163+
112164
@Test
113165
public void testForkRepositoriesFound_forkRepoIsSkipped() throws Exception {
114166
DockerfileGitHubUtil dockerfileGitHubUtil = mock(DockerfileGitHubUtil.class);

0 commit comments

Comments
 (0)