Merge pull request #152 from salesforce/meaningul-pr-body-parent-child

justinharringa · web-flow · commit 294b8109f3c4 · 2020-07-11T13:09:02.000-07:00
Add a meaningful PR body for parent/child commands
diff --git a/.github/workflows/cd.yml b/.github/workflows/cd.yml
@@ -72,15 +72,15 @@ jobs:
         with:
           name: dockerfile-image-update
       - name: Upload coverage to Code Climate
-        uses: paambaati/codeclimate-action@v2.5.5
+        uses: paambaati/codeclimate-action@v2.6.0
         env:
           CC_TEST_REPORTER_ID: ${{ secrets.CODE_CLIMATE_REPORTER_ID }}
           JACOCO_SOURCE_PATH: "${{github.workspace}}/dockerfile-image-update/src/main/java"
         with:
           # The report file must be there, otherwise Code Climate won't find it
           coverageCommand: echo "already done"
-          coverageLocations:
-            "${{github.workspace}}/dockerfile-image-update/target/site/jacoco/jacoco.xml:jacoco"
+          coverageLocations: |
+            ${{github.workspace}}/dockerfile-image-update/target/site/jacoco/jacoco.xml:jacoco
   master-deploy:
     needs: integration-test
     runs-on: ubuntu-latest
diff --git a/dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/model/PullRequestInfo.java b/dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/model/PullRequestInfo.java
@@ -0,0 +1,43 @@
+package com.salesforce.dockerfileimageupdate.model;
+
+import com.salesforce.dockerfileimageupdate.utils.Constants;
+
+public class PullRequestInfo {
+    public static final String BODY_TEMPLATE =
+            "`%s` changed recently. This pull request ensures you're using the latest version of the image and " +
+            "changes `%s` to the latest tag: `%s`\n" +
+            "\n" +
+            "New base image: `%s`";
+    public static final String DEFAULT_TITLE = "Automatic Dockerfile Image Updater";
+    public static final String OLD_CONSTANT_BODY = Constants.PULL_REQ_ID;
+    private final String title;
+    private final String image;
+    private final String tag;
+
+    public PullRequestInfo(String title, String image, String tag) {
+        if (title == null || title.trim().isEmpty()) {
+            this.title = DEFAULT_TITLE;
+        } else {
+            this.title = title;
+        }
+        this.image = image;
+        this.tag = tag;
+    }
+
+    /**
+     * @return title for the pull request
+     */
+    public String getTitle() {
+        return title;
+    }
+
+    /**
+     * @return formatted pull request body
+     */
+    public String getBody() {
+        if (this.image == null && this.tag == null) {
+            return OLD_CONSTANT_BODY;
+        }
+        return String.format(BODY_TEMPLATE, image, image, tag, image + ":" + tag);
+    }
+}
diff --git a/dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/subcommands/impl/All.java b/dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/subcommands/impl/All.java
@@ -14,6 +14,7 @@
 import com.google.gson.JsonParseException;
 import com.google.gson.JsonParser;
 import com.salesforce.dockerfileimageupdate.SubCommand;
+import com.salesforce.dockerfileimageupdate.model.PullRequestInfo;
 import com.salesforce.dockerfileimageupdate.repository.GitHub;
 import com.salesforce.dockerfileimageupdate.subcommands.ExecutableWithNamespace;
 import com.salesforce.dockerfileimageupdate.utils.Constants;
@@ -240,7 +241,11 @@ protected void changeDockerfiles(Namespace ns,
         }
 
         if (isContentModified) {
-            dockerfileGitHubUtil.createPullReq(parent, branch, forkedRepo, ns.get(Constants.GIT_PR_TITLE));
+            // Passing null for image/tag to temporarily maintain old behavior of using constant.
+            PullRequestInfo pullRequestInfo =   
+                    new PullRequestInfo(ns.get(Constants.GIT_PR_TITLE),null, null);
+
+            dockerfileGitHubUtil.createPullReq(parent, branch, forkedRepo, pullRequestInfo);
         }
     }
 
diff --git a/dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/subcommands/impl/Child.java b/dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/subcommands/impl/Child.java
@@ -10,6 +10,7 @@
 
 import com.salesforce.dockerfileimageupdate.SubCommand;
 import com.salesforce.dockerfileimageupdate.model.GitForkBranch;
+import com.salesforce.dockerfileimageupdate.model.PullRequestInfo;
 import com.salesforce.dockerfileimageupdate.subcommands.ExecutableWithNamespace;
 import com.salesforce.dockerfileimageupdate.utils.Constants;
 import com.salesforce.dockerfileimageupdate.utils.DockerfileGitHubUtil;
@@ -20,7 +21,7 @@
 
 import java.io.IOException;
 
-@SubCommand(help="updates one specific repository with given tag",
+@SubCommand(help = "updates one specific repository with given tag",
         requiredParams = {Constants.GIT_REPO, Constants.IMG, Constants.FORCE_TAG}, optionalParams = {"s", Constants.STORE})
 public class Child implements ExecutableWithNamespace {
     private static final Logger log = LoggerFactory.getLogger(Child.class);
@@ -45,11 +46,18 @@ public void execute(final Namespace ns, final DockerfileGitHubUtil dockerfileGit
         }
 
         GitForkBranch gitForkBranch = new GitForkBranch(img, forceTag, branch);
+        PullRequestInfo pullRequestInfo =
+                new PullRequestInfo(ns.get(Constants.GIT_PR_TITLE),
+                        gitForkBranch.getImageName(),
+                        gitForkBranch.getImageTag());
 
         dockerfileGitHubUtil.createOrUpdateForkBranchToParentDefault(repo, fork, gitForkBranch);
 
         log.info("Modifying on Github...");
         dockerfileGitHubUtil.modifyAllOnGithub(fork, gitForkBranch.getBranchName(), img, forceTag);
-        dockerfileGitHubUtil.createPullReq(repo, gitForkBranch.getBranchName(), fork, ns.get(Constants.GIT_PR_TITLE));
+        dockerfileGitHubUtil.createPullReq(repo,
+                gitForkBranch.getBranchName(),
+                fork,
+                pullRequestInfo);
     }
 }
diff --git a/dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/subcommands/impl/Parent.java b/dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/subcommands/impl/Parent.java
@@ -12,6 +12,7 @@
 import com.salesforce.dockerfileimageupdate.SubCommand;
 import com.salesforce.dockerfileimageupdate.model.GitForkBranch;
 import com.salesforce.dockerfileimageupdate.model.GitHubContentToProcess;
+import com.salesforce.dockerfileimageupdate.model.PullRequestInfo;
 import com.salesforce.dockerfileimageupdate.process.ForkableRepoValidator;
 import com.salesforce.dockerfileimageupdate.process.GitHubPullRequestSender;
 import com.salesforce.dockerfileimageupdate.subcommands.ExecutableWithNamespace;
@@ -125,8 +126,14 @@ protected void changeDockerfiles(Namespace ns,
         }
 
         if (isContentModified) {
+            PullRequestInfo pullRequestInfo =
+                    new PullRequestInfo(ns.get(Constants.GIT_PR_TITLE),
+                            gitForkBranch.getImageName(), gitForkBranch.getImageTag());
             // TODO: get the new PR number and cross post over to old ones
-            dockerfileGitHubUtil.createPullReq(parent, gitForkBranch.getBranchName(), forkedRepo, ns.get(Constants.GIT_PR_TITLE));
+            dockerfileGitHubUtil.createPullReq(parent,
+                    gitForkBranch.getBranchName(),
+                    forkedRepo,
+                    pullRequestInfo);
             // TODO: Run through PRs in fork to see if they have head branches that match the prefix and close those?
         }
     }
diff --git a/dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/DockerfileGitHubUtil.java b/dockerfile-image-update/src/main/java/com/salesforce/dockerfileimageupdate/utils/DockerfileGitHubUtil.java
@@ -11,6 +11,7 @@
 import com.google.common.collect.Multimap;
 import com.salesforce.dockerfileimageupdate.model.FromInstruction;
 import com.salesforce.dockerfileimageupdate.model.GitForkBranch;
+import com.salesforce.dockerfileimageupdate.model.PullRequestInfo;
 import com.salesforce.dockerfileimageupdate.storage.GitHubJsonStore;
 import org.kohsuke.github.*;
 import org.slf4j.Logger;
@@ -212,13 +213,11 @@ public GitHubJsonStore getGitHubJsonStore(String store) {
 
     public void createPullReq(GHRepository origRepo,
                               String branch, GHRepository forkRepo,
-                              String title) throws InterruptedException, IOException {
-        if (title == null) {
-            title = "Automatic Dockerfile Image Updater";
-        }
+                              PullRequestInfo pullRequestInfo) throws InterruptedException, IOException {
         // TODO: This may loop forever in the event of constant -1 pullRequestExitCodes...
         while (true) {
-            int pullRequestExitCode = gitHubUtil.createPullReq(origRepo, branch, forkRepo, title, Constants.PULL_REQ_ID);
+            int pullRequestExitCode = gitHubUtil.createPullReq(origRepo,
+                    branch, forkRepo, pullRequestInfo.getTitle(), pullRequestInfo.getBody());
             if (pullRequestExitCode == 0) {
                 return;
             } else if (pullRequestExitCode == 1) {
diff --git a/dockerfile-image-update/src/test/java/com/salesforce/dockerfileimageupdate/model/PullRequestInfoTest.java b/dockerfile-image-update/src/test/java/com/salesforce/dockerfileimageupdate/model/PullRequestInfoTest.java
@@ -0,0 +1,44 @@
+package com.salesforce.dockerfileimageupdate.model;
+
+import org.testng.annotations.Test;
+
+import static com.salesforce.dockerfileimageupdate.model.PullRequestInfo.*;
+import static org.testng.Assert.*;
+
+public class PullRequestInfoTest {
+
+    public static final String IMAGE = "myimage";
+    public static final String TAG = "tag";
+    public static final String TITLE = "title";
+
+    @Test
+    public void testGetBody() {
+        String imageAndTag = IMAGE + ":" + TAG;
+        PullRequestInfo pullRequestInfo = new PullRequestInfo(TITLE, IMAGE, TAG);
+        assertEquals(pullRequestInfo.getBody(), String.format(BODY_TEMPLATE, IMAGE, IMAGE, TAG, imageAndTag));
+    }
+
+    @Test
+    public void testGetBodyIsOldConstantIfImageAndTagNull() {
+        PullRequestInfo pullRequestInfo = new PullRequestInfo(TITLE, null, null);
+        assertEquals(pullRequestInfo.getBody(), OLD_CONSTANT_BODY);
+    }
+
+    @Test
+    public void testGetTitle() {
+        PullRequestInfo pullRequestInfo = new PullRequestInfo(TITLE, IMAGE, TAG);
+        assertEquals(pullRequestInfo.getTitle(), TITLE);
+    }
+
+    @Test
+    public void testGetDefaultTitleIfNull() {
+        PullRequestInfo pullRequestInfo = new PullRequestInfo(null, IMAGE, TAG);
+        assertEquals(pullRequestInfo.getTitle(), DEFAULT_TITLE);
+    }
+
+    @Test
+    public void testGetDefaultTitleIfEmpty() {
+        PullRequestInfo pullRequestInfo = new PullRequestInfo("   ", IMAGE, TAG);
+        assertEquals(pullRequestInfo.getTitle(), DEFAULT_TITLE);
+    }
+}
diff --git a/dockerfile-image-update/src/test/java/com/salesforce/dockerfileimageupdate/subcommands/impl/AllTest.java b/dockerfile-image-update/src/test/java/com/salesforce/dockerfileimageupdate/subcommands/impl/AllTest.java
@@ -170,7 +170,7 @@ public void testChangeDockerfiles_returnIfNotDesiredParent() throws Exception {
         Mockito.verify(dockerfileGitHubUtil, times(0))
                 .modifyOnGithub(any(), anyString(), anyString(), anyString(), anyString());
         Mockito.verify(dockerfileGitHubUtil, times(0))
-                .createPullReq(eq(parentRepo), anyString(), eq(forkedRepo), anyString());
+                .createPullReq(eq(parentRepo), anyString(), eq(forkedRepo), any());
     }
 
     @Test
@@ -192,7 +192,7 @@ public void testChangeDockerfiles_returnWhenForkedRepoNotFound() throws Exceptio
                 anyString(), anyString());
         Mockito.verify(dockerfileGitHubUtil, times(0))
                 .modifyOnGithub(any(), anyString(), anyString(), anyString(), anyString());
-        Mockito.verify(dockerfileGitHubUtil, times(0)).createPullReq(any(), anyString(), any(), anyString());
+        Mockito.verify(dockerfileGitHubUtil, times(0)).createPullReq(any(), anyString(), any(), any());
 
     }
 
@@ -248,7 +248,7 @@ public void testChangeDockerfiles_pullRequestCreation() throws Exception {
         Mockito.verify(dockerfileGitHubUtil, times(1))
                 .modifyOnGithub(any(), eq("branch"), eq("image2"), eq("tag2"), anyString());
         Mockito.verify(dockerfileGitHubUtil, times(1)).createPullReq(eq(parentRepo),
-                eq("branch"), eq(forkedRepo), anyString());
+                eq("branch"), eq(forkedRepo), any());
     }
 
     @Test
@@ -318,7 +318,7 @@ public void testOnePullRequestForMultipleDockerfilesInSameRepo() throws Exceptio
 
         // Only one PR created on the repo with changes to both Dockerfiles.
         Mockito.verify(dockerfileGitHubUtil, times(1)).createPullReq(eq(parentRepo),
-                eq("branch"), eq(forkedRepo), anyString());
+                eq("branch"), eq(forkedRepo), any());
     }
 
     @Test
@@ -372,7 +372,7 @@ public void testNoPullRequestForMissingDockerfile() throws Exception {
         Mockito.verify(dockerfileGitHubUtil, times(0))
                 .modifyOnGithub(any(), anyString(), anyString(), anyString(), anyString());
         Mockito.verify(dockerfileGitHubUtil, times(0)).createPullReq(eq(parentRepo),
-                anyString(), eq(forkedRepo), anyString());
+                anyString(), eq(forkedRepo), any());
     }
 
     @Test
@@ -444,7 +444,7 @@ public void checkPullRequestNotMadeForArchived() throws Exception {
         all.changeDockerfiles(ns, pathToDockerfilesInParentRepo, null, null, forkRepo, null);
 
         Mockito.verify(dockerfileGitHubUtil, Mockito.never())
-                .createPullReq(Mockito.any(), anyString(), Mockito.any(), anyString());
+                .createPullReq(Mockito.any(), anyString(), Mockito.any(), any());
         //Make sure we at least check if its archived
         Mockito.verify(parentRepo, Mockito.times(2)).isArchived();
     }
diff --git a/dockerfile-image-update/src/test/java/com/salesforce/dockerfileimageupdate/subcommands/impl/ChildTest.java b/dockerfile-image-update/src/test/java/com/salesforce/dockerfileimageupdate/subcommands/impl/ChildTest.java
@@ -54,12 +54,12 @@ public void checkPullRequestMade(Map<String, Object> inputMap) throws Exception
         doNothing().when(dockerfileGitHubUtil).modifyAllOnGithub(Mockito.any(), Mockito.any(), Mockito.any(), Mockito.any());
         GitHubJsonStore gitHubJsonStore = mock(GitHubJsonStore.class);
         when(dockerfileGitHubUtil.getGitHubJsonStore(anyString())).thenReturn(gitHubJsonStore);
-        doNothing().when(dockerfileGitHubUtil).createPullReq(Mockito.any(), anyString(), Mockito.any(), anyString());
+        doNothing().when(dockerfileGitHubUtil).createPullReq(Mockito.any(), anyString(), Mockito.any(), any());
 
         child.execute(ns, dockerfileGitHubUtil);
 
         Mockito.verify(dockerfileGitHubUtil, atLeastOnce())
-                .createPullReq(Mockito.any(), anyString(), Mockito.any(), anyString());
+                .createPullReq(Mockito.any(), anyString(), Mockito.any(), any());
     }
 
     @Test
diff --git a/dockerfile-image-update/src/test/java/com/salesforce/dockerfileimageupdate/subcommands/impl/ParentTest.java b/dockerfile-image-update/src/test/java/com/salesforce/dockerfileimageupdate/subcommands/impl/ParentTest.java
@@ -69,7 +69,7 @@ public void testChangeDockerfiles_pullRequestCreation() throws Exception {
         Mockito.verify(dockerfileGitHubUtil, times(1))
                 .modifyOnGithub(eq(content), eq("image-tag"), eq("image"), eq("tag"), anyString());
         Mockito.verify(dockerfileGitHubUtil, times(1))
-                .createPullReq(eq(parentRepo), eq("image-tag"), eq(forkedRepo), anyString());
+                .createPullReq(eq(parentRepo), eq("image-tag"), eq(forkedRepo), any());
     }
 
     @Test
@@ -120,7 +120,7 @@ public void testOnePullRequestForMultipleDockerfilesInSameRepo() throws Exceptio
 
         // Only one PR created on the repo with changes to both Dockerfiles.
         Mockito.verify(dockerfileGitHubUtil, times(1)).createPullReq(eq(parentRepo),
-                eq("image-tag"), eq(forkedRepo), anyString());
+                eq("image-tag"), eq(forkedRepo), any());
     }
 
     @Test
@@ -160,6 +160,6 @@ public void testNoPullRequestForMissingDockerfile() throws Exception {
         Mockito.verify(dockerfileGitHubUtil, times(0))
                 .modifyOnGithub(any(), anyString(), anyString(), anyString(), anyString());
         Mockito.verify(dockerfileGitHubUtil, times(0)).createPullReq(eq(parentRepo),
-                anyString(), eq(forkedRepo), anyString());
+                anyString(), eq(forkedRepo), any());
     }
 }
diff --git a/dockerfile-image-update/src/test/java/com/salesforce/dockerfileimageupdate/utils/DockerfileGitHubUtilTest.java b/dockerfile-image-update/src/test/java/com/salesforce/dockerfileimageupdate/utils/DockerfileGitHubUtilTest.java
@@ -9,6 +9,7 @@
 package com.salesforce.dockerfileimageupdate.utils;
 
 import com.salesforce.dockerfileimageupdate.model.GitForkBranch;
+import com.salesforce.dockerfileimageupdate.model.PullRequestInfo;
 import org.kohsuke.github.*;
 import org.mockito.Mock;
 import org.testng.annotations.DataProvider;
@@ -22,6 +23,7 @@
 import java.util.Optional;
 import java.util.concurrent.TimeUnit;
 
+import static com.salesforce.dockerfileimageupdate.model.PullRequestInfo.DEFAULT_TITLE;
 import static org.mockito.Matchers.*;
 import static org.mockito.Mockito.*;
 import static org.testng.Assert.*;
@@ -483,8 +485,9 @@ public void testCreatePullReq_Loop() throws Exception {
 
         when(gitHubUtil.createPullReq(any(), anyString(), any(), anyString(), eq(Constants.PULL_REQ_ID))).thenReturn(-1, -1, -1, 0);
         dockerfileGitHubUtil = new DockerfileGitHubUtil(gitHubUtil);
-        dockerfileGitHubUtil.createPullReq(new GHRepository(), "branch", new GHRepository(), "Automatic Dockerfile Image Updater");
-        verify(gitHubUtil, times(4)).createPullReq(any(), anyString(), any(), anyString(), eq(Constants.PULL_REQ_ID));
+        PullRequestInfo pullRequestInfo = new PullRequestInfo(null, null, null);
+        dockerfileGitHubUtil.createPullReq(new GHRepository(), "branch", new GHRepository(), pullRequestInfo);
+        verify(gitHubUtil, times(4)).createPullReq(any(), anyString(), any(), eq(DEFAULT_TITLE), eq(Constants.PULL_REQ_ID));
     }
 
     @Test
@@ -503,7 +506,8 @@ 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, null);
+        PullRequestInfo pullRequestInfo = new PullRequestInfo(null, null, null);
+        dockerfileGitHubUtil.createPullReq(new GHRepository(), "branch", forkRepo, pullRequestInfo);
         verify(gitHubUtil, times(1)).createPullReq(any(), anyString(), any(), anyString(), eq(Constants.PULL_REQ_ID));
         verify(forkRepo, times(1)).delete();
     }

Original file line number	Diff line number	Diff line change
`@@ -14,6 +14,7 @@`
`14`	`14`	`import com.google.gson.JsonParseException;`
`15`	`15`	`import com.google.gson.JsonParser;`
`16`	`16`	`import com.salesforce.dockerfileimageupdate.SubCommand;`
	`17`	`+import com.salesforce.dockerfileimageupdate.model.PullRequestInfo;`
`17`	`18`	`import com.salesforce.dockerfileimageupdate.repository.GitHub;`
`18`	`19`	`import com.salesforce.dockerfileimageupdate.subcommands.ExecutableWithNamespace;`
`19`	`20`	`import com.salesforce.dockerfileimageupdate.utils.Constants;`
`@@ -240,7 +241,11 @@ protected void changeDockerfiles(Namespace ns,`
`240`	`241`	`}`
`241`	`242`
`242`	`243`	`if (isContentModified) {`
`243`		`- dockerfileGitHubUtil.createPullReq(parent, branch, forkedRepo, ns.get(Constants.GIT_PR_TITLE));`
	`244`	`+ // Passing null for image/tag to temporarily maintain old behavior of using constant.`
	`245`	`+ PullRequestInfo pullRequestInfo =`
	`246`	`+ new PullRequestInfo(ns.get(Constants.GIT_PR_TITLE),null, null);`
	`247`	`+`
	`248`	`+ dockerfileGitHubUtil.createPullReq(parent, branch, forkedRepo, pullRequestInfo);`
`244`	`249`	`}`
`245`	`250`	`}`
`246`	`251`
Original file line number	Diff line number	Diff line change
`@@ -69,7 +69,7 @@ public void testChangeDockerfiles_pullRequestCreation() throws Exception {`
`69`	`69`	`Mockito.verify(dockerfileGitHubUtil, times(1))`
`70`	`70`	`.modifyOnGithub(eq(content), eq("image-tag"), eq("image"), eq("tag"), anyString());`
`71`	`71`	`Mockito.verify(dockerfileGitHubUtil, times(1))`
`72`		`- .createPullReq(eq(parentRepo), eq("image-tag"), eq(forkedRepo), anyString());`
	`72`	`+ .createPullReq(eq(parentRepo), eq("image-tag"), eq(forkedRepo), any());`
`73`	`73`	`}`
`74`	`74`
`75`	`75`	`@Test`
`@@ -120,7 +120,7 @@ public void testOnePullRequestForMultipleDockerfilesInSameRepo() throws Exceptio`
`120`	`120`
`121`	`121`	`// Only one PR created on the repo with changes to both Dockerfiles.`
`122`	`122`	`Mockito.verify(dockerfileGitHubUtil, times(1)).createPullReq(eq(parentRepo),`
`123`		`- eq("image-tag"), eq(forkedRepo), anyString());`
	`123`	`+ eq("image-tag"), eq(forkedRepo), any());`
`124`	`124`	`}`
`125`	`125`
`126`	`126`	`@Test`
`@@ -160,6 +160,6 @@ public void testNoPullRequestForMissingDockerfile() throws Exception {`
`160`	`160`	`Mockito.verify(dockerfileGitHubUtil, times(0))`
`161`	`161`	`.modifyOnGithub(any(), anyString(), anyString(), anyString(), anyString());`
`162`	`162`	`Mockito.verify(dockerfileGitHubUtil, times(0)).createPullReq(eq(parentRepo),`
`163`		`- anyString(), eq(forkedRepo), anyString());`
	`163`	`+ anyString(), eq(forkedRepo), any());`
`164`	`164`	`}`
`165`	`165`	`}`