Skip DFIU PRs for repos onboarded to Renovate Enterprise #1128

avimanyum · 2024-02-07T01:37:55Z

Renovate enterprise is another solution that can be used to receive remediation PRs. This change disables sending DFIU PRs for those repositories that have been onboarded to Renovate Enterprise.

codecov · 2024-02-07T01:40:37Z

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (a21d7b6) 85.69% compared to head (7fd7521) 85.91%.

Additional details and impacted files

@@             Coverage Diff              @@
##               main    #1128      +/-   ##
============================================
+ Coverage     85.69%   85.91%   +0.22%     
- Complexity      385      390       +5     
============================================
  Files            27       27              
  Lines          1272     1292      +20     
  Branches        165      167       +2     
============================================
+ Hits           1090     1110      +20     
  Misses          146      146              
  Partials         36       36

Files	Coverage Δ
.../salesforce/dockerfileimageupdate/CommandLine.java	`76.47% <100.00%> (+0.71%)`	⬆️
...esforce/dockerfileimageupdate/utils/Constants.java	`100.00% <100.00%> (ø)`
...orce/dockerfileimageupdate/utils/PullRequests.java	`100.00% <100.00%> (ø)`

cjneasbi · 2024-02-08T18:19:07Z

dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/Constants.java

@@ -51,4 +51,6 @@ 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";
+    public static final String RENOVATE_CONFIG_FILEPATH = "renovate.json";


While we will using renovate.json internally there are actually 9 different potential renovate configuration files, see https://docs.renovatebot.com/configuration-options/ . We might want to check for all of these paths.

cjneasbi · 2024-02-08T18:20:26Z

...file-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/PullRequests.java

+            InputStream is = fileContent.read();
+            BufferedReader bufferedReader = new BufferedReader(new InputStreamReader(is));


I think these should be closed after use.

cjneasbi · 2024-02-08T18:25:47Z

...file-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/PullRequests.java

+            if (json.has("enabled") && json.getBoolean("enabled") == false) {
+                return false;
+            }


I think you can replace this if statement with

return json.optBoolean("enabled", true)

If the file exists and enabled is not set it is assumed to be true, otherwise return the value of the enabled field.

cjneasbi · 2024-02-08T18:29:05Z

...file-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/PullRequests.java

+            return false;
+        }
+        return true;


I think you can remove the return inside of the catch and change the last return to return false. If the file isn't found should be only way the last return is executed so we should assume that renovate is not enabled.

cjneasbi · 2024-02-08T18:30:18Z

dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/CommandLine.java

@@ -65,6 +65,10 @@ static ArgumentParser getArgumentParser() {
                ArgumentParsers.newFor("dockerfile-image-update").addHelp(true).build()
                .description("Image Updates through Pull Request Automator");

+        parser.addArgument("-re", "--" + CHECK_FOR_RENOVATE)


What about -R instead for the short opt?

cjneasbi · 2024-02-08T19:04:44Z

...file-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/PullRequests.java

+                is.close();
+                bufferedReader.close();


You might want to do this in a try-with-resources statement, see https://stackoverflow.com/a/56151320 . If is.close throws an exception then the close on buffered reader won't happen.

cjneasbi · 2024-02-08T19:53:15Z

...file-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/PullRequests.java

+                try (InputStream is = fileContent.read();
+                     BufferedReader bufferedReader = new BufferedReader(new InputStreamReader(is))) {


I think the input stream is closed when the buffered reader is closed so I think you can inline fileContent.read() in the InputStreamReader constructor.

jeetchoudhary · 2024-02-08T20:45:31Z

dockerfile-image-update/pom.xml

@@ -69,6 +69,11 @@


    <dependencies>
+        <dependency>
+            <groupId>org.json</groupId>


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?

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

jeetchoudhary · 2024-02-08T20:57:10Z

...file-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/PullRequests.java

+     * @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 {


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);
}
}

jeetchoudhary · 2024-02-08T21:14:25Z

...file-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/PullRequests.java

+                    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);


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

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.

"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

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

or am I overlooking something?

"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

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

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.

Skip DFIU PRs for renovate onboarded repos

50df365

avimanyum requested review from afalko and jeetchoudhary as code owners February 7, 2024 01:37

avimanyum added the feature New features and improvements label Feb 7, 2024

github-actions bot added chore Chores such as updates to CI processes and other maintenance tests Improvements / maintenance to tests labels Feb 7, 2024

avimanyum added 3 commits February 7, 2024 15:01

Adding tests

bbb42f8

Updating version of dependency

62b1d6b

Fix imports

50725f7

avimanyum removed chore Chores such as updates to CI processes and other maintenance tests Improvements / maintenance to tests labels Feb 8, 2024

Checking for boolean value in config file

5bd598b

github-actions bot added chore Chores such as updates to CI processes and other maintenance tests Improvements / maintenance to tests labels Feb 8, 2024

cjneasbi reviewed Feb 8, 2024

View reviewed changes

Addressing PR comments

10a819a

cjneasbi reviewed Feb 8, 2024

View reviewed changes

Addressing PR comments

e1c9a83

cjneasbi reviewed Feb 8, 2024

View reviewed changes

avimanyum added 2 commits February 8, 2024 11:58

Adding more tests

ac16208

Making one resource inline with other

77c660f

cjneasbi previously approved these changes Feb 8, 2024

View reviewed changes

jeetchoudhary reviewed Feb 8, 2024

View reviewed changes

avimanyum added 2 commits February 8, 2024 13:44

Addressing PR comments

43a3454

Addressing PR comments

7fd7521

avimanyum dismissed cjneasbi’s stale review via 7fd7521 February 8, 2024 21:46

jeetchoudhary approved these changes Feb 8, 2024

View reviewed changes

jeetchoudhary merged commit e604ebb into salesforce:main Feb 8, 2024
12 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Skip DFIU PRs for repos onboarded to Renovate Enterprise #1128

Skip DFIU PRs for repos onboarded to Renovate Enterprise #1128

avimanyum commented Feb 7, 2024 •

edited

Loading

codecov bot commented Feb 7, 2024 •

edited

Loading

cjneasbi Feb 8, 2024

cjneasbi Feb 8, 2024

cjneasbi Feb 8, 2024

cjneasbi Feb 8, 2024

cjneasbi Feb 8, 2024

cjneasbi Feb 8, 2024

cjneasbi Feb 8, 2024

jeetchoudhary Feb 8, 2024

avimanyum Feb 8, 2024

jeetchoudhary Feb 8, 2024

jeetchoudhary Feb 8, 2024

avimanyum Feb 8, 2024 •

edited

Loading

jeetchoudhary Feb 8, 2024

jeetchoudhary Feb 8, 2024 •

edited

Loading

jeetchoudhary Feb 8, 2024

avimanyum Feb 8, 2024

avimanyum Feb 8, 2024

jeetchoudhary Feb 8, 2024

		InputStream is = fileContent.read();
		BufferedReader bufferedReader = new BufferedReader(new InputStreamReader(is));

		try (InputStream is = fileContent.read();
		BufferedReader bufferedReader = new BufferedReader(new InputStreamReader(is))) {

Skip DFIU PRs for repos onboarded to Renovate Enterprise #1128

Skip DFIU PRs for repos onboarded to Renovate Enterprise #1128

Conversation

avimanyum commented Feb 7, 2024 • edited Loading

codecov bot commented Feb 7, 2024 • edited Loading

Codecov Report

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avimanyum Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jeetchoudhary Feb 8, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

avimanyum commented Feb 7, 2024 •

edited

Loading

codecov bot commented Feb 7, 2024 •

edited

Loading

avimanyum Feb 8, 2024 •

edited

Loading

jeetchoudhary Feb 8, 2024 •

edited

Loading