misc fixes

src/extension.ts

@@ -201,7 +201,7 @@ async function updateLanguageServer(extVersion: string, lsPath: ServerPath, sche
   }
 
   if (lsPath.hasCustomBinPath()) {
-    // skip install if a language server binary path is set
+    // skip install check if user has specified a custom path to the LS
     return;
   }
 
@@ -214,9 +214,14 @@ async function updateLanguageServer(extVersion: string, lsPath: ServerPath, sche
       reporter,
     );
 
-    if (scheduled === false) {
-      await clientHandler.startClient();
+    // On scheduled checks, we download to stg and do not replace prod path
+    // So we *do not* need to stop or start the LS
+    if (scheduled) {
+      return;
     }
+
+    // On fresh starts we *need* to start the lang client always
+    await clientHandler.startClient();
   } catch (error) {
     console.log(error); // for test failure reporting
     if (error instanceof Error) {

src/installer/detector.ts

@@ -16,6 +16,8 @@ export async function getLsVersion(binPath: string): Promise<string | undefined>
     return jsonOutput.version;
   } catch (err) {
     // assume older version of LS which didn't have json flag
+    // return undefined as regex matching isn't useful here
+    // if it's old enough to not have the json version, we would be updating anyway
     return undefined;
   }
 }
@@ -27,6 +29,7 @@ export async function getRequiredVersionRelease(
 ): Promise<Release> {
   const userAgent = `Terraform-VSCode/${extensionVersion} VSCode/${vscodeVersion}`;
 
+  // Take the user requested version and query the hashicorp release site
   try {
     const release = await getRelease('terraform-ls', versionString, userAgent);
     console.log(`Found Terraform language server version ${release.version} which satisfies range '${versionString}'`);
@@ -35,6 +38,7 @@ export async function getRequiredVersionRelease(
     if (versionString == DEFAULT_LS_VERSION) {
       throw err;
     }
+
     console.log(
       `Error while finding Terraform language server release which satisfies range '${versionString}' and will reattempt with '${DEFAULT_LS_VERSION}': ${err}`,
     );
@@ -43,7 +47,8 @@ export async function getRequiredVersionRelease(
     );
   }
 
-  // Attempt to find the latest release
+  // User supplied version is either invalid or a version could not satisfy the range requested
+  // Attempt to find the latest release, as we need a LS to function
   const release = await getRelease('terraform-ls', DEFAULT_LS_VERSION, userAgent);
   console.log(
     `Found Default Terraform language server version ${release.version} which satisfies range '${DEFAULT_LS_VERSION}'`,

src/installer/installer.ts

@@ -11,23 +11,27 @@ export async function installTerraformLS(
   vscodeVersion: string,
   reporter: TelemetryReporter,
 ): Promise<void> {
-  const userAgent = `Terraform-VSCode/${extensionVersion} VSCode/${vscodeVersion}`;
-
   reporter.sendTelemetryEvent('installingLs', { terraformLsVersion: release.version });
 
   const zipfile = path.resolve(installPath, `terraform-ls_v${release.version}.zip`);
-
+  const userAgent = `Terraform-VSCode/${extensionVersion} VSCode/${vscodeVersion}`;
   const os = getPlatform();
   const arch = getArch();
   const build = release.getBuild(os, arch);
+
   if (!build) {
     throw new Error(`Install error: no matching terraform-ls binary for ${os}/${arch}`);
   }
 
+  // On brand new extension installs, there isn't a directory until we execute here
+  // Create it if it doesn't exist so the downloader can unpack
   if ((await pathExists(installPath)) === false) {
     await vscode.workspace.fs.createDirectory(vscode.Uri.file(installPath));
   }
 
+  // Download and unpack async inside the VS Code notification window
+  // This will show in the statusbar for the duration of the download and unpack
+  // This was the most non-distuptive choice that still provided some status to the user
   return vscode.window.withProgress(
     {
       cancellable: false,

src/installer/updater.ts

@@ -22,8 +22,9 @@ export async function updateOrInstall(
 ): Promise<void> {
   const stgingExists = await pathExists(lsPath.stgBinPath());
   if (stgingExists) {
-    // we updated during the last run while user was using editor
-    // need to move to prod path now
+    // LS was updated during the last run while user was using the extension
+    // Do not check for updates here, as normal execution flow will handle decision logic later
+    // Need to move stg path to prod path now and return normal execution
     await vscode.workspace.fs.rename(vscode.Uri.file(lsPath.stgBinPath()), vscode.Uri.file(lsPath.binPath()), {
       overwrite: true,
     });
@@ -37,10 +38,13 @@ export async function updateOrInstall(
   const lsPresent: boolean = await pathExists(lsPath.binPath());
   const autoUpdate = config('extensions').get<boolean>('autoUpdate', true);
   if (lsPresent === true && autoUpdate === false) {
-    // we have a LS, but user does not want automatic updates
+    // LS is present in prod path, but user does not want automatic updates
+    // Return normal execution
     return;
   }
 
+  // Get LS release information from hashicorp release site
+  // Fall back to latest if not requested version not available
   let release: Release;
   try {
     release = await getRequiredVersionRelease(versionString, extensionVersion, vscodeVersion);
@@ -56,28 +60,32 @@ export async function updateOrInstall(
   }
 
   if (lsPresent === false) {
-    // no ls present, need to download now
-    // install to production path
+    // LS is not present, need to download now in order to function
+    // Install directly to production path and return normal execution
     return installTerraformLS(lsPath.installPath(), release, extensionVersion, vscodeVersion, reporter);
   }
 
+  // We know there is an LS Present at this point, find out version if possible
   const installedVersion: string | undefined = await getLsVersion(lsPath.binPath());
   if (installedVersion === undefined) {
     console.log(`Currently installed Terraform language server is version '${installedVersion}`);
-    // ls is present but too old to tell us the version, need to update now
+    // ls is present but too old to tell us the version, so need to update now
     return installTerraformLS(lsPath.installPath(), release, extensionVersion, vscodeVersion, reporter);
   }
 
+  // We know there is an LS present and know the version, so decide whether to update or not
   console.log(`Currently installed Terraform language server is version '${installedVersion}`);
   reporter.sendTelemetryEvent('foundLsInstalled', { terraformLsVersion: installedVersion });
 
+  // Already at the latest or specified version, no update needed
+  // return to normal execution flow
   if (semver.eq(release.version, installedVersion, { includePrerelease: true })) {
-    // Already at the specified version, no update needed
     console.log(`Language server release is current: ${release.version}`);
     return;
   }
 
-  // update indicated and user wants autoupdates
+  // We used to prompt for decision here, but effectively downgrading or upgrading
+  // are the same operation so log decision and update
   if (semver.gt(release.version, installedVersion, { includePrerelease: true })) {
     // Upgrade
     console.log(`A newer language server release is available: ${release.version}`);
@@ -86,6 +94,6 @@ export async function updateOrInstall(
     console.log(`An older language server release is available: ${release.version}`);
   }
 
-  // update
+  // Update indicated and user wants autoupdates, so update to latest or specified version
   return installTerraformLS(lsPath.stgInstallPath(), release, extensionVersion, vscodeVersion, reporter);
 }