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.

Sign up for GitHub

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

Handle JSONException while reading Renovate configuration file #1130

Merged
merged 7 commits into from
Feb 13, 2024
Merged

Handle JSONException while reading Renovate configuration file #1130

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
53a890a
Catch json exception
avimanyum Feb 13, 2024
3eb7b54
Merge pull request #3 from avimanyum/W-14925234
avimanyum Feb 13, 2024
01dbc9d
Merge branch 'main' into main
avimanyum Feb 13, 2024
3a525c1
Add expected return value to error message
avimanyum Feb 13, 2024
e6f7b19
Merge pull request #4 from avimanyum/W-14925234
avimanyum Feb 13, 2024
678785f
Changing log level to warn
avimanyum Feb 13, 2024
1b2fa73
Merge pull request #5 from avimanyum/W-14925234
avimanyum Feb 13, 2024
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
7 changes: 5 additions & 2 deletions ...e-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/PullRequests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
import net.sourceforge.argparse4j.inf.*;
import org.json.JSONObject;
import org.json.JSONTokener;
import org.json.JSONException;
import org.kohsuke.github.*;
import org.slf4j.*;

Expand Down Expand Up @@ -64,9 +65,11 @@ protected boolean isRenovateEnabled(List<String> filePaths, GitHubContentToProce
//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 readJsonFromContent(fork.getParent().getFileContent(filePath)).optBoolean("enabled", true);
} catch (FileNotFoundException e) {
log.debug("The file with name {} not found in the repository. Exception: {}", filePath, e.getMessage());
log.debug("The file with name {} not found in the repository.Returning false. Exception: {}", filePath, e.getMessage());
Copy link
Member

Choose a reason for hiding this comment

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

FileNotFoundException error should be in the debug log because we are expecting this for most of the repos. But do we want to do that for IO and JSON exceptions as well? We should log it in warn mode if they are expected to be raised in corner cases

Copy link
Contributor Author

Choose a reason for hiding this comment

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

@jeetchoudhary Updated

} catch (IOException e) {
log.debug("Exception while trying to close a resource. Exception: {}", e.getMessage());
log.warn("Exception while trying to close a resource. Returning false. Exception: {}", e.getMessage());
} catch (JSONException e) {
log.warn("Exception while trying to read the renovate configuration file. Returning false. Exception: {}", e.getMessage());
}
}
return false;
Expand Down
16 changes: 15 additions & 1 deletion ...age-update/src/test/java/com/salesforce/dockerfileimageupdate/utils/PullRequestsTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@
import com.salesforce.dockerfileimageupdate.model.*;
import com.salesforce.dockerfileimageupdate.process.*;
import net.sourceforge.argparse4j.inf.*;
import org.json.JSONException;
import org.kohsuke.github.*;
import org.mockito.*;
import org.testng.*;
Expand Down Expand Up @@ -199,7 +200,7 @@ public void testisRenovateEnabledReturnsTrueIfRenovateConfigFileFoundButEnabledK
}

@Test
public void testisRenovateEnabledReturnsTrueIfRenovateConfigFileFoundAndResourcesThrowAnException() throws IOException {
public void testisRenovateEnabledReturnsFalseIfRenovateConfigFileFoundAndResourcesThrowAnException() throws IOException {
PullRequests pullRequests = new PullRequests();
List<String> filePaths = Collections.singletonList("renovate.json");
GitHubContentToProcess gitHubContentToProcess = mock(GitHubContentToProcess.class);
Expand All @@ -211,6 +212,19 @@ public void testisRenovateEnabledReturnsTrueIfRenovateConfigFileFoundAndResource
Assert.assertFalse(pullRequests.isRenovateEnabled(filePaths, gitHubContentToProcess));
}

@Test
public void testisRenovateEnabledReturnsFalseIfRenovateConfigFileFoundAndJSONParsingThrowsAnException() 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 JSONException(""));
Assert.assertFalse(pullRequests.isRenovateEnabled(filePaths, gitHubContentToProcess));
}

@Test
public void testisRenovateEnabledReturnsTrueIfRenovateConfigFileFoundAndEnabledKeySetToTrue() throws IOException {
PullRequests pullRequests = new PullRequests();
Expand Down