Skip to content

Commit 86d2b1c

Browse files
afalkoafalko
authored
Merge branch 'master' into meaningul-pr-body-parent-child
2 parents 13b1ad1 + 32b3015 commit 86d2b1c

File tree

9 files changed

+619
-117
lines changed

9 files changed

+619
-117
lines changed

‎dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/model/FromInstruction.java

+16
Original file line numberDiff line numberDiff line change
@@ -91,6 +91,22 @@ private FromInstruction(String baseImageName, String tag, List<String> additiona
9191
this.comments = comments;
9292
}
9393

94+
/**
95+
* Check if this {@code lineInFile} is a FROM instruction, it is referencing {@code imageName} as a base image,
96+
* and the tag is not the same as {@code imageTag} (or there is no tag)
97+
*
98+
* @param lineInFile a single line from a file
99+
* @param imageName the base image name we're looking for
100+
* @param imageTag the proposed new tag
101+
*/
102+
public static boolean isFromInstructionWithThisImageAndOlderTag(String lineInFile, String imageName, String imageTag) {
103+
if (FromInstruction.isFromInstruction(lineInFile)) {
104+
FromInstruction fromInstruction = new FromInstruction(lineInFile);
105+
return fromInstruction.hasBaseImage(imageName) && fromInstruction.hasADifferentTag(imageTag);
106+
}
107+
return false;
108+
}
109+
94110
/**
95111
* Get a new {@code FromInstruction} the same as this but with the {@code tag} set as {@code newTag}
96112
* @param newTag the new image tag

‎dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/model/ShouldForkResult.java

+70
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,70 @@
1+
package com.salesforce.dockerfileimageupdate.model;
2+
3+
4+
import com.google.common.base.Objects;
5+
6+
/**
7+
* A result which can help determine if a repo should be forked or not.
8+
* If it should not be forked, comes with a reason.
9+
*/
10+
public class ShouldForkResult {
11+
public static final String NO_REASON = "no reason. Repo can be forked.";
12+
private final boolean shouldFork;
13+
private final String reason;
14+
15+
private ShouldForkResult(boolean shouldFork, String reason) {
16+
this.shouldFork = shouldFork;
17+
this.reason = reason;
18+
}
19+
20+
public static ShouldForkResult shouldForkResult() {
21+
return new ShouldForkResult(true, NO_REASON);
22+
}
23+
24+
public static ShouldForkResult shouldNotForkResult(String reason) {
25+
return new ShouldForkResult(false, reason);
26+
}
27+
28+
/**
29+
* Allows for chaining ShouldForkResult instances such that and() will return the first
30+
* ShouldForkResult which results in isForkable() == false.
31+
*
32+
* @param otherShouldForkResult the other ShouldForkResult to return if this is forkable.
33+
*/
34+
public ShouldForkResult and(ShouldForkResult otherShouldForkResult) {
35+
if (isForkable()) {
36+
return otherShouldForkResult;
37+
}
38+
return this;
39+
}
40+
41+
public boolean isForkable() {
42+
return shouldFork;
43+
}
44+
45+
public String getReason() {
46+
return reason;
47+
}
48+
49+
@Override
50+
public boolean equals(Object o) {
51+
if (this == o) return true;
52+
if (o == null || getClass() != o.getClass()) return false;
53+
ShouldForkResult that = (ShouldForkResult) o;
54+
return shouldFork == that.shouldFork &&
55+
Objects.equal(reason, that.reason);
56+
}
57+
58+
@Override
59+
public int hashCode() {
60+
return Objects.hashCode(shouldFork, reason);
61+
}
62+
63+
@Override
64+
public String toString() {
65+
if (isForkable()) {
66+
return "ShouldForkResult[Is forkable]";
67+
}
68+
return String.format("ShouldForkResult[Not forkable because %s", getReason());
69+
}
70+
}

‎dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/process/ForkableRepoValidator.java

+148
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,148 @@
1+
package com.salesforce.dockerfileimageupdate.process;
2+
3+
import com.salesforce.dockerfileimageupdate.model.FromInstruction;
4+
import com.salesforce.dockerfileimageupdate.model.GitForkBranch;
5+
import com.salesforce.dockerfileimageupdate.model.ShouldForkResult;
6+
import com.salesforce.dockerfileimageupdate.utils.DockerfileGitHubUtil;
7+
import org.kohsuke.github.GHContent;
8+
import org.kohsuke.github.GHRepository;
9+
import org.slf4j.Logger;
10+
import org.slf4j.LoggerFactory;
11+
12+
import java.io.BufferedReader;
13+
import java.io.IOException;
14+
import java.io.InputStream;
15+
import java.io.InputStreamReader;
16+
17+
import static com.salesforce.dockerfileimageupdate.model.ShouldForkResult.shouldForkResult;
18+
import static com.salesforce.dockerfileimageupdate.model.ShouldForkResult.shouldNotForkResult;
19+
20+
public class ForkableRepoValidator {
21+
private static final Logger log = LoggerFactory.getLogger(ForkableRepoValidator.class);
22+
23+
public static final String REPO_IS_FORK =
24+
"it's a fork already. Sending a PR to a fork is unsupported at the moment.";
25+
public static final String REPO_IS_ARCHIVED = "it's archived.";
26+
public static final String REPO_IS_OWNED_BY_THIS_USER = "it is owned by this user.";
27+
public static final String COULD_NOT_CHECK_THIS_USER =
28+
"we could not determine fork status because we don't know the identity of the authenticated user.";
29+
public static final String CONTENT_PATH_NOT_IN_DEFAULT_BRANCH_TEMPLATE =
30+
"didn't find content path %s in default branch";
31+
public static final String COULD_NOT_FIND_IMAGE_TO_UPDATE_TEMPLATE =
32+
"didn't find the image '%s' which required an update in path %s";
33+
private final DockerfileGitHubUtil dockerfileGitHubUtil;
34+
35+
public ForkableRepoValidator(DockerfileGitHubUtil dockerfileGitHubUtil) {
36+
this.dockerfileGitHubUtil = dockerfileGitHubUtil;
37+
}
38+
39+
/**
40+
* Check various conditions required for a repo to qualify for updates
41+
*
42+
* @param parentRepo parent repo we're checking
43+
* @param searchResultContent search result content we'll check against
44+
* @return should we fork the parentRepo?
45+
*/
46+
public ShouldForkResult shouldFork(GHRepository parentRepo,
47+
GHContent searchResultContent,
48+
GitForkBranch gitForkBranch) {
49+
return parentIsFork(parentRepo)
50+
.and(parentIsArchived(parentRepo)
51+
.and(thisUserIsNotOwner(parentRepo)
52+
.and(contentHasChangesInDefaultBranch(parentRepo, searchResultContent, gitForkBranch))));
53+
}
54+
55+
/**
56+
* Check to see if the default branch of the parentRepo has the path we found in searchResultContent and
57+
* whether that content has a qualifying base image update
58+
* @param parentRepo parentRepo which we'd fork off of
59+
* @param searchResultContent search result with path to check in parent repo's default branch (where we'd PR)
60+
* @param gitForkBranch information about the imageName we'd like to update with the new tag
61+
*/
62+
protected ShouldForkResult contentHasChangesInDefaultBranch(GHRepository parentRepo,
63+
GHContent searchResultContent,
64+
GitForkBranch gitForkBranch) {
65+
try {
66+
String searchContentPath = searchResultContent.getPath();
67+
GHContent content =
68+
dockerfileGitHubUtil.tryRetrievingContent(parentRepo,
69+
searchContentPath, parentRepo.getDefaultBranch());
70+
if (content == null) {
71+
return shouldNotForkResult(
72+
String.format(CONTENT_PATH_NOT_IN_DEFAULT_BRANCH_TEMPLATE, searchContentPath));
73+
} else {
74+
if (hasNoChanges(content, gitForkBranch)) {
75+
return shouldNotForkResult(
76+
String.format(COULD_NOT_FIND_IMAGE_TO_UPDATE_TEMPLATE,
77+
gitForkBranch.getImageName(), searchContentPath));
78+
}
79+
}
80+
} catch (InterruptedException e) {
81+
log.warn("Couldn't get parent content to check for some reason for {}. Trying to proceed... exception: {}",
82+
parentRepo.getFullName(), e.getMessage());
83+
}
84+
return shouldForkResult();
85+
}
86+
87+
/**
88+
* Check to see whether there are any changes in the specified content where the specified base image
89+
* in gitForkBranch needs an update
90+
*
91+
* @param content content to check
92+
* @param gitForkBranch information about the base image we'd like to update
93+
*/
94+
protected boolean hasNoChanges(GHContent content, GitForkBranch gitForkBranch) {
95+
try (InputStream stream = content.read();
96+
InputStreamReader streamR = new InputStreamReader(stream);
97+
BufferedReader reader = new BufferedReader(streamR)) {
98+
String line;
99+
while ( (line = reader.readLine()) != null ) {
100+
if (FromInstruction.isFromInstructionWithThisImageAndOlderTag(line,
101+
gitForkBranch.getImageName(), gitForkBranch.getImageTag())) {
102+
return false;
103+
}
104+
}
105+
} catch (IOException exception) {
106+
log.warn("Failed while checking if there are changes in {}. Skipping... exception: {}",
107+
content.getPath(), exception.getMessage());
108+
}
109+
return true;
110+
}
111+
112+
/**
113+
* Attempts to check to see if this user is the owner of the repo (don't fork your own repo).
114+
* If we can't tell because of a systemic error, don't attempt to fork (we could perhaps loosen this in the future).
115+
* If this user is the owner of the repo, do not fork.
116+
*
117+
* @param parentRepo parent repo we're checking
118+
*/
119+
protected ShouldForkResult thisUserIsNotOwner(GHRepository parentRepo) {
120+
try {
121+
if (dockerfileGitHubUtil.thisUserIsOwner(parentRepo)) {
122+
return shouldNotForkResult(REPO_IS_OWNED_BY_THIS_USER);
123+
}
124+
} catch (IOException ioException) {
125+
return shouldNotForkResult(COULD_NOT_CHECK_THIS_USER);
126+
}
127+
return shouldForkResult();
128+
}
129+
130+
/**
131+
* Check if parentRepo is a fork. Do not fork a fork (for now, at least).
132+
*
133+
* @param parentRepo parent repo we're checking
134+
*/
135+
protected ShouldForkResult parentIsFork(GHRepository parentRepo) {
136+
return parentRepo.isFork() ? shouldNotForkResult(REPO_IS_FORK) : shouldForkResult();
137+
}
138+
139+
/**
140+
* Check if parentRepo is archived. We won't be able to update it, so do not fork.
141+
*
142+
* @param parentRepo parent repo we're checking
143+
*/
144+
protected ShouldForkResult parentIsArchived(GHRepository parentRepo) {
145+
return parentRepo.isArchived() ? shouldNotForkResult(REPO_IS_ARCHIVED) : shouldForkResult();
146+
147+
}
148+
}

‎dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/process/GitHubPullRequestSender.java

+12-31
Original file line numberDiff line numberDiff line change
@@ -2,7 +2,9 @@
22

33
import com.google.common.collect.HashMultimap;
44
import com.google.common.collect.Multimap;
5+
import com.salesforce.dockerfileimageupdate.model.GitForkBranch;
56
import com.salesforce.dockerfileimageupdate.model.GitHubContentToProcess;
7+
import com.salesforce.dockerfileimageupdate.model.ShouldForkResult;
68
import com.salesforce.dockerfileimageupdate.utils.DockerfileGitHubUtil;
79
import org.kohsuke.github.GHContent;
810
import org.kohsuke.github.GHRepository;
@@ -16,13 +18,12 @@
1618
public class GitHubPullRequestSender {
1719
private static final Logger log = LoggerFactory.getLogger(GitHubPullRequestSender.class);
1820
public static final String REPO_IS_FORK = "it's a fork already. Sending a PR to a fork is unsupported at the moment.";
19-
public static final String REPO_IS_ARCHIVED = "it's archived.";
20-
public static final String REPO_IS_OWNED_BY_THIS_USER = "it is owned by this user.";
21-
public static final String COULD_NOT_CHECK_THIS_USER = "we could not determine fork status because we don't know the identity of the authenticated user.";
2221
private final DockerfileGitHubUtil dockerfileGitHubUtil;
22+
private final ForkableRepoValidator forkableRepoValidator;
2323

24-
public GitHubPullRequestSender(DockerfileGitHubUtil dockerfileGitHubUtil) {
24+
public GitHubPullRequestSender(DockerfileGitHubUtil dockerfileGitHubUtil, ForkableRepoValidator forkableRepoValidator) {
2525
this.dockerfileGitHubUtil = dockerfileGitHubUtil;
26+
this.forkableRepoValidator = forkableRepoValidator;
2627
}
2728

2829
/* There is a separation here with forking and performing the Dockerfile update. This is because of the delay
@@ -32,7 +33,9 @@ public GitHubPullRequestSender(DockerfileGitHubUtil dockerfileGitHubUtil) {
3233
*
3334
* NOTE: We are not currently forking repositories that are already forks
3435
*/
35-
public Multimap<String, GitHubContentToProcess> forkRepositoriesFoundAndGetPathToDockerfiles(PagedSearchIterable<GHContent> contentsWithImage) {
36+
public Multimap<String, GitHubContentToProcess> forkRepositoriesFoundAndGetPathToDockerfiles(
37+
PagedSearchIterable<GHContent> contentsWithImage,
38+
GitForkBranch gitForkBranch) {
3639
log.info("Forking repositories...");
3740
Multimap<String, GitHubContentToProcess> pathToDockerfilesInParentRepo = HashMultimap.create();
3841
GHRepository parent;
@@ -50,12 +53,12 @@ public Multimap<String, GitHubContentToProcess> forkRepositoriesFoundAndGetPathT
5053
// Refresh the repo to ensure that the object has full details
5154
try {
5255
parent = dockerfileGitHubUtil.getRepo(parentRepoName);
53-
Optional<String> shouldNotForkRepo = shouldNotForkRepo(parent);
54-
if (shouldNotForkRepo.isPresent()) {
55-
log.warn("Skipping {} because {}", parentRepoName, shouldNotForkRepo.get());
56-
} else {
56+
ShouldForkResult shouldForkResult = forkableRepoValidator.shouldFork(parent, ghContent, gitForkBranch);
57+
if (shouldForkResult.isForkable()) {
5758
// fork the parent if not already forked
5859
ensureForkedAndAddToListForProcessing(pathToDockerfilesInParentRepo, parent, parentRepoName, ghContent);
60+
} else {
61+
log.warn("Skipping {} because {}", parentRepoName, shouldForkResult.getReason());
5962
}
6063
} catch (IOException exception) {
6164
log.warn("Could not refresh details of {}", parentRepoName);
@@ -110,26 +113,4 @@ protected GHRepository getForkFromExistingRecordToProcess(Multimap<String, GitHu
110113
}
111114
return null;
112115
}
113-
114-
/**
115-
* Returns a optional with the reason if we should not fork a repo else returns an empty optional if we should fork.
116-
* @param parentRepo The parent repo which may or may not be a candidate to fork
117-
*/
118-
protected Optional<String> shouldNotForkRepo(GHRepository parentRepo) {
119-
Optional<String> result = Optional.empty();
120-
if (parentRepo.isFork()) {
121-
result = Optional.of(REPO_IS_FORK);
122-
} else if (parentRepo.isArchived()) {
123-
result = Optional.of(REPO_IS_ARCHIVED);
124-
} else {
125-
try {
126-
if (dockerfileGitHubUtil.thisUserIsOwner(parentRepo)) {
127-
result = Optional.of(REPO_IS_OWNED_BY_THIS_USER);
128-
}
129-
} catch (IOException ioException) {
130-
result = Optional.of(COULD_NOT_CHECK_THIS_USER);
131-
}
132-
}
133-
return result;
134-
}
135116
}

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

+8-3
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,7 @@
1212
import com.salesforce.dockerfileimageupdate.SubCommand;
1313
import com.salesforce.dockerfileimageupdate.model.GitForkBranch;
1414
import com.salesforce.dockerfileimageupdate.model.GitHubContentToProcess;
15-
import com.salesforce.dockerfileimageupdate.model.PullRequestInfo;
15+
import com.salesforce.dockerfileimageupdate.process.ForkableRepoValidator;
1616
import com.salesforce.dockerfileimageupdate.process.GitHubPullRequestSender;
1717
import com.salesforce.dockerfileimageupdate.subcommands.ExecutableWithNamespace;
1818
import com.salesforce.dockerfileimageupdate.utils.Constants;
@@ -49,12 +49,17 @@ public void execute(final Namespace ns, DockerfileGitHubUtil dockerfileGitHubUti
4949
log.info("Updating store...");
5050
this.dockerfileGitHubUtil.getGitHubJsonStore(ns.get(Constants.STORE)).updateStore(img, tag);
5151

52-
GitHubPullRequestSender pullRequestSender = new GitHubPullRequestSender(dockerfileGitHubUtil);
52+
GitHubPullRequestSender pullRequestSender =
53+
new GitHubPullRequestSender(dockerfileGitHubUtil, new ForkableRepoValidator(dockerfileGitHubUtil));
54+
55+
GitForkBranch gitForkBranch =
56+
new GitForkBranch(ns.get(Constants.IMG), ns.get(Constants.TAG), ns.get(Constants.GIT_BRANCH));
5357

5458
log.info("Finding Dockerfiles with the given image...");
5559
Optional<PagedSearchIterable<GHContent>> contentsWithImage = dockerfileGitHubUtil.getGHContents(ns.get(Constants.GIT_ORG), img);
5660
if (contentsWithImage.isPresent()) {
57-
Multimap<String, GitHubContentToProcess> pathToDockerfilesInParentRepo = pullRequestSender.forkRepositoriesFoundAndGetPathToDockerfiles(contentsWithImage.get());
61+
Multimap<String, GitHubContentToProcess> pathToDockerfilesInParentRepo =
62+
pullRequestSender.forkRepositoriesFoundAndGetPathToDockerfiles(contentsWithImage.get(), gitForkBranch);
5863
List<IOException> exceptions = new ArrayList<>();
5964
List<String> skippedRepos = new ArrayList<>();
6065

‎dockerfile-image-update/src/test/java/com/salesforce/dockerfileimageupdate/model/FromInstructionTest.java

+15
Original file line numberDiff line numberDiff line change
@@ -224,4 +224,19 @@ public Object[][] invalidData() {
224224
public void testInvalidFromData(String input) {
225225
new FromInstruction(input);
226226
}
227+
228+
@DataProvider
229+
public Object[][] isFromInstructionWithThisImageAndOlderTagData() {
230+
return new Object[][]{
231+
{"FROM someimage", "someimage", "sometag", true},
232+
{"FROM someimage:sometag", "someimage", "sometag", false},
233+
{"not a from instruction", "someimage", "sometag", false},
234+
{"FROM someimage", "someimage", "sometag", true}
235+
};
236+
}
237+
238+
@Test(dataProvider = "isFromInstructionWithThisImageAndOlderTagData")
239+
public void isFromInstructionWithThisImageAndOlderTag(String line, String imageName, String imageTag, boolean expectedResult) {
240+
assertEquals(FromInstruction.isFromInstructionWithThisImageAndOlderTag(line, imageName, imageTag), expectedResult);
241+
}
227242
}

0 commit comments

Comments
 (0)