From a3bbb9f3664749a174abbecf92554d73d04dd707 Mon Sep 17 00:00:00 2001
From: Jeff Kreeftmeijer <jeffkreeftmeijer@gmail.com>
Date: Mon, 28 Jun 2021 08:09:00 +0200
Subject: [PATCH] Return the full log path in the diagnose report (#402)

* Return the full log path in the diagnose report

Currently, the diagnose command expects the logPath to include the
filename for the `appsignal.log` file. To get the `log_dir_path`, the
filename is stripped off. The `LogPath` is then used to return the full
path to the file.

In reality, it should be the other way around. The `LogPath` does not
include the filename, so it can be used as the log_dir_path in the
diagnose. Then, to get the full path to the `appsignal.log` file, this
patch appends the filename to the log path.

* Use internal value for logFilePath in diagnose

Co-authored-by: Tom de Bruijn <tom@tomdebruijn.com>

* Explicitly cast logFilePath to <string>

* Diagnose strips "/appsignal.log" from directory path, if present

Co-authored-by: Tom de Bruijn <tom@tomdebruijn.com>
---
 .../.changesets/fix-diagnose-log-paths.md     |  6 +++++
 .../nodejs/src/__tests__/diagnose.test.ts     | 27 +++++++++++++++++++
 packages/nodejs/src/diagnose.ts               |  8 +++---
 3 files changed, 37 insertions(+), 4 deletions(-)
 create mode 100644 packages/nodejs/.changesets/fix-diagnose-log-paths.md

diff --git a/packages/nodejs/.changesets/fix-diagnose-log-paths.md b/packages/nodejs/.changesets/fix-diagnose-log-paths.md
new file mode 100644
index 00000000..fa8fed98
--- /dev/null
+++ b/packages/nodejs/.changesets/fix-diagnose-log-paths.md
@@ -0,0 +1,6 @@
+---
+bump: "patch"
+---
+
+Fix the log path reported when running the diagnose command to include the
+filename.
diff --git a/packages/nodejs/src/__tests__/diagnose.test.ts b/packages/nodejs/src/__tests__/diagnose.test.ts
index 52ccbd9c..f8057df9 100644
--- a/packages/nodejs/src/__tests__/diagnose.test.ts
+++ b/packages/nodejs/src/__tests__/diagnose.test.ts
@@ -26,4 +26,31 @@ describe("DiagnoseTool", () => {
 
     expect(output.process.uid).toEqual(process.getuid())
   })
+
+  it("returns the log_dir_path", () => {
+    expect(tool.generate().paths.log_dir_path.path).toEqual("/tmp")
+  })
+
+  it("returns the appsignal.log path", () => {
+    expect(tool.generate().paths["appsignal.log"].path).toEqual(
+      "/tmp/appsignal.log"
+    )
+  })
+
+  describe("when to log path is configured as a full path", () => {
+    beforeEach(() => {
+      process.env["APPSIGNAL_LOG_PATH"] = "/path/to/appsignal.log"
+      tool = new DiagnoseTool({})
+    })
+
+    it("returns the log_dir_path", () => {
+      expect(tool.generate().paths.log_dir_path.path).toEqual("/path/to")
+    })
+
+    it("returns the appsignal.log path", () => {
+      expect(tool.generate().paths["appsignal.log"].path).toEqual(
+        "/path/to/appsignal.log"
+      )
+    })
+  })
 })
diff --git a/packages/nodejs/src/diagnose.ts b/packages/nodejs/src/diagnose.ts
index 3a4879f6..35087793 100644
--- a/packages/nodejs/src/diagnose.ts
+++ b/packages/nodejs/src/diagnose.ts
@@ -93,7 +93,7 @@ export class DiagnoseTool {
 
     // we want to fall over if this value isn't present
     // (it should be)
-    const logPath = this.#config.data.logPath!
+    const logFilePath = <string>this.#config.data.logFilePath!
 
     // add any paths we want to check to this object!
     const files = {
@@ -101,11 +101,11 @@ export class DiagnoseTool {
         path: process.cwd()
       },
       log_dir_path: {
-        path: logPath.replace("/appsignal.log", "")
+        path: this.#config.data.logPath!.replace("/appsignal.log", "")
       },
       "appsignal.log": {
-        path: logPath,
-        content: safeReadFromPath(logPath).split("\n")
+        path: logFilePath,
+        content: safeReadFromPath(logFilePath).split("\n")
       }
     }