Address minor TODOs

packages/cli/changelog/@unreleased/pr-55.v2.yml

@@ -0,0 +1,5 @@
+type: improvement
+improvement:
+  description: Address minor issues
+  links:
+  - https://github.com/palantir/osdk-ts/pull/55

packages/cli/package.json

@@ -38,12 +38,14 @@
     "consola": "^3.2.3",
     "find-up": "^6.3.0",
     "open": "^9.1.0",
+    "semver": "^7.6.0",
     "yargs": "^17.7.2"
   },
   "devDependencies": {
     "@types/archiver": "^6.0.0",
     "@types/ngeohash": "^0.6.4",
     "@types/node": "^18.0.0",
+    "@types/semver": "^7.5.7",
     "@types/yargs": "^17.0.29",
     "ts-expect": "^1.3.0",
     "typescript": "^5.2.2"
@@ -54,7 +56,7 @@
   "imports": {
     "#net": "./src/net/index.mts"
   },
-  "keywords": [ ],
+  "keywords": [],
   "bin": {
     "osdk": "./bin/osdk.mjs"
   },

packages/cli/src/commands/site/version/list/versionListCommand.mts

@@ -55,7 +55,10 @@ export default async function versionListCommand(
   }
 
   consola.success("Found versions:");
-  for (const version of versions) {
+
+  const semver = await import("semver");
+  const sortedVersions = semver.rsort(versions.filter(v => semver.valid(v)));
+  for (const version of sortedVersions) {
     consola.log(
       `    - ${version}${
         deployedVersion

packages/cli/src/util/autoVersion.test.ts

@@ -14,70 +14,84 @@
  * limitations under the License.
  */
 
-import { execSync } from "node:child_process";
+import { exec } from "node:child_process";
+import { promisify } from "node:util";
 import { describe, expect, it, vi } from "vitest";
 import { autoVersion } from "./autoVersion.js";
 
 vi.mock("node:child_process");
+const execAsync = promisify(exec);
 
 describe("autoVersion", () => {
-  const execSyncMock = vi.mocked(execSync);
+  const execMock = vi.mocked(execAsync);
+  const execReturnValue = (out: string) => ({ stdout: out, stderr: "" });
 
   it("should return a valid SemVer version from git describe", async () => {
     const validGitVersion = "1.2.3";
-    execSyncMock.mockReturnValue(validGitVersion);
+    execMock.mockResolvedValue(execReturnValue(validGitVersion));
 
     const version = await autoVersion();
     expect(version).toBe("1.2.3");
-
-    expect(execSyncMock).toHaveBeenCalledWith(
-      "git describe --tags --first-parent --dirty",
-      { encoding: "utf8" },
-    );
   });
 
   it("should replace default prefix v from git describe output", async () => {
     const validGitVersion = "v1.2.3";
-    execSyncMock.mockReturnValue(validGitVersion);
-    const version = await autoVersion();
+    execMock.mockResolvedValue(execReturnValue(validGitVersion));
 
+    const version = await autoVersion();
     expect(version).toBe("1.2.3");
-    expect(execSyncMock).toHaveBeenCalledWith(
-      "git describe --tags --first-parent --dirty",
-      { encoding: "utf8" },
-    );
   });
 
   it("should replace the prefix from the found git tag", async () => {
     const validGitVersion = "@[email protected]";
-    execSyncMock.mockReturnValue(validGitVersion);
+    execMock.mockResolvedValue(execReturnValue(validGitVersion));
 
     const version = await autoVersion("@package@");
-
     expect(version).toBe("1.2.3");
-    expect(execSyncMock).toHaveBeenCalledWith(
-      "git describe --tags --first-parent --dirty --match=\"@package@*\"",
-      { encoding: "utf8" },
-    );
   });
 
   it("should only replace the prefix if found at the start of the tag only", async () => {
     const validGitVersion = "1.2.3-package";
-    execSyncMock.mockReturnValue(validGitVersion);
+    execMock.mockResolvedValue(execReturnValue(validGitVersion));
 
     const version = await autoVersion("-package");
-
     expect(version).toBe("1.2.3-package");
-    expect(execSyncMock).toHaveBeenCalledWith(
-      "git describe --tags --first-parent --dirty --match=\"-package*\"",
-      { encoding: "utf8" },
-    );
   });
 
   it("should throw an error if git describe returns a non-SemVer string", async () => {
     const nonSemVerGitVersion = "not-semver";
-    execSyncMock.mockReturnValue(nonSemVerGitVersion);
+    execMock.mockResolvedValue(execReturnValue(nonSemVerGitVersion));
+
+    await expect(autoVersion()).rejects.toThrowError();
+  });
 
-    await expect(autoVersion()).rejects.toThrow();
+  it("should throw an error if git isn't found", async () => {
+    execMock.mockImplementation(() => {
+      throw new Error("Command not found");
+    });
+
+    await expect(autoVersion()).rejects.toThrowError(
+      "Unable to determine the version using git-describe as git is not installed",
+    );
+  });
+
+  it("should throw an error if the current directory is not a git repository", async () => {
+    execMock.mockImplementation(() => {
+      throw new Error("fatal: not a git repository");
+    });
+
+    await expect(autoVersion()).rejects.toThrowError(
+      "Unable to determine the version using git-describe as the current directory is not a git repository",
+    );
+  });
+
+  it("should throw an error if no git tags are found", async () => {
+    execMock.mockImplementation(() => {
+      throw new Error("fatal: no names found, cannot describe anything.");
+    });
+
+    await expect(autoVersion()).rejects.toThrowError(
+      "Unable to determine the version using git-describe as no matching tags were found.",
+    );
   });
 });

packages/cli/src/util/autoVersion.ts

@@ -14,7 +14,8 @@
  * limitations under the License.
  */
 
-import { execSync } from "node:child_process";
+import { exec } from "node:child_process";
+import { promisify } from "node:util";
 import { ExitProcessError } from "../ExitProcessError.js";
 import { isValidSemver } from "./isValidSemver.js";
 
@@ -29,26 +30,71 @@ export async function autoVersion(tagPrefix: string = ""): Promise<string> {
   const [matchPrefix, prefixRegex] = tagPrefix !== ""
     ? [tagPrefix, new RegExp(`^${tagPrefix}`)]
     : [undefined, new RegExp(`^v?`)];
+
+  const gitVersion = await gitDescribe(matchPrefix);
+  const version = gitVersion.trim().replace(prefixRegex, "");
+  if (!isValidSemver(version)) {
+    throw new ExitProcessError(
+      2,
+      `The version string ${version} is not SemVer compliant.`,
+    );
+  }
+
+  return version;
+}
+
+async function gitDescribe(matchPrefix: string | undefined): Promise<string> {
+  let gitVersion;
   try {
-    const gitVersion = execSync(
+    const execAsync = promisify(exec);
+    const { stdout } = await execAsync(
       `git describe --tags --first-parent --dirty${
         matchPrefix != null ? ` --match="${matchPrefix}*"` : ""
       }`,
       { encoding: "utf8" },
     );
-    const version = gitVersion.trim().replace(prefixRegex, "");
-    if (!isValidSemver(version)) {
-      throw new ExitProcessError(
-        2,
-        `The version string ${version} is not SemVer compliant.`,
-      );
+    gitVersion = stdout;
+  } catch (error: any) {
+    if (error instanceof Error) {
+      const errorMessage: string = error.message.toLowerCase();
+
+      if (
+        errorMessage.includes("not recognized")
+        || errorMessage.includes("command not found")
+        || errorMessage.includes("no such file or directory")
+      ) {
+        throw new ExitProcessError(
+          2,
+          `Unable to determine the version using git-describe as git is not installed or found in the PATH.\nPlease correctly configure git or supply a --version option.`,
+        );
+      }
+
+      if (
+        errorMessage.includes("fatal: not a git repository")
+      ) {
+        throw new ExitProcessError(
+          2,
+          `Unable to determine the version using git-describe as the current directory is not a git repository.\nPlease run the command in a git respository or supply a --version option.`,
+        );
+      }
+
+      if (
+        errorMessage.includes(
+          "fatal: no names found, cannot describe anything.",
+        )
+      ) {
+        throw new ExitProcessError(
+          2,
+          `Unable to determine the version using git-describe as no matching tags were found.\nPlease tag a matching version or supply a --version option.`,
+        );
+      }
     }
 
-    return version;
-  } catch (error) {
     throw new ExitProcessError(
       2,
-      `Unable to determine the version automatically. Please supply a --version argument. ${error}`,
+      `Unable to determine the version automatically: ${error}.\nPlease supply a --version option.`,
     );
   }
+
+  return gitVersion;
 }

packages/cli/src/util/config.test.ts

@@ -16,14 +16,19 @@
 
 import { findUp } from "find-up";
 import { promises as fsPromises } from "node:fs";
-import { describe, expect, it, vi } from "vitest";
+import { extname } from "node:path";
+import { beforeEach, describe, expect, it, vi } from "vitest";
 import { loadFoundryConfig } from "./config.js";
 
 vi.mock("find-up");
 vi.mock("node:fs");
+vi.mock("node:path");
 
 describe("loadFoundryConfig", () => {
-  vi.mocked(findUp).mockResolvedValue("/path/foundry.config.json");
+  beforeEach(() => {
+    vi.mocked(findUp).mockResolvedValue("/path/foundry.config.json");
+    vi.mocked(extname).mockReturnValue(".json");
+  });
 
   it("should load and parse the configuration file correctly", async () => {
     const correctConfig = {
@@ -149,4 +154,11 @@ describe("loadFoundryConfig", () => {
     vi.mocked(findUp).mockResolvedValue(undefined);
     await expect(loadFoundryConfig()).resolves.toBeUndefined();
   });
+
+  it("should throw if config file extension isn't supported", async () => {
+    vi.mocked(extname).mockResolvedValue(".yaml");
+    await expect(loadFoundryConfig()).rejects.toThrow(
+      "Unsupported file extension:",
+    );
+  });
 });

packages/cli/src/util/config.ts

@@ -16,6 +16,7 @@
 
 import type { JSONSchemaType } from "ajv";
 import { promises as fsPromises } from "node:fs";
+import { extname } from "node:path";
 import { ExitProcessError } from "../ExitProcessError.js";
 
 export interface GitDescribeAutoVersionConfig {
@@ -94,11 +95,11 @@ export async function loadFoundryConfig(): Promise<
     let foundryConfig: FoundryConfig;
     try {
       const fileContent = await fsPromises.readFile(configFilePath, "utf-8");
-      foundryConfig = JSON.parse(fileContent);
-    } catch {
+      foundryConfig = parseConfigFile(fileContent, configFilePath);
+    } catch (error) {
       throw new ExitProcessError(
         2,
-        `Couldn't read or parse config file ${configFilePath}`,
+        `Couldn't read or parse config file ${configFilePath}. Error: ${error}`,
       );
     }
 
@@ -116,3 +117,19 @@ export async function loadFoundryConfig(): Promise<
 
   return undefined;
 }
+
+function parseConfigFile(
+  fileContent: string,
+  configFilePath: string,
+): FoundryConfig {
+  const extension = extname(configFilePath);
+  switch (extension) {
+    case ".json":
+      return JSON.parse(fileContent);
+    default:
+      throw new ExitProcessError(
+        2,
+        `Unsupported file extension: ${extension} for config file.`,
+      );
+  }
+}

packages/cli/src/util/isValidSemver.ts

@@ -14,10 +14,8 @@
  * limitations under the License.
  */
 
-// https://semver.org/#is-there-a-suggested-regular-expression-regex-to-check-a-semver-string
-const semver =
-  /^(0|[1-9]\d*)\.(0|[1-9]\d*)\.(0|[1-9]\d*)(?:-((?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*)(?:\.(?:0|[1-9]\d*|\d*[a-zA-Z-][0-9a-zA-Z-]*))*))?(?:\+([0-9a-zA-Z-]+(?:\.[0-9a-zA-Z-]+)*))?$/;
+import { valid } from "semver";
 
 export function isValidSemver(semverString: string): boolean {
-  return semver.test(semverString);
+  return valid(semverString) != null;
 }

packages/cli/src/util/token.ts

@@ -74,10 +74,10 @@ interface LoadedToken {
   token: string;
 }
 /**
- * Synchronously reads a JWT Auth Token from a file.
+ * Reads a JWT Auth Token from a file.
  * @param filePath The path to the token file.
  * @returns The token as a string.
- * @throws An error if the file cannot be read or if the file does not contain a valid JWT.
+ * @throws An error if the file cannot be read.
  */
 export async function loadTokenFile(filePath: string): Promise<LoadedToken> {
   let token: string;