Merge pull request #761 from danger/logs_ci

Improve logs and verifies --process on the CI
danger · Nov 12, 2018 · 3fa50c8 · 3fa50c8
2 parents b138a3d + 2063cd9
commit 3fa50c8
Show file tree

Hide file tree

Showing 17 changed files with 113 additions and 37 deletions.
diff --git a/.travis.yml b/.travis.yml
@@ -29,6 +29,14 @@ matrix:
         - yarn build
         - node scripts/run-fixtures.js
 
+    # Runs both the CI and PR with a custom process to ensure apps like swift/rust work
+    - node_js: "10.13"
+      script:
+        - yarn build
+        - node distribution/commands/danger-ci.js --process "ruby scripts/danger_runner.rb"
+        - node distribution/commands/danger-pr.js --process "ruby scripts/danger_runner.rb"
+          https://github.com/danger/danger-js/pull/760
+
     # Does the real danger run
     - node_js: "9"
       script:

diff --git a/.vscode/settings.json b/.vscode/settings.json
@@ -12,5 +12,6 @@
     "distribution/": true
   },
   "editor.formatOnSave": true,
+  "debug.node.autoAttach": "on",
   "cSpell.words": ["APIPR", "BITBUCKETSERVER", "Commenter", "JSONDSL", "PRDSL", "bitbucket", "jsome", "repo's"]
 }
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -13,6 +13,9 @@
 
 ## Master
 
+- Improves debug logging, messaging on CI about status updates - [@orta][]
+- Better detection of json results: {} from a sub-process - [@orta][]
+- CLI Args are actually passed to subprocess instead of an empty object - [@orta][]
 - Fix Netlify integration when repo url includes an access token - [@imorente][]
 
 # 6.1.0

diff --git a/scripts/danger_runner.rb b/scripts/danger_runner.rb
@@ -5,6 +5,7 @@
 
 # Have a dumb fake response
 require "json"
+puts "Hello from ruby!"
 results = { fails: [], warnings: [], messages: [], markdowns: [] }.to_json
 
 STDOUT.write(results)
diff --git a/source/commands/ci/runner.ts b/source/commands/ci/runner.ts
@@ -57,7 +57,7 @@ export const runRunner = async (app: SharedCLI, config?: Partial<RunnerConfig>)
     }
 
     if (platform) {
-      const dangerJSONDSL = await jsonDSLGenerator(platform, source)
+      const dangerJSONDSL = await jsonDSLGenerator(platform, source, app)
       const execConfig: ExecutorOptions = {
         stdoutOnly: !platform.supportsCommenting() || app.textOnly,
         verbose: app.verbose,

diff --git a/source/commands/danger-pr.ts b/source/commands/danger-pr.ts
@@ -104,7 +104,7 @@ if (program.args.length === 0) {
 
 // Run the first part of a Danger Process and output the JSON to CLI
 async function runHalfProcessJSON(platform: Platform, source: CISource) {
-  const dangerDSL = await jsonDSLGenerator(platform, source)
+  const dangerDSL = await jsonDSLGenerator(platform, source, program)
   const processInput = prepareDangerDSL(dangerDSL)
   const output = JSON.parse(processInput)
   const dsl = { danger: output }

diff --git a/source/commands/danger-process.ts b/source/commands/danger-process.ts
@@ -25,11 +25,13 @@ const d = debug("process")
 declare const global: any
 
 let subprocessName: string | undefined
+let deprecationWarning = chalk.bold("Semi-deprecated, as ci/pr/local all support --process")
 
 program
   .usage("[options] <process_name>")
   .description(
-    "Does a Danger run, but instead of handling the execution of a Dangerfile it will pass the DSL " +
+    deprecationWarning +
+      "\n\nDoes a Danger run, but instead of handling the execution of a Dangerfile it will pass the DSL " +
       "into another process expecting the process to eventually return results back as JSON. If you don't " +
       "provide another process, then it will output to STDOUT."
   )
@@ -70,7 +72,7 @@ getRuntimeCISource(app).then(source => {
     }
 
     if (platform) {
-      jsonDSLGenerator(platform, source).then(dangerJSONDSL => {
+      jsonDSLGenerator(platform, source, app).then(dangerJSONDSL => {
         if (!subprocessName) {
           //  Just pipe it out to the CLI
           const processInput = prepareDangerDSL(dangerJSONDSL)

diff --git a/source/commands/utils/runDangerSubprocess.ts b/source/commands/utils/runDangerSubprocess.ts
@@ -37,11 +37,13 @@ export const runDangerSubprocess = (
   const dslJSONString = prepareDangerDSL(dslJSON)
   d(`Running subprocess: ${processDisplayName} - ${args}`)
   const child = spawn(processName, args, { env: { ...process.env, ...config.additionalEnvVars } })
-  let allLogs = ""
 
+  d(`Started passing in STDIN`)
   child.stdin.write(dslJSONString)
   child.stdin.end()
+  d(`Passed in STDIN`)
 
+  let allLogs = ""
   child.stdout.on("data", async data => {
     const stdout = data.toString()
     allLogs += stdout
@@ -50,15 +52,21 @@ export const runDangerSubprocess = (
     const maybeJSON = getJSONFromSTDOUT(stdout)
     const maybeJSONURL = getJSONURLFromSTDOUT(stdout)
 
+    // Remove message sent back to danger-js
+    const withoutURLs: string = data
+      .toString()
+      .replace(maybeJSON, "")
+      .replace(maybeJSONURL, "")
+
+    console.log(withoutURLs)
+
+    // Pass it back to the user
     if (!results && maybeJSONURL) {
       d("Got JSON URL from STDOUT, results are at: \n" + maybeJSONURL)
       results = JSON.parse(readFileSync(maybeJSONURL.replace("danger-results:/", ""), "utf8"))
     } else if (!results && maybeJSON) {
       d("Got JSON results from STDOUT, results: \n" + maybeJSON)
       results = JSON.parse(maybeJSON)
-    } else {
-      // Pass it back to the user
-      console.log(data.toString())
     }
   })
 
@@ -98,14 +106,17 @@ const getJSONURLFromSTDOUT = (stdout: string): string | undefined => {
 
 /** Pulls the JSON directly out, this has proven to be less reliable  */
 const getJSONFromSTDOUT = (stdout: string): string | undefined => {
-  const trimmed = stdout.trim()
-  return trimmed.startsWith("{") &&
-    trimmed.endsWith("}") &&
-    trimmed.includes("markdowns") &&
-    trimmed.includes("fails") &&
-    trimmed.includes("warnings")
-    ? trimmed
-    : undefined
+  const lines = stdout.split("\n")
+  return lines.find(line => {
+    const trimmed = line.trim()
+    return (
+      trimmed.startsWith("{") &&
+      trimmed.endsWith("}") &&
+      trimmed.includes("markdowns") &&
+      trimmed.includes("fails") &&
+      trimmed.includes("warnings")
+    )
+  })
 }
 
 export default runDangerSubprocess
diff --git a/source/danger-incoming-process-schema.json b/source/danger-incoming-process-schema.json
@@ -486,21 +486,27 @@
       "description": "Describes the possible arguments that\ncould be used when calling the CLI",
       "properties": {
         "base": {
+          "description": "The base reference used by danger PR (e.g. not master)",
           "type": "string"
         },
         "dangerfile": {
+          "description": "A custom path for the dangerfile (can also be a remote reference)",
           "type": "string"
         },
         "externalCiProvider": {
+          "description": "Used by danger-js o allow having a custom CI",
           "type": "string"
         },
         "id": {
+          "description": "So you can have many danger runs in one code review",
           "type": "string"
         },
         "textOnly": {
+          "description": "Use SDTOUT instead of posting to the code review side",
           "type": "string"
         },
         "verbose": {
+          "description": "For debugging",
           "type": "string"
         }
       },

diff --git a/source/danger.d.ts b/source/danger.d.ts
@@ -1108,13 +1108,21 @@ interface Violation {
  * could be used when calling the CLI
  */
 interface CliArgs {
+  /** The base reference used by danger PR (e.g. not master) */
   base: string
+  /** For debugging  */
   verbose: string
+  /** Used by danger-js o allow having a custom CI */
   externalCiProvider: string
+  /** Use SDTOUT instead of posting to the code review side */
   textOnly: string
+  /** A custom path for the dangerfile (can also be a remote reference) */
   dangerfile: string
+  /** So you can have many danger runs in one code review */
   id: string
 }
+
+// NOTE: if add something new here, you need to change dslGenerator.ts
 /** A function with a callback function, which Danger wraps in a Promise */
 type CallbackableFn = (callback: (done: any) => void) => void
 

diff --git a/source/dsl/cli-args.ts b/source/dsl/cli-args.ts
@@ -3,10 +3,18 @@
  * could be used when calling the CLI
  */
 export interface CliArgs {
+  /** The base reference used by danger PR (e.g. not master) */
   base: string
+  /** For debugging  */
   verbose: string
+  /** Used by danger-js o allow having a custom CI */
   externalCiProvider: string
+  /** Use SDTOUT instead of posting to the code review side */
   textOnly: string
+  /** A custom path for the dangerfile (can also be a remote reference) */
   dangerfile: string
+  /** So you can have many danger runs in one code review */
   id: string
 }
+
+// NOTE: if add something new here, you need to change dslGenerator.ts
diff --git a/source/platforms/github/GitHubAPI.ts b/source/platforms/github/GitHubAPI.ts
@@ -353,18 +353,28 @@ export class GitHubAPI {
     if (passed === "pending") {
       state = "pending"
     }
-    const res = await this.post(
-      `repos/${repo}/statuses/${ref}`,
-      {},
-      {
-        state: state,
-        context: process.env["PERIL_BOT_USER_ID"] ? "Peril" : "Danger",
-        target_url: url || "http://danger.systems/js",
-        description: message,
+    // Suppress errors, because in OSS
+    // this failure could be due to access rights.
+    //
+    // So only error when it's a real message.
+    try {
+      const res = await this.post(
+        `repos/${repo}/statuses/${ref}`,
+        {},
+        {
+          state: state,
+          context: process.env["PERIL_BOT_USER_ID"] ? "Peril" : "Danger",
+          target_url: url || "http://danger.systems/js",
+          description: message,
+        },
+        true
+      )
+      return res.ok
+    } catch (error) {
+      if (prJSON.base.repo.private) {
+        console.log("Could not post a commit status.")
       }
-    )
-
-    return res.ok
+    }
   }
 
   postCheckRun = async (check: CheckOptions, token: string) => {

diff --git a/source/platforms/github/_tests/_github_api.test.ts b/source/platforms/github/_tests/_github_api.test.ts
@@ -162,7 +162,7 @@ describe("API testing", () => {
         method: "POST",
         body: '{"state":"pending","context":"Danger","target_url":"http://danger.systems/js","description":"message"}',
       },
-      undefined
+      true
     )
   })
 
@@ -181,7 +181,7 @@ describe("API testing", () => {
         method: "POST",
         body: '{"state":"failure","context":"Danger","target_url":"http://danger.systems/js","description":"message"}',
       },
-      undefined
+      true
     )
   })
 
@@ -200,7 +200,7 @@ describe("API testing", () => {
         method: "POST",
         body: '{"state":"success","context":"Danger","target_url":"http://example.org","description":"message"}',
       },
-      undefined
+      true
     )
   })
 
@@ -219,7 +219,7 @@ describe("API testing", () => {
         method: "POST",
         body: '{"state":"success","context":"Danger","target_url":"http://danger.systems/js","description":"message"}',
       },
-      undefined
+      true
     )
   })
 

diff --git a/source/runner/_tests/_executor.test.ts b/source/runner/_tests/_executor.test.ts
@@ -29,7 +29,7 @@ const defaultConfig = {
 const fakeCI = new FakeCI({})
 
 const defaultDsl = (platform: any): Promise<DangerDSLType> => {
-  return jsonDSLGenerator(platform, fakeCI).then(jsonDSL => {
+  return jsonDSLGenerator(platform, fakeCI, {} as any).then(jsonDSL => {
     jsonDSL.github = {
       pr: {
         number: 1,

diff --git a/source/runner/dslGenerator.ts b/source/runner/dslGenerator.ts
@@ -3,8 +3,13 @@ import { DangerDSLJSONType } from "../dsl/DangerDSL"
 import { CliArgs } from "../dsl/cli-args"
 import { CISource } from "../ci_source/ci_source"
 import { emptyGitJSON } from "../platforms/github/GitHubGit"
+import { CommanderStatic } from "commander"
 
-export const jsonDSLGenerator = async (platform: Platform, source: CISource): Promise<DangerDSLJSONType> => {
+export const jsonDSLGenerator = async (
+  platform: Platform,
+  source: CISource,
+  program: CommanderStatic
+): Promise<DangerDSLJSONType> => {
   const useSimpleDSL = platform.getPlatformReviewSimpleRepresentation && source.useEventDSL
 
   const git = useSimpleDSL ? emptyGitJSON() : await platform.getPlatformGitRepresentation()
@@ -15,6 +20,14 @@ export const jsonDSLGenerator = async (platform: Platform, source: CISource): Pr
 
   const platformDSL = await getDSLFunc!()
 
+  const cliArgs: CliArgs = {
+    base: program.base,
+    dangerfile: program.dangerfile,
+    externalCiProvider: program.externalCiProvider,
+    id: program.id,
+    textOnly: program.textOnly,
+    verbose: program.verbose,
+  }
   return {
     git,
     [platform.name === "BitBucketServer" ? "bitbucket_server" : "github"]: platformDSL,
@@ -24,7 +37,7 @@ export const jsonDSLGenerator = async (platform: Platform, source: CISource): Pr
         additionalHeaders: {},
         baseURL: process.env["DANGER_GITHUB_API_BASE_URL"] || undefined,
       },
-      cliArgs: {} as CliArgs,
+      cliArgs,
     },
   }
 }
diff --git a/source/runner/jsonToContext.ts b/source/runner/jsonToContext.ts
@@ -3,6 +3,7 @@ import { jsonToDSL } from "./jsonToDSL"
 import { contextForDanger, DangerContext } from "./Dangerfile"
 import { DangerDSLJSON } from "./dangerDSLJSON"
 import { CISource } from "../ci_source/ci_source"
+import { CommanderStatic } from "commander"
 
 /**
  * Reads in the JSON string converts to a dsl object and gets the change context
@@ -11,8 +12,12 @@ import { CISource } from "../ci_source/ci_source"
  * @param program {any} commander
  * @returns {Promise<DangerContext>} context for danger
  */
-export async function jsonToContext(JSONString: string, program: any, source: CISource): Promise<DangerContext> {
-  const dslJSON = { danger: new DangerDSLJSON(JSONString, program as CliArgs) }
+export async function jsonToContext(
+  JSONString: string,
+  program: CommanderStatic,
+  source: CISource
+): Promise<DangerContext> {
+  const dslJSON = { danger: new DangerDSLJSON(JSONString, (program as any) as CliArgs) }
   const dsl = await jsonToDSL(dslJSON.danger, source)
   return contextForDanger(dsl)
 }
diff --git a/source/runner/runners/_tests/vm2.test.ts b/source/runner/runners/_tests/vm2.test.ts
@@ -48,7 +48,7 @@ runners.forEach(run => {
       const source = new FakeCI({})
       exec = new Executor(source, platform, run.fn, config)
 
-      const dsl = await jsonDSLGenerator(platform, new FakeCI({}))
+      const dsl = await jsonDSLGenerator(platform, new FakeCI({}), {} as any)
       dsl.github = {
         pr: {
           number: 1,