fix: review comments (TBS)

docs/reference/commands.md

@@ -826,21 +826,13 @@ Examples:
 
 Access tools included by providers.
 
-Run a tool defined by a provider in your project, downloading and extracting it if
-necessary. Run without arguments to get a list of all tools available.
+Run a tool defined by a provider in your project, downloading and extracting it if necessary. Run without arguments to get a list of all tools available.
 
-Run with the --get-path flag to just print the path to the binary or library
-directory (depending on the tool type). If the tool is a non-executable library, this
-flag is implicit.
+Run with the --get-path flag to just print the path to the binary or library directory (depending on the tool type). If the tool is a non-executable library, this flag is implicit.
 
-When multiple plugins provide a tool with the same name, you can choose a specific
-plugin/version by specifying <plugin name>.<tool name>, instead of just <tool name>.
-This is generally advisable when using this command in scripts, to avoid accidental
-conflicts.
+When multiple plugins provide a tool with the same name, you can choose a specific plugin/version by specifying <plugin name>.<tool name>, instead of just <tool name>. This is generally advisable when using this command in scripts, to avoid accidental conflicts.
 
-When there are name conflicts and a plugin name is not specified, the preference is
-for defined by configured providers in the current project (if applicable), and then
-alphabetical by plugin name.
+When there are name conflicts and a plugin name is not specified, we first prefer tools defined by configured providers in the current project (if applicable), and then alphabetical by plugin name.
 
 Examples:
 

garden-service/src/analytics/analytics.ts

@@ -19,6 +19,7 @@ import { Garden } from "../garden"
 import { AnalyticsType } from "./analytics-types"
 import dedent from "dedent"
 import { getGitHubUrl } from "../docs/common"
+import { InternalError } from "../exceptions"
 
 const API_KEY = process.env.ANALYTICS_DEV ? SEGMENT_DEV_API_KEY : SEGMENT_PROD_API_KEY
 
@@ -125,12 +126,15 @@ export class AnalyticsHandler {
   private ciName = ci.name
   private systemConfig: SystemInfo
   private isCI = ci.isCI
-  private sessionId: string | null
+  private sessionId: string
   private pendingEvents: Map<string, SegmentEvent>
   protected garden: Garden
   private projectMetadata: ProjectMetadata
 
   private constructor(garden: Garden, log: LogEntry) {
+    if (!garden.sessionId) {
+      throw new InternalError(`Garden instance with null sessionId passed to AnalyticsHandler constructor.`, {})
+    }
     this.segment = new segmentClient(API_KEY, { flushAt: 20, flushInterval: 300 })
     this.log = log
     this.garden = garden
@@ -243,7 +247,7 @@ export class AnalyticsHandler {
    * Used internally to check if a users has opted-in or not.
    */
   private analyticsEnabled(): boolean {
-    if (!this.sessionId || process.env.GARDEN_DISABLE_ANALYTICS === "true") {
+    if (process.env.GARDEN_DISABLE_ANALYTICS === "true") {
       return false
     }
     return this.analyticsConfig.optedIn || false
@@ -278,7 +282,7 @@ export class AnalyticsHandler {
       ciName: this.ciName,
       system: this.systemConfig,
       isCI: this.isCI,
-      sessionId: this.sessionId!,
+      sessionId: this.sessionId,
       projectMetadata: this.projectMetadata,
     }
   }

garden-service/src/commands/tools.ts

@@ -42,25 +42,16 @@ export class ToolsCommand extends Command<Args, Opts> {
   help = "Access tools included by providers."
   cliOnly = true
 
-  // FIXME: We need this while we're still resolving providers in the AnalyticsHandler
   noProject = true
 
   description = dedent`
-    Run a tool defined by a provider in your project, downloading and extracting it if
-    necessary. Run without arguments to get a list of all tools available.
+    Run a tool defined by a provider in your project, downloading and extracting it if necessary. Run without arguments to get a list of all tools available.
 
-    Run with the --get-path flag to just print the path to the binary or library
-    directory (depending on the tool type). If the tool is a non-executable library, this
-    flag is implicit.
+    Run with the --get-path flag to just print the path to the binary or library directory (depending on the tool type). If the tool is a non-executable library, this flag is implicit.
 
-    When multiple plugins provide a tool with the same name, you can choose a specific
-    plugin/version by specifying <plugin name>.<tool name>, instead of just <tool name>.
-    This is generally advisable when using this command in scripts, to avoid accidental
-    conflicts.
+    When multiple plugins provide a tool with the same name, you can choose a specific plugin/version by specifying <plugin name>.<tool name>, instead of just <tool name>. This is generally advisable when using this command in scripts, to avoid accidental conflicts.
 
-    When there are name conflicts and a plugin name is not specified, the preference is
-    for defined by configured providers in the current project (if applicable), and then
-    alphabetical by plugin name.
+    When there are name conflicts and a plugin name is not specified, we first prefer tools defined by configured providers in the current project (if applicable), and then alphabetical by plugin name.
 
     Examples:
 
@@ -128,6 +119,7 @@ export class ToolsCommand extends Command<Args, Opts> {
       const projectRoot = await findProjectConfig(garden.projectRoot)
 
       if (projectRoot) {
+        // This will normally be the case, but we're checking explictly to accommodate testing
         if (garden instanceof DummyGarden) {
           garden = await Garden.factory(garden.projectRoot, { ...omit(garden.opts, "config"), log })
         }

garden-service/src/plugins/container/helpers.ts

@@ -295,7 +295,7 @@ const helpers = {
         ignoreError,
         log,
         stdout: outputStream,
-        timeout,
+        timeoutSec: timeout,
       })
       return res
     } catch (err) {

garden-service/src/plugins/kubernetes/commands/cleanup-cluster-registry.ts

@@ -459,6 +459,6 @@ async function execInBuildSync({ provider, log, args, timeout, podName }: Builde
     args: execCmd,
     log,
     namespace: systemNamespace,
-    timeout,
+    timeoutSec: timeout,
   })
 }

garden-service/src/plugins/kubernetes/container/build.ts

@@ -422,7 +422,7 @@ export async function execInPod({
     ignoreError,
     log,
     namespace: systemNamespace,
-    timeout,
+    timeoutSec: timeout,
     stdout,
     stderr,
   })

garden-service/src/plugins/kubernetes/container/exec.ts

@@ -85,7 +85,7 @@ export async function execInWorkload({
     namespace,
     args: kubecmd,
     ignoreError: true,
-    timeout: 999999,
+    timeoutSec: 999999,
     tty: interactive,
   })
 

garden-service/src/plugins/kubernetes/helm/helm-cli.ts

@@ -174,6 +174,6 @@ export async function helm({
     args: [...opts, ...args],
     env: envVars,
     // Helm itself will time out pretty reliably, so we shouldn't time out early on our side.
-    timeout: 3600,
+    timeoutSec: 3600,
   })
 }

garden-service/src/plugins/kubernetes/run.ts

@@ -452,7 +452,7 @@ export class PodRunner extends PodRunnerParams {
       ignoreError,
       args: kubecmd,
       stdout,
-      timeout,
+      timeoutSec: timeout,
       tty: interactive,
     })
 
@@ -561,7 +561,7 @@ export class PodRunner extends PodRunnerParams {
       log,
       stdout,
       stderr,
-      timeout,
+      timeoutSec: timeout,
     })
 
     let result: string = ""
@@ -624,7 +624,7 @@ export class PodRunner extends PodRunnerParams {
       log,
       stdout,
       stderr,
-      timeout,
+      timeoutSec: timeout,
     })
 
     if (res.timedOut) {

garden-service/src/plugins/terraform/commands.ts

@@ -62,7 +62,7 @@ function makeRootCommand(commandName: string) {
         cwd: root,
         rawMode: false,
         tty: true,
-        timeout: 999999,
+        timeoutSec: 999999,
       })
 
       return { result: {} }
@@ -96,7 +96,7 @@ function makeModuleCommand(commandName: string) {
         cwd: root,
         rawMode: false,
         tty: true,
-        timeout: 999999,
+        timeoutSec: 999999,
       })
 
       return { result: {} }

garden-service/src/plugins/terraform/common.ts

@@ -45,7 +45,7 @@ export async function tfValidate(log: LogEntry, provider: TerraformProvider, roo
     if (reasons.includes("Could not satisfy plugin requirements") || reasons.includes("Module not installed")) {
       // We need to run `terraform init` and retry validation
       log.debug("Initializing Terraform")
-      await terraform(provider).exec({ log, args: ["init"], cwd: root, timeout: 300 })
+      await terraform(provider).exec({ log, args: ["init"], cwd: root, timeoutSec: 300 })
 
       const retryRes = await terraform(provider).json({
         log,

garden-service/src/util/ext-tools.ts

@@ -35,7 +35,7 @@ export interface ExecParams {
   cwd?: string
   env?: { [key: string]: string }
   log: LogEntry
-  timeout?: number
+  timeoutSec?: number
   input?: Buffer | string
   ignoreError?: boolean
   stdout?: Writable
@@ -118,7 +118,7 @@ export class PluginTool {
     return path
   }
 
-  async exec({ args, cwd, env, log, timeout, input, ignoreError, stdout, stderr }: ExecParams) {
+  async exec({ args, cwd, env, log, timeoutSec, input, ignoreError, stdout, stderr }: ExecParams) {
     const path = await this.getPath(log)
 
     if (!args) {
@@ -132,7 +132,7 @@ export class PluginTool {
 
     return exec(path, args, {
       cwd,
-      timeout: timeout ? timeout * 1000 : undefined,
+      timeout: timeoutSec ? timeoutSec * 1000 : undefined,
       env,
       input,
       reject: !ignoreError,
@@ -173,7 +173,7 @@ export class PluginTool {
     return crossSpawn(path, args, { cwd, env })
   }
 
-  async spawnAndWait({ args, cwd, env, log, ignoreError, rawMode, stdout, stderr, timeout, tty }: SpawnParams) {
+  async spawnAndWait({ args, cwd, env, log, ignoreError, rawMode, stdout, stderr, timeoutSec, tty }: SpawnParams) {
     const path = await this.getPath(log)
 
     if (!args) {
@@ -186,7 +186,7 @@ export class PluginTool {
     log.debug(`Spawning '${path} ${args.join(" ")}' in ${cwd}`)
     return spawn(path, args || [], {
       cwd,
-      timeout,
+      timeout: timeoutSec,
       ignoreError,
       env,
       rawMode,

garden-service/test/unit/src/commands/util.ts

@@ -13,7 +13,6 @@ import { expect } from "chai"
 import { DEFAULT_API_VERSION, GARDEN_GLOBAL_PATH } from "../../../../src/constants"
 import { createGardenPlugin } from "../../../../src/types/plugin/plugin"
 import { pick } from "lodash"
-import { homedir } from "os"
 import { join } from "path"
 
 describe("FetchToolsCommand", () => {