Avoid infinite sleep loop if multiple fork names come back from GitHub API #52
Conversation
| GHRepository fork = dockerfileGitHubUtil.closeOutdatedPullRequestAndFork(parent); | ||
| if (fork == null) { | ||
| log.info("Could not fork {}", parentRepoName); | ||
| pathToDockerfilesInParentRepo.remove(parentRepoName, c.getPath()); | ||
| imagesFoundInParentRepo.remove(parentRepoName, image); |
There was a problem hiding this comment.
This seems to be what really affects the logic. It seems like you reverted the name of parentReposAlreadyChecked as well as the comment here https://github.com/salesforce/dockerfile-image-update/pull/52/files#diff-850febb926ac48d58dc0962f42249092R118
There was a problem hiding this comment.
I'm not sure if I understand you completely. Are you saying with this fix, the logic is being affected?
There was a problem hiding this comment.
@VijayKumarMidde I don't think renaming parentReposAlreadyChecked to parentReposForked or putting parentReposForked.add(parentRepoName) in the else has any effect. We also lost part of the comment: https://github.com/salesforce/dockerfile-image-update/pull/52/files#diff-850febb926ac48d58dc0962f42249092L118
There was a problem hiding this comment.
@justinharringa If you don't mind, can you please elaborate why you think putting parentReposForked.add(parentRepoName) in the else doesn't have any effect? Thanks!
There was a problem hiding this comment.
It's not returned and doesn't modify anything that is passed in. What effect did it have moving it into the else?
There was a problem hiding this comment.
@afalko imagesFoundInParentRepo is passed as an empty Map to this function. This function was updating imagesFoundInParentRepo before these changes.
I think we can't use another form of Map with fork path as the key because in some cases we may need to update multiple Dockerfiles present in a repo.
There was a problem hiding this comment.
@afalko seems like some of this should be refactored to remove duplication and move more towards immutable calls. I could see some of these things being pulled into first-class objects rather than passing around a bunch of MultiMaps but that might be a tall order for this PR? Definitely open to ideas though.
There was a problem hiding this comment.
(part of what I meant to say was that the mutation of the MultiMap is something that has been here for a while and wasn't introduced by Vijay)
There was a problem hiding this comment.
Yeh, maybe file an issue for that and move forward here
| } | ||
|
|
||
| @Test | ||
| public void testForkRepositoriesFound_unableToforkRepo() throws Exception { |
There was a problem hiding this comment.
Please either add a comment on what this is testing. It seems like it would take some time to figure out what went wrong with this test if it started failing.
There was a problem hiding this comment.
Added comments.
| @@ -109,6 +109,49 @@ public void testForkRepositoriesFound() throws Exception { | |||
| assertEquals(repoMap.size(), 3); | |||
| } | |||
|
|
|||
| @Test | |||
| public void testForkRepositoriesFound_unableToforkRepo() throws Exception { | |||
There was a problem hiding this comment.
Same. What are we testing? What is the expected result here?
There was a problem hiding this comment.
Added comments.
| GHRepository contentRepo1 = mock(GHRepository.class); | ||
| when(contentRepo1.getFullName()).thenReturn("1"); | ||
|
|
||
| GHRepository contentRepo2 = mock(GHRepository.class); |
There was a problem hiding this comment.
Better name for this? duplicateContentRepo1 maybe?
There was a problem hiding this comment.
Sounds good. I will update.
Updated PR. |
| log.info("Forking {}", parentRepoName); | ||
| GHRepository fork = dockerfileGitHubUtil.closeOutdatedPullRequestAndFork(parent); | ||
| if (fork == null) { | ||
| if (fork != null) { |
There was a problem hiding this comment.
Could you flip this to avoid negative conditionals?
There was a problem hiding this comment.
@justinharringa Updated PR.
| GHRepository fork = dockerfileGitHubUtil.closeOutdatedPullRequestAndFork(parent); | ||
| if (fork == null) { | ||
| if (fork != null) { |
There was a problem hiding this comment.
Could you flip this to avoid negative conditionals?
There was a problem hiding this comment.
@justinharringa Updated PR.
| // 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)) { |
There was a problem hiding this comment.
Could you make this a positive assertion?
| if (!parentReposForked.contains(parentRepoName)) { | |
| if (parentReposForked.contains(parentRepoName) == false) { |
There was a problem hiding this comment.
@justinharringa Updated PR.
justinharringa
changed the title
When we try to fork a repo and fail for some reason, we were still adding that repo to parentReposAlreadyChecked as it would help us to avoid trying to create a fork and failing multiple times, when we are updating multiple Dockerfiles in the same repository. But this is not completely correct as we add the
(repo, dockerfile)first to pathToDockerfilesInParentRepo and then we check. Consider the following case:If contentsWithImage is
['seanhuitest/helloworld-docker-child', 'vmidde/helloworld-docker-child', 'vmidde/helloworld-docker-child',]. In this case we have two Dockerfiles invmidde/helloworld-docker-childrepository which needs to be updated. Also assume thatvmidde/helloworld-docker-childis unforkable.In this case, when we first visit
vmidde/helloworld-docker-childwhile iterating we add and removevmidde/helloworld-docker-childfrom pathToDockerfilesInParentRepo. In the same iteration we also addvmidde/helloworld-docker-childto parentReposAlreadyChecked. When we visitvmidde/helloworld-docker-childfor the second time, we first addvmidde/helloworld-docker-childto pathToDockerfilesInParentRepo and we never remove it because it is already checked in the previous iteration.To fix this issue, I'm simply not adding a repo to parentReposAlreadyChecked if we are unable to fork the repo.