Resetting the list of orgs to include or exclude in the search for each image #326

avimanyum · 2022-03-08T01:02:26Z

While running the all subcommand, there is a Git search that happens for each image found in the tag store one after the other. While processing one of the images, if the hashmap orgsToIncludeInSearch is modified (this can happen one image returns more than 1000 search results. Refer to this for more details), the modified map persists and is carried over while performing the search for the next image in the tag store. This is a bug.
This PR resets the hashmap before performing the search for each image.

codeclimate · 2022-03-08T01:04:12Z

...le-image-update/src/main/java/com/salesforce/dockerfileimageupdate/subcommands/impl/All.java

-
+                Map<String, Boolean> orgsToIncludeInSearch = new HashMap<>();
+                if (ns.get(Constants.GIT_ORG) != null) {
+                    // If there is a Git org specified, that needs to be included in the search query. In


Line is longer than 100 characters (found 105).

codeclimate · 2022-03-08T01:04:12Z

...le-image-update/src/main/java/com/salesforce/dockerfileimageupdate/subcommands/impl/All.java

+                Map<String, Boolean> orgsToIncludeInSearch = new HashMap<>();
+                if (ns.get(Constants.GIT_ORG) != null) {
+                    // If there is a Git org specified, that needs to be included in the search query. In
+                    // the orgsToIncludeInSearch a true value associated with an org name ensures that


Line is longer than 100 characters (found 102).

codecov · 2022-03-08T01:04:20Z

Codecov Report

Merging #326 (9ea7bc8) into master (51c97da) will increase coverage by 0.01%.
The diff coverage is 100.00%.

@@             Coverage Diff              @@
##             master     #326      +/-   ##
============================================
+ Coverage     81.47%   81.49%   +0.01%     
  Complexity      263      263              
============================================
  Files            19       19              
  Lines           934      935       +1     
  Branches        126      126              
============================================
+ Hits            761      762       +1     
  Misses          142      142              
  Partials         31       31

Impacted Files	Coverage Δ
...kerfileimageupdate/utils/DockerfileGitHubUtil.java	`85.45% <100.00%> (+0.06%)`	⬆️

afalko

It think it wants us to add a unit test for this case. If it is too difficult, I'm still OK merging.

cjneasbi · 2022-03-10T17:26:20Z

So I think this change is solving the wrong problem. The problem is a method in the call chain is programming by side effect instead of just modifying its return value. If I understand correctly the map being cleared is passed here, to here, and then is inappropriately modified at here. Thats the actual bug, getSearchResultsExcludingOrgWithMostHits should not be modifying its input parameter to which is programming by side effect. Instead we should change the return value of getSearchResultsExcludingOrgWithMostHits to also return any additional information (like the updates to the input parameter) required by the callers.

cjneasbi · 2022-03-10T17:27:26Z

...le-image-update/src/main/java/com/salesforce/dockerfileimageupdate/subcommands/impl/All.java

@@ -59,7 +52,13 @@ public void execute(final Namespace ns, final DockerfileGitHubUtil dockerfileGit
                GitForkBranch gitForkBranch = getGitForkBranch(image, tag, ns);

                log.info("Finding Dockerfiles with the image name {}...", image);
-
+                Map<String, Boolean> orgsToIncludeInSearch = new HashMap<>();


See comments about why I think this is solving the wrong problem.

The map is being modified as expected. The modification of the map happens only when we exclude orgs recursively to reduce the search space to get around the limit that Git has of returning only 1000 search results.
For every image that is being searched for from the image tag store, the map needs to start as an empty map. Only if the search of that image returns more than 1000 results does the map get modified - which is what we want.
However, when that image is processed completely and the run proceeds to the next image, this map should again start as an empty map and not automatically exclude the orgs that were excluded during the search of some previous image from the tag store.

codeclimate · 2022-03-10T18:56:10Z

...ge-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/DockerfileGitHubUtil.java

+        Map<String, Boolean> orgsToExcludeFromSearch = new HashMap<>();
+        orgsToExcludeFromSearch.putAll(orgsToExclude);
+        orgsToExcludeFromSearch.put(orgWithMaximumHits, false);
+        log.info("Running search by excluding the orgs {}.", orgsToExcludeFromSearch.keySet().toString());


Line is longer than 100 characters (found 106).

codeclimate · 2022-03-10T18:56:11Z

...ge-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/DockerfileGitHubUtil.java

        Optional<List<PagedSearchIterable<GHContent>>> contentsExcludingOrgWithMaximumHits;
-        contentsExcludingOrgWithMaximumHits = findFilesWithImage(image, orgsToExclude, gitApiSearchLimit);
+        contentsExcludingOrgWithMaximumHits = findFilesWithImage(image, orgsToExcludeFromSearch, gitApiSearchLimit);


Line is longer than 100 characters (found 116).

…image-update into reset_exclude_list

cjneasbi · 2022-03-10T19:26:35Z

...ge-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/DockerfileGitHubUtil.java

+        Map<String, Boolean> orgsToExcludeFromSearch = new HashMap<>();
+        orgsToExcludeFromSearch.putAll(orgsToExclude);
+        orgsToExcludeFromSearch.put(orgWithMaximumHits, false);
+        log.info("Running search by excluding the orgs {}.", orgsToExcludeFromSearch.keySet().toString());


I don't think you need toString here. See https://stackoverflow.com/questions/13199214/parameterized-logging-in-slf4j-how-does-it-compare-to-scalas-by-name-paramete

cjneasbi · 2022-03-10T19:28:35Z

...ge-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/DockerfileGitHubUtil.java

-        orgsToExclude.put(orgWithMaximumHits, false);
-        log.info("Running search by excluding the orgs {}.", orgsToExclude.keySet().toString());
+        Map<String, Boolean> orgsToExcludeFromSearch = new HashMap<>();
+        orgsToExcludeFromSearch.putAll(orgsToExclude);


I don't think you need putAll here if you this constructor for HashMap, https://docs.oracle.com/javase/8/docs/api/java/util/HashMap.html#HashMap-java.util.Map-

cjneasbi · 2022-03-10T19:29:38Z

...ge-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/DockerfileGitHubUtil.java

@@ -177,10 +177,12 @@ protected String getOrgNameWithMaximumHits(PagedSearchIterable<GHContent> files)
        Optional<List<PagedSearchIterable<GHContent>>> contentsForOrgWithMaximumHits;
        contentsForOrgWithMaximumHits = findFilesWithImage(image, orgsToInclude, gitApiSearchLimit);

-        orgsToExclude.put(orgWithMaximumHits, false);
-        log.info("Running search by excluding the orgs {}.", orgsToExclude.keySet().toString());
+        Map<String, Boolean> orgsToExcludeFromSearch = new HashMap<>();


Local variables should be marked as final if possible to prevent reference reassignment.

cjneasbi · 2022-03-10T20:14:25Z

...pdate/src/test/java/com/salesforce/dockerfileimageupdate/utils/DockerfileGitHubUtilTest.java

@@ -330,6 +330,7 @@ public void getSearchResultsExcludingOrgWithMostHits() throws Exception {
        Map<String, Boolean> orgsToIncludeOrExclude = new HashMap<>();

        assertEquals((dockerfileGitHubUtil.getSearchResultsExcludingOrgWithMostHits("image", contentsWithImage, orgsToIncludeOrExclude, 1000)).get().size(), 2);
+        assertEquals(orgsToIncludeOrExclude.size(), 0);


Minor: We might want to add a small comment as to why we are checking for this condition.

cjneasbi · 2022-03-10T20:15:47Z

...ge-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/DockerfileGitHubUtil.java

@@ -177,10 +177,11 @@ protected String getOrgNameWithMaximumHits(PagedSearchIterable<GHContent> files)
        Optional<List<PagedSearchIterable<GHContent>>> contentsForOrgWithMaximumHits;
        contentsForOrgWithMaximumHits = findFilesWithImage(image, orgsToInclude, gitApiSearchLimit);

-        orgsToExclude.put(orgWithMaximumHits, false);
-        log.info("Running search by excluding the orgs {}.", orgsToExclude.keySet().toString());
+        final Map<String, Boolean> orgsToExcludeFromSearch = new HashMap(orgsToExclude);


You'll want to use new HashMap<>(orgsToExclude) otherwise you'll get type warnings during compilation.

Resetting the list for each image

f9f36c2

avimanyum requested review from justinharringa and afalko as code owners March 8, 2022 01:02

codeclimate bot reviewed Mar 8, 2022

View reviewed changes

avimanyum added the bug Bug fixes label Mar 8, 2022

afalko previously approved these changes Mar 10, 2022

View reviewed changes

afalko requested a review from cjneasbi March 10, 2022 16:58

cjneasbi requested changes Mar 10, 2022

View reviewed changes

Making local copy of map

325364c

avimanyum dismissed afalko’s stale review via 325364c March 10, 2022 18:54

Merge branch 'master' into reset_exclude_list

1c2b568

codeclimate bot reviewed Mar 10, 2022

View reviewed changes

avimanyum added 2 commits March 10, 2022 10:57

Removing unnecessary change

5b9fe73

Merge branch 'reset_exclude_list' of github.com:avimanyum/dockerfile-…

6db0772

…image-update into reset_exclude_list

cjneasbi requested changes Mar 10, 2022

View reviewed changes

Adding tests

b91477f

github-actions bot added the tests Improvements / maintenance to tests label Mar 10, 2022

cjneasbi requested changes Mar 10, 2022

View reviewed changes

Addressing PR comments

9ea7bc8

cjneasbi approved these changes Mar 10, 2022

View reviewed changes

afalko approved these changes Mar 10, 2022

View reviewed changes

afalko merged commit 859c50c into salesforce:master Mar 10, 2022

justinharringa removed the tests Improvements / maintenance to tests label Mar 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Resetting the list of orgs to include or exclude in the search for each image #326

Resetting the list of orgs to include or exclude in the search for each image #326

avimanyum commented Mar 8, 2022

codeclimate bot Mar 8, 2022

codeclimate bot Mar 8, 2022

codecov bot commented Mar 8, 2022 •

edited

Loading

afalko left a comment

cjneasbi commented Mar 10, 2022

cjneasbi Mar 10, 2022

avimanyum Mar 10, 2022

codeclimate bot Mar 10, 2022

codeclimate bot Mar 10, 2022

cjneasbi Mar 10, 2022

cjneasbi Mar 10, 2022

cjneasbi Mar 10, 2022

cjneasbi Mar 10, 2022

cjneasbi Mar 10, 2022

Resetting the list of orgs to include or exclude in the search for each image #326

Resetting the list of orgs to include or exclude in the search for each image #326

Conversation

avimanyum commented Mar 8, 2022

codeclimate bot Mar 8, 2022

Choose a reason for hiding this comment

codeclimate bot Mar 8, 2022

Choose a reason for hiding this comment

codecov bot commented Mar 8, 2022 • edited Loading

Codecov Report

afalko left a comment

Choose a reason for hiding this comment

cjneasbi commented Mar 10, 2022

cjneasbi Mar 10, 2022

Choose a reason for hiding this comment

avimanyum Mar 10, 2022

Choose a reason for hiding this comment

codeclimate bot Mar 10, 2022

Choose a reason for hiding this comment

codeclimate bot Mar 10, 2022

Choose a reason for hiding this comment

cjneasbi Mar 10, 2022

Choose a reason for hiding this comment

cjneasbi Mar 10, 2022

Choose a reason for hiding this comment

cjneasbi Mar 10, 2022

Choose a reason for hiding this comment

cjneasbi Mar 10, 2022

Choose a reason for hiding this comment

cjneasbi Mar 10, 2022

Choose a reason for hiding this comment

codecov bot commented Mar 8, 2022 •

edited

Loading