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.

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

Skip DFIU PRs for repos onboarded to Renovate Enterprise #1128

Merged
merged 11 commits into from
Feb 8, 2024
Merged
Show file tree
Hide file tree
Changes from 9 commits
Commits
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
1 change: 1 addition & 0 deletions .gitignore
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
*.ipr
*.iws
*.iml
*.DS_Store

# Vim files
*.swp
Expand Down
5 changes: 5 additions & 0 deletions dockerfile-image-update/pom.xml
Original file line number Diff line number Diff line change
Expand Up @@ -69,6 +69,11 @@


<dependencies>
<dependency>
<groupId>org.json</groupId>
Copy link
Member

Choose a reason for hiding this comment

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

is there something we can't go with gson dependency that we already have https://github.com/avimanyum/dockerfile-image-update/blob/master/dockerfile-image-update/pom.xml#L116. or you are brining it in for easy of use?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah its just a cleaner and easier way to convert to Json object

<artifactId>json</artifactId>
<version>20240205</version>
</dependency>
<dependency>
<groupId>org.slf4j</groupId>
<artifactId>slf4j-simple</artifactId>
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,10 @@ static ArgumentParser getArgumentParser() {
ArgumentParsers.newFor("dockerfile-image-update").addHelp(true).build()
.description("Image Updates through Pull Request Automator");

parser.addArgument("-R", "--" + CHECK_FOR_RENOVATE)
.type(Boolean.class)
.setDefault(false) //To prevent null from being returned by the argument
.help("Check if Renovate app is being used to receive remediation PRs.");
parser.addArgument("-l", "--" + GIT_API_SEARCH_LIMIT)
.type(Integer.class)
.setDefault(1000)
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@


import java.time.Duration;
import java.util.*;

/**
* @author minho-park
Expand Down Expand Up @@ -51,4 +52,7 @@ private Constants() {
public static final Duration DEFAULT_TOKEN_ADDING_RATE = Duration.ofMinutes(DEFAULT_CONSUMING_TOKEN_RATE);
public static final String FILENAME_DOCKERFILE = "dockerfile";
public static final String FILENAME_DOCKER_COMPOSE = "docker-compose";
public static final String CHECK_FOR_RENOVATE = "checkforrenovate";
//The Renovate configuration file can be in any one of the following locations. Refer to https://docs.renovatebot.com/configuration-options/
public static final List<String> RENOVATE_CONFIG_FILEPATHS = Arrays.asList("renovate.json", "renovate.json5", ".github/renovate.json", ".github/renovate.json5", ".gitlab/renovate.json", ".gitlab/renovate.json5", ".renovaterc", ".renovaterc.json", ".renovaterc.json5");
}
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,8 @@
import com.salesforce.dockerfileimageupdate.model.*;
import com.salesforce.dockerfileimageupdate.process.*;
import net.sourceforge.argparse4j.inf.*;
import org.json.JSONObject;
import org.json.JSONTokener;
import org.kohsuke.github.*;
import org.slf4j.*;

Expand All @@ -27,10 +29,16 @@ public void prepareToCreate(final Namespace ns,
pathToDockerfilesInParentRepo.get(currUserRepo).stream().findFirst();
if (forkWithContentPaths.isPresent()) {
try {
dockerfileGitHubUtil.changeDockerfiles(ns,
pathToDockerfilesInParentRepo,
forkWithContentPaths.get(), skippedRepos,
gitForkBranch, rateLimiter);
//If the repository has been onboarded to renovate enterprise, skip sending the DFIU PR
if(ns.getBoolean(Constants.CHECK_FOR_RENOVATE)
&& (isRenovateEnabled(Constants.RENOVATE_CONFIG_FILEPATHS, forkWithContentPaths.get()))) {
log.info("Found a renovate configuration file in the repo %s. Skip sending DFIU PRs to this repository.", forkWithContentPaths.get().getParent().getFullName());
} else {
dockerfileGitHubUtil.changeDockerfiles(ns,
pathToDockerfilesInParentRepo,
forkWithContentPaths.get(), skippedRepos,
gitForkBranch, rateLimiter);
}
} catch (IOException | InterruptedException e) {
log.error(String.format("Error changing Dockerfile for %s", forkWithContentPaths.get().getParent().getFullName()), e);
exceptions.add((IOException) e);
Expand All @@ -41,5 +49,33 @@ public void prepareToCreate(final Namespace ns,
}
ResultsProcessor.processResults(skippedRepos, exceptions, log);
}

/**
* Check if the repository is onboarded to Renovate. The signal we are looking for are
* (1) The presence of a file where renovate configurations are stored in the repository
* (2) Ensuring that the file does not have the key "enabled" set to "false"
* @param filePaths the list that contains all the names of the files that we are searching for in the repo
* @param fork A GitHubContentToProcess object that contains the fork repository that is under process
* @return true if the file is found in the path specified and is not disabled, false otherwise
*/
protected boolean isRenovateEnabled(List<String> filePaths, GitHubContentToProcess fork) throws IOException {
Copy link
Member

Choose a reason for hiding this comment

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

we can also extract reading Json from GHContent, that way we can reuse this method if needed
private JSONObject readJsonFromContent(GHContent content) throws IOException {
try (BufferedReader bufferedReader = new BufferedReader(new InputStreamReader(content.read()))) {
JSONTokener tokener = new JSONTokener(bufferedReader);
return new JSONObject(tokener);
}
}

for (String filePath : filePaths) {
try {
GHContent fileContent = fork.getParent().getFileContent(filePath);
JSONObject json;
try (BufferedReader bufferedReader = new BufferedReader(new InputStreamReader(fileContent.read()))) {
JSONTokener tokener = new JSONTokener(bufferedReader);
json = new JSONObject(tokener);
//If the file has the key 'enabled' set to false, it indicates that while the repo has been onboarded to renovate, it has been disabled for some reason
return json.optBoolean("enabled", true);
Copy link
Member

Choose a reason for hiding this comment

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

So the users need to delete this key altogether to get DFIU PRs back? true/false both mean we skip DFIU PRs?

Copy link
Contributor Author

@avimanyum avimanyum Feb 8, 2024

Choose a reason for hiding this comment

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

Here are the scenarios

  • Renovate config file not found - Send DFIU PR
  • Renovate config file found and file does not have key enabled - Skip DFIU PR
  • Renovate config file found and file has the key enabled - return the boolean value for that key. If set to true, skip DFIU PR, if set to false, send DFIU PR

This key is usually used when a repo onboards to Renovate but then decides to off board by adding the key enabled: false.

Copy link
Member

Choose a reason for hiding this comment

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

"Renovate config file found and file does not have key enabled - Skip DFIU PR" Should we do this? the config file doesn't have this key, doesn't that mean it's enabled? the default is set to true https://docs.renovatebot.com/configuration-options/#enabled

Copy link
Member

@jeetchoudhary jeetchoudhary Feb 8, 2024

Choose a reason for hiding this comment

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

https://github.com/stleary/JSON-java/blob/master/src/main/java/org/json/JSONObject.java#L1143
The method will return true if the file doesn't have the key enabled and we'll skip the PR

Copy link
Member

Choose a reason for hiding this comment

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

or am I overlooking something?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

"Renovate config file found and file does not have key enabled - Skip DFIU PR" Should we do this? the config file doesn't have this key, doesn't that mean it's enabled? the default is set to true https://docs.renovatebot.com/configuration-options/#enabled

Yes, it does mean it is enabled. So we will skip DFIU because we expect to receive PRs from Renovate

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 method will return true if the file doesn't have the key enabled and we'll skip the PR

Yes, it will return true. We will assume that Renovate is enabled

Copy link
Member

Choose a reason for hiding this comment

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

I see. This comment confused me a little, I thought we wanted to skip sending the PR in this case.
//If the file has the key 'enabled' set to false, it indicates that while the repo has been onboarded to renovate, it has been disabled for some reason.

} catch (IOException e) {
log.debug("Exception while trying to close a resource. Exception: %s", e.getMessage());
}
} catch (FileNotFoundException e) {
log.debug("The file with name %s not found in the repository. Exception: %s", filePath, e.getMessage());
}
}
return false;
}
}

Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import net.sourceforge.argparse4j.inf.*;
import org.kohsuke.github.*;
import org.mockito.*;
import org.testng.*;
import org.testng.annotations.*;

import java.io.*;
Expand All @@ -21,7 +22,7 @@ public void testPullRequestsPrepareToCreateSuccessful() throws Exception {
"image", Constants.TAG,
"tag", Constants.STORE,
"store", Constants.SKIP_PR_CREATION,
false);
false, Constants.CHECK_FOR_RENOVATE, false);
Namespace ns = new Namespace(nsMap);
PullRequests pullRequests = new PullRequests();
GitHubPullRequestSender pullRequestSender = mock(GitHubPullRequestSender.class);
Expand Down Expand Up @@ -51,7 +52,7 @@ public void testPullRequestsPrepareThrowsException() throws Exception {
"image", Constants.TAG,
"tag", Constants.STORE,
"store", Constants.SKIP_PR_CREATION,
false);
false, Constants.CHECK_FOR_RENOVATE, false);
Namespace ns = new Namespace(nsMap);
PullRequests pullRequests = new PullRequests();
GitHubPullRequestSender pullRequestSender = mock(GitHubPullRequestSender.class);
Expand Down Expand Up @@ -89,7 +90,7 @@ public void testPullRequestsPrepareToCreateWhenNoDockerfileFound() throws Except
"image", Constants.TAG,
"tag", Constants.STORE,
"store", Constants.SKIP_PR_CREATION,
false);
false, Constants.CHECK_FOR_RENOVATE, false);
Namespace ns = new Namespace(nsMap);
PullRequests pullRequests = new PullRequests();
GitHubPullRequestSender pullRequestSender = mock(GitHubPullRequestSender.class);
Expand All @@ -110,4 +111,117 @@ public void testPullRequestsPrepareToCreateWhenNoDockerfileFound() throws Except
eq(pathToDockerfilesInParentRepo),
eq(gitHubContentToProcess), anyList(), eq(gitForkBranch),eq(rateLimiter));
}

@Test
public void testPullRequestsPrepareSkipsSendingPRIfRepoOnboardedToRenovate() throws Exception {
Map<String, Object> nsMap = ImmutableMap.of(
Constants.IMG, "image",
Constants.TAG, "tag",
Constants.STORE,"store",
Constants.SKIP_PR_CREATION,false,
Constants.CHECK_FOR_RENOVATE, true);


Namespace ns = new Namespace(nsMap);
PullRequests pullRequests = new PullRequests();
GitHubPullRequestSender pullRequestSender = mock(GitHubPullRequestSender.class);
PagedSearchIterable<GHContent> contentsFoundWithImage = mock(PagedSearchIterable.class);
GitForkBranch gitForkBranch = mock(GitForkBranch.class);
DockerfileGitHubUtil dockerfileGitHubUtil = mock(DockerfileGitHubUtil.class);
RateLimiter rateLimiter = Mockito.spy(new RateLimiter());
Multimap<String, GitHubContentToProcess> pathToDockerfilesInParentRepo = ArrayListMultimap.create();
GitHubContentToProcess gitHubContentToProcess = mock(GitHubContentToProcess.class);
pathToDockerfilesInParentRepo.put("repo1", gitHubContentToProcess);
pathToDockerfilesInParentRepo.put("repo2", gitHubContentToProcess);
pathToDockerfilesInParentRepo.put("repo3", gitHubContentToProcess);
GHContent content = mock(GHContent.class);
InputStream inputStream1 = new ByteArrayInputStream("{someKey:someValue}".getBytes());
InputStream inputStream2 = new ByteArrayInputStream("{enabled:false}".getBytes());
GHRepository ghRepository = mock(GHRepository.class);

when(pullRequestSender.forkRepositoriesFoundAndGetPathToDockerfiles(contentsFoundWithImage, gitForkBranch)).thenReturn(pathToDockerfilesInParentRepo);
when(gitHubContentToProcess.getParent()).thenReturn(ghRepository);
//Fetch the content of the renovate.json file for the 3 repos.
// The first one returns a file with regular json content.
// The second one returns a file with the key 'enabled' set to 'false' to replicate a repo that has been onboarded to renovate but has it disabled
// The third repo does not have the renovate.json file
when(ghRepository.getFileContent(anyString())).thenReturn(content).thenReturn(content).thenThrow(new FileNotFoundException());
when(ghRepository.getFullName()).thenReturn("org/repo");
when(content.read()).thenReturn(inputStream1).thenReturn(inputStream2);

pullRequests.prepareToCreate(ns, pullRequestSender, contentsFoundWithImage,
gitForkBranch, dockerfileGitHubUtil, rateLimiter);

//Verify that the DFIU PR is skipped for the first repo, but is sent to the other two repos
verify(dockerfileGitHubUtil, times(2)).changeDockerfiles(eq(ns),
eq(pathToDockerfilesInParentRepo),
eq(gitHubContentToProcess), anyList(), eq(gitForkBranch),
eq(rateLimiter));
}

@Test
public void testisRenovateEnabledReturnsFalseIfRenovateConfigFileNotFound() throws IOException {
PullRequests pullRequests = new PullRequests();
List<String> filePaths = Collections.singletonList("renovate.json");
GitHubContentToProcess gitHubContentToProcess = mock(GitHubContentToProcess.class);
GHRepository ghRepository = mock(GHRepository.class);
when(gitHubContentToProcess.getParent()).thenReturn(ghRepository);
when(ghRepository.getFileContent(anyString())).thenThrow(new FileNotFoundException());
Assert.assertFalse(pullRequests.isRenovateEnabled(filePaths, gitHubContentToProcess));
}

@Test
public void testisRenovateEnabledReturnsFalseIfRenovateConfigFileFoundButIsDisabled() throws IOException {
PullRequests pullRequests = new PullRequests();
List<String> filePaths = Collections.singletonList("renovate.json");
GitHubContentToProcess gitHubContentToProcess = mock(GitHubContentToProcess.class);
GHRepository ghRepository = mock(GHRepository.class);
GHContent content = mock(GHContent.class);
InputStream inputStream = new ByteArrayInputStream("{enabled:false}".getBytes());
when(gitHubContentToProcess.getParent()).thenReturn(ghRepository);
when(ghRepository.getFileContent(anyString())).thenReturn(content);
when(content.read()).thenReturn(inputStream);
Assert.assertFalse(pullRequests.isRenovateEnabled(filePaths, gitHubContentToProcess));
}

@Test
public void testisRenovateEnabledReturnsTrueIfRenovateConfigFileFoundButEnabledKeyNotFound() throws IOException {
PullRequests pullRequests = new PullRequests();
List<String> filePaths = Collections.singletonList("renovate.json");
GitHubContentToProcess gitHubContentToProcess = mock(GitHubContentToProcess.class);
GHRepository ghRepository = mock(GHRepository.class);
GHContent content = mock(GHContent.class);
InputStream inputStream = new ByteArrayInputStream("{someKey:someValue}".getBytes());
when(gitHubContentToProcess.getParent()).thenReturn(ghRepository);
when(ghRepository.getFileContent(anyString())).thenReturn(content);
when(content.read()).thenReturn(inputStream);
Assert.assertTrue(pullRequests.isRenovateEnabled(filePaths, gitHubContentToProcess));
}

@Test
public void testisRenovateEnabledReturnsTrueIfRenovateConfigFileFoundAndResourcesThrowAnException() throws IOException {
PullRequests pullRequests = new PullRequests();
List<String> filePaths = Collections.singletonList("renovate.json");
GitHubContentToProcess gitHubContentToProcess = mock(GitHubContentToProcess.class);
GHRepository ghRepository = mock(GHRepository.class);
GHContent content = mock(GHContent.class);
when(gitHubContentToProcess.getParent()).thenReturn(ghRepository);
when(ghRepository.getFileContent(anyString())).thenReturn(content);
when(content.read()).thenThrow(new IOException());
Assert.assertFalse(pullRequests.isRenovateEnabled(filePaths, gitHubContentToProcess));
}

@Test
public void testisRenovateEnabledReturnsTrueIfRenovateConfigFileFoundAndEnabledKeySetToTrue() throws IOException {
PullRequests pullRequests = new PullRequests();
List<String> filePaths = Collections.singletonList("renovate.json");
GitHubContentToProcess gitHubContentToProcess = mock(GitHubContentToProcess.class);
GHRepository ghRepository = mock(GHRepository.class);
GHContent content = mock(GHContent.class);
InputStream inputStream = new ByteArrayInputStream("{enabled:true}".getBytes());
when(gitHubContentToProcess.getParent()).thenReturn(ghRepository);
when(ghRepository.getFileContent(anyString())).thenReturn(content);
when(content.read()).thenReturn(inputStream);
Assert.assertTrue(pullRequests.isRenovateEnabled(filePaths, gitHubContentToProcess));
}
}
Loading