update multiple Dockerfiles of the same repo #25

jdoshi1 · 2018-07-18T01:17:06Z

Updates multiple Dockerfile present in the same repo at root level and under different folders.
This change does not support custom Dockerfile names.

salesforce-cla · 2018-07-18T01:17:10Z

Thanks for the contribution! Unfortunately we can't verify the commit author(s): Jinesh Doshi <j***@s***.com>. One possible solution is to add that email to your GitHub account. Alternatively you can change your commits to another email and force push the change. After getting your commits associated with your GitHub account, refresh the status of this Pull Request.

coveralls · 2018-07-18T01:20:11Z

Coverage increased (+3.2%) to 68.26% when pulling 156a64a on update-multiple-dockerfiles into cc7da78 on master.

afalko · 2018-07-18T16:56:46Z

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

@@ -57,32 +61,43 @@ public void execute(final Namespace ns, final DockerfileGitHubUtil dockerfileGit
        }

        log.info("Retrieving all the forks...");
-        PagedIterable<GHRepository> repos = dockerfileGitHubUtil.getGHRepositories(parentToPath, currentUser);
+        PagedIterable<GHRepository> listOfcurrUserRepos =


You want listOfCurrUserRepos to maintain camelCase

afalko · 2018-07-18T16:57:56Z

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

            } catch (IOException e) {
                exceptions.add(e);
            }
        }

-        if (exceptions.size() > 0) {
+        if (!exceptions.isEmpty()) {


Positive conditions are generally more readable

Yep agreed but in this case, isn't isEmpty() more readable/preferred than size() > 0?

I'll leave it alone; not going to block things on nitpicks :)

afalko · 2018-07-18T16:58:34Z

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

            log.info("There were {} errors with changing Dockerfiles.", exceptions.size());
            throw exceptions.get(0);
        }
+
+        if (!skippedRepos.isEmpty()) {
+            log.info("List of repos skipped: {}", skippedRepos.toArray());


Does slf4j need the toArray?

Nope. Good catch, will fix it.

afalko · 2018-07-18T17:05:17Z

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

+        Iterator<String> pathToDockerfileInParentRepoIterator = pathToDockerfilesInParentRepo.get(parentName).iterator();
+        Iterator<String> imagesFoundInParentRepoIterator = imagesFoundInParentRepo.get(parentName).iterator();
+
+        while (pathToDockerfileInParentRepoIterator.hasNext() && imagesFoundInParentRepoIterator.hasNext()) {


Are pathToDockerfileInParentRepoIterator and imagesFoundInParentRepoIterator supposed to be the same number of elements?

Yes they store the same number of elements - All.java#115 and All.java#116.

Why not iterate on only one of them then? You can make the objects immutable after they are created, to make sure no one break this property later.

Hmm.. Now iterating over only one of them but I think ideally it should be a class that stores repoName with pathToDockerfiles and corresponding images in those Dockerfiles.

afalko · 2018-07-18T17:07:10Z

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


+        if (isContentModified) {
+            dockerfileGitHubUtil.createPullReq(parent, branch, forkedRepo, ns.get("m"));


You might want to create identifying constants for things getting checked in ns.get. E.g. ns.get(Constants.GIT_REPO)

afalko · 2018-07-18T17:11:25Z

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

+            GHContent content = dockerfileGitHubUtil.tryRetrievingContent(forkedRepo, pathToDockerfile, branch);
+            log.info("content: {}", content);
+            if (content != null) {
+                dockerfileGitHubUtil.modifyOnGithub(content, branch, ns.get(Constants.IMG), ns.get(Constants.TAG), ns.get("c"));


Constant for "c" as well

will create a constant for it 👍

afalko · 2018-07-18T17:12:46Z

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

@@ -68,10 +73,10 @@ public GHRepository getRepo(String repoName) throws IOException {
            throw new IOException("Invalid image name.");
        }
        search.q("\"FROM " + query + "\"");
-        log.debug("Searching for {}", query);
+        log.info("Searching for {}", query);


change change this to info?

Oops.. I forgot to switch it back to debug.

I believe there is a --verbose/--debug flag that auto-gives you more output.

afalko · 2018-07-18T17:14:56Z

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

@@ -488,7 +548,7 @@ public void testCreatePullReq_Delete() throws Exception {
        when(gitHubUtil.createPullReq(any(), anyString(), any(), anyString(), eq(Constants.PULL_REQ_ID))).thenReturn(1);
        doCallRealMethod().when(gitHubUtil).safeDeleteRepo(forkRepo);
        dockerfileGitHubUtil = new DockerfileGitHubUtil(gitHubUtil);
-        dockerfileGitHubUtil.createPullReq(new GHRepository(), "branch", forkRepo, "Automatic Dockerfile Image Updater");
+        dockerfileGitHubUtil.createPullReq(new GHRepository(), "branch", forkRepo, null);


why set to null?

Just to test the null scenario. We have a test that already provides a non-null message.

afalko · 2018-07-18T18:42:10Z

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

+                if (pr != null) {
+                    // close the pull-request since the fork is out of date
+                    log.info("closing existing pr: {}", pr.getUrl());
+                    pr.close();


Would it be better to close this in a try-finally?

Good point. I think I can use try catch to log about not able to close PR..and move ahead.

try catch around closing pr

afalko · 2018-07-20T17:43:39Z

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

+        for (String pathToDockerfile : pathToDockerfilesInParentRepo.get(parentName)) {
+            GHContent content = dockerfileGitHubUtil.tryRetrievingContent(forkedRepo, pathToDockerfile, branch);
+            log.info("content: {}", content);
+            if (content != null) {


If you can flip this if else to make condition positive

afalko · 2018-07-20T17:44:38Z

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

@@ -71,7 +80,7 @@ public GHRepository getRepo(String repoName) throws IOException {
        log.debug("Searching for {}", query);
        PagedSearchIterable<GHContent> files = search.list();
        int totalCount = files.getTotalCount();
-        log.debug("Number of files found for {}: {}", query, totalCount);
+        log.info("Number of files found for {}:{}", query, totalCount);


Forgot to flip this back to debug?

justinharringa · 2018-07-24T18:04:31Z

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

            } else {
-                log.info("Forking {}...", parent.getFullName());


Did this move?

Just 2 lines below :)

Uh oops... /Runs away

Thanks! 😆

jdoshi1 · 2018-07-25T22:13:37Z

@afalko, Seems like integration tests are broken. Created a new issue to fix it - #29

jdoshi1 · 2018-07-26T21:14:37Z

@afalko had missed addressing 2 comments. It should be good to merge if everything looks good.

afalko · 2018-07-26T22:23:34Z

@jdoshi1 Itests passed locally. Coveralls is showing a light decrease between your commits, so it is red.

afalko

Local itests pass

update multiple Dockerfiles of the same repo

1b48508

jdoshi1 requested review from justinharringa and afalko July 18, 2018 01:17

salesforce-cla bot added the cla:missing label Jul 18, 2018

salesforce-cla bot removed the cla:missing label Jul 18, 2018

remove unwanted debug logs

20483c5

jdoshi1 requested a review from npwolf July 18, 2018 01:29

afalko reviewed Jul 18, 2018

View reviewed changes

jdoshi1 and others added 3 commits July 18, 2018 12:27

use constants for accessing commandline namespace values

44f2832

try catch around closing pr

rename method and minor fixes

fc75976

Merge branch 'master' into update-multiple-dockerfiles

2a83bf9

afalko reviewed Jul 20, 2018

View reviewed changes

justinharringa reviewed Jul 24, 2018

View reviewed changes

Merge branch 'master' into update-multiple-dockerfiles

77eb466

jdoshi1 and others added 2 commits July 26, 2018 13:59

Merge branch 'master' into update-multiple-dockerfiles

6796aa3

use positive condition

156a64a

afalko approved these changes Jul 26, 2018

View reviewed changes

afalko merged commit 5304960 into master Jul 26, 2018

afalko deleted the update-multiple-dockerfiles branch July 26, 2018 22:25

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update multiple Dockerfiles of the same repo #25

update multiple Dockerfiles of the same repo #25

jdoshi1 commented Jul 18, 2018

salesforce-cla bot commented Jul 18, 2018

coveralls commented Jul 18, 2018 •

edited

Loading

afalko Jul 18, 2018

afalko Jul 18, 2018

jdoshi1 Jul 18, 2018

afalko Jul 20, 2018

afalko Jul 18, 2018

jdoshi1 Jul 18, 2018

afalko Jul 18, 2018

jdoshi1 Jul 18, 2018

afalko Jul 18, 2018 •

edited

Loading

jdoshi1 Jul 18, 2018

afalko Jul 18, 2018

afalko Jul 18, 2018

jdoshi1 Jul 18, 2018

afalko Jul 18, 2018

jdoshi1 Jul 18, 2018

afalko Jul 18, 2018

afalko Jul 18, 2018

jdoshi1 Jul 18, 2018 •

edited

Loading

afalko Jul 18, 2018

jdoshi1 Jul 18, 2018 •

edited

Loading

afalko Jul 20, 2018

afalko Jul 20, 2018

justinharringa Jul 24, 2018

jdoshi1 Jul 24, 2018

justinharringa Jul 24, 2018

jdoshi1 commented Jul 25, 2018 •

edited

Loading

jdoshi1 commented Jul 26, 2018

afalko commented Jul 26, 2018

afalko left a comment


		if (isContentModified) {
		dockerfileGitHubUtil.createPullReq(parent, branch, forkedRepo, ns.get("m"));

update multiple Dockerfiles of the same repo #25

update multiple Dockerfiles of the same repo #25

Conversation

jdoshi1 commented Jul 18, 2018

salesforce-cla bot commented Jul 18, 2018

coveralls commented Jul 18, 2018 • 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

afalko Jul 18, 2018 • 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

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

jdoshi1 Jul 18, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jdoshi1 Jul 18, 2018 • 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

Choose a reason for hiding this comment

jdoshi1 commented Jul 25, 2018 • edited Loading

jdoshi1 commented Jul 26, 2018

afalko commented Jul 26, 2018

afalko left a comment

Choose a reason for hiding this comment

coveralls commented Jul 18, 2018 •

edited

Loading

afalko Jul 18, 2018 •

edited

Loading

jdoshi1 Jul 18, 2018 •

edited

Loading

jdoshi1 Jul 18, 2018 •

edited

Loading

jdoshi1 commented Jul 25, 2018 •

edited

Loading