Refactor settings.load into separate parts.

- getSettings: just return the current settings
- createSettings: create a new settings object,
                  based on defaults and any deployment profiles (and save it)
- load: Either call createSettings if there's no settings.json, otherwise load it.

Set-up code common to both `load` and `createSettings` has been pulled out into a
separate function `finishConfiguringSettings`, which does any sanity-checking and
merges in any locked-field settings.

The `getSettings` call is made to return the current settings from the main process,
which can't send a message requesting them. Now the only place `settings.load` is called is
during startup in `background.ts`.

And now the unit tests can simply call `createSettings` which will read in the current
deployment profile, no need to do an artificial `settings.clear()`.

Signed-off-by: Eric Promislow <epromislow@suse.com>

background.ts

@@ -200,7 +200,15 @@ Electron.app.whenReady().then(async() => {
 
       throw ex;
     }
-    cfg = settings.load(deploymentProfiles);
+    try {
+      cfg = settings.load(deploymentProfiles);
+      settings.updateLockedFields(deploymentProfiles.locked);
+    } catch (err: any) {
+      const titlePart = err.name || 'Failed to load settings';
+      const message = err.message || err.toString();
+
+      showErrorDialog(titlePart, message, true);
+    }
     try {
       // The profile loader did rudimentary type-validation on profiles, but the validator checks for things
       // like invalid strings for application.pathManagementStrategy.

pkg/rancher-desktop/config/__tests__/settings.spec.ts

@@ -295,7 +295,8 @@ describe('settings', () => {
         test('all fields are unlocked', async() => {
           const profiles = await readDeploymentProfiles();
 
-          settings.load(profiles);
+          settings.createSettings(profiles);
+          settings.updateLockedFields(profiles.locked);
           verifyAllFieldsAreUnlocked(settings.getLockedSettings());
         });
       });
@@ -324,7 +325,8 @@ describe('settings', () => {
                 .mockImplementation(createMocker(system, user));
               const profiles = await readDeploymentProfiles();
 
-              settings.load(profiles);
+              settings.createSettings(profiles);
+              settings.updateLockedFields(profiles.locked);
               if (shouldLock) {
                 verifyAllFieldsAreLocked(settings.getLockedSettings());
               } else {

pkg/rancher-desktop/config/settings.ts

@@ -96,7 +96,7 @@ export const defaultSettings = {
     name: ContainerEngine.MOBY,
   },
   virtualMachine: {
-    memoryInGB:   2,
+    memoryInGB:   getDefaultMemory(),
     numberCPUs:   2,
     /**
      * when set to true Dnsmasq is disabled and all DNS resolution
@@ -170,26 +170,24 @@ let settings: Settings | undefined;
  * Load the settings file from disk, doing any migrations as necessary.
  */
 function loadFromDisk(): Settings {
+  // Throw an ENOENT error if the file doesn't exist; the caller should know what to do.
   const rawdata = fs.readFileSync(join(paths.config, 'settings.json'));
-  let settings;
+  let cfg = clone(defaultSettings);
 
   try {
-    settings = JSON.parse(rawdata.toString());
-  } catch {
-    save(defaultSettings);
+    // If the existing settings file is partial, fill in the missing fields with defaults.
+    merge(cfg, JSON.parse(rawdata.toString()));
+  } catch (err: any) {
+    console.log(`Error JSON-parsing existing settings contents ${ rawdata }`, err);
+    console.log('The old settings file will be replaced with the default settings.');
 
-    return defaultSettings;
+    return cfg;
   }
-
-  // clone settings because we check to see if the returned value is different
-  const cfg = updateSettings(clone(settings));
+  cfg = updateSettings(cfg);
 
   if (!Object.values(ContainerEngine).map(String).includes(cfg.containerEngine.name)) {
     console.warn(`Replacing unrecognized saved container engine pref of '${ cfg.containerEngine.name }' with ${ ContainerEngine.CONTAINERD }`);
     cfg.containerEngine.name = ContainerEngine.CONTAINERD;
-    save(cfg);
-  } else if (!_.isEqual(cfg, settings)) {
-    save(cfg);
   }
 
   return cfg;
@@ -216,55 +214,53 @@ export function save(cfg: Settings) {
   }
 }
 
+export function getSettings(): Settings {
+  return settings ?? defaultSettings;
+}
+
 /**
- * Remove all stored settings.
+ * createSettings
+ * - Called when either there's no settings file, or for testing purposes, where we want to use a particular deployment profile.
+ * @param {DeploymentProfileType} deploymentProfiles
+ * @returns default settings merged with any default profile
  */
-export async function clear() {
-  // The node version packed with electron might not have fs.rm yet.
-  await fs.promises.rm(paths.config, { recursive: true, force: true } as any);
+export function createSettings(deploymentProfiles: DeploymentProfileType): Settings {
+  const cfg = clone(defaultSettings);
+
+  merge(cfg, deploymentProfiles.defaults);
+
+  // If there's no deployment profile, put up the first-run dialog box.
+  if (!Object.keys(deploymentProfiles.defaults).length && !Object.keys(deploymentProfiles.locked).length) {
+    _isFirstRun = true;
+  }
+
+  return finishConfiguringSettings(cfg, deploymentProfiles);
 }
 
 /**
- * Load the settings file or create it if not present.  If the settings have
- * already been loaded, return it without re-loading from disk.
+ * Load the settings file or create it if not present.
  */
 export function load(deploymentProfiles: DeploymentProfileType): Settings {
   try {
-    settings ??= loadFromDisk();
+    return finishConfiguringSettings(loadFromDisk(), deploymentProfiles);
   } catch (err: any) {
-    settings = clone(defaultSettings);
     if (err.code === 'ENOENT') {
-      // If a deployment profile doesn't set `virtualMachine.memoryInGB`, assign a default value (a few lines down)
-      // based on the available memory.
-      settings.virtualMachine.memoryInGB = 0;
-
-      // If there is no settings file, use the contents of the selected defaults deployment profile.
-      // Whether or not there's a settings file, give highest priority to any settings in the locked profile
-      // (which is merged outside this if-block().
-      //
-      // The deployment profile always returns an empty object if there is no profile.
-      // This means that we treat an empty hash defaults profile, or an empty registry hive,
-      // as if there is no profile in place (for the purposes of setting the first-run entry).
-      merge(settings, deploymentProfiles.defaults);
-      if (!Object.keys(deploymentProfiles.defaults).length && !Object.keys(deploymentProfiles.locked).length) {
-        _isFirstRun = true;
-      }
+      return createSettings(deploymentProfiles);
+    } else {
+      // JSON problems in the settings file will be caught, and we let any
+      // other errors (most likely permission-related) bubble up to the surface
+      // and most likely result in a dialog box and the app shutting down.
+      throw err;
     }
   }
-  if ((os.platform() === 'darwin' || os.platform() === 'linux') &&
-    !settings.virtualMachine.memoryInGB) {
-    const totalMemoryInGB = os.totalmem() / 2 ** 30;
-
-    // 25% of available ram up to a maximum of 6gb
-    settings.virtualMachine.memoryInGB = Math.min(6, Math.round(totalMemoryInGB / 4.0));
-  }
+}
 
-  // determine whether updates should be enabled
+function finishConfiguringSettings(cfg: Settings, deploymentProfiles: DeploymentProfileType): Settings {
   if (process.env.RD_FORCE_UPDATES_ENABLED) {
     console.debug('updates enabled via RD_FORCE_UPDATES_ENABLED');
-    settings.application.updater.enabled = true;
+    cfg.application.updater.enabled = true;
   } else if (os.platform() === 'linux' && !process.env.APPIMAGE) {
-    settings.application.updater.enabled = false;
+    cfg.application.updater.enabled = false;
   } else {
     const appVersion = getProductionVersion();
 
@@ -273,18 +269,29 @@ export function load(deploymentProfiles: DeploymentProfileType): Settings {
     // CI builds use a version string like `git describe`, e.g. "v1.1.0-4140-g717225dc".
     // Versions like "1.9.0-tech-preview" are pre-releases and not CI builds, so should not disable auto-update.
     if (appVersion.match(/^v?\d+\.\d+\.\d+-\d+-g[0-9a-f]+$/) || appVersion.includes('?')) {
-      settings.application.updater.enabled = false;
+      cfg.application.updater.enabled = false;
       console.log('updates disabled');
     }
   }
   // Replace existing settings fields with whatever is set in the locked deployment-profile
-  merge(settings, deploymentProfiles.locked);
-  save(settings);
-  lockedSettings = determineLockedFields(deploymentProfiles.locked);
+  merge(cfg, deploymentProfiles.locked);
+  save(cfg);
+  // Update the global settings variable for later retrieval
+  settings = cfg;
 
-  return settings;
+  return cfg;
 }
 
+function getDefaultMemory() {
+  if (os.platform() === 'darwin' || os.platform() === 'linux') {
+    const totalMemoryInGB = os.totalmem() / 2 ** 30;
+
+    // 25% of available ram up to a maximum of 6gb
+    return Math.min(6, Math.round(totalMemoryInGB / 4.0));
+  } else {
+    return 2;
+  }
+}
 /**
  * Merge settings in-place with changes, returning the merged settings.
  * @param cfg Baseline settings.  This will be modified.
@@ -321,6 +328,10 @@ export function getLockedSettings(): LockedSettingsType {
   return lockedSettings;
 }
 
+export function updateLockedFields(lockedDeploymentProfile: RecursivePartial<Settings>) {
+  lockedSettings = determineLockedFields(lockedDeploymentProfile);
+}
+
 /**
  * Returns an object that mirrors `lockedProfileSettings` but all leaves are `true`.
  * @param lockedProfileSettings

pkg/rancher-desktop/store/applicationSettings.ts

@@ -1,7 +1,7 @@
 
 import { ActionContext, MutationsType } from './ts-helpers';
 
-import { load as loadSettings } from '@pkg/config/settings';
+import { getSettings } from '@pkg/config/settings';
 import type { PathManagementStrategy } from '@pkg/integrations/pathManager';
 import { ipcRenderer } from '@pkg/utils/ipcRenderer';
 
@@ -16,7 +16,7 @@ type State = {
 export const state: () => State = () => {
   // While we load the settings from disk here, we only otherwise interact with
   // the settings only via ipcRenderer.
-  const cfg = loadSettings({ defaults: {}, locked: {} });
+  const cfg = getSettings();
 
   return {
     pathManagementStrategy: cfg.application.pathManagementStrategy,

pkg/rancher-desktop/window/index.ts

@@ -6,7 +6,7 @@ import Electron, {
 } from 'electron';
 
 import * as K8s from '@pkg/backend/k8s';
-import { load as loadSettings } from '@pkg/config/settings';
+import { getSettings } from '@pkg/config/settings';
 import { IpcRendererEvents } from '@pkg/typings/electron-ipc';
 import { isDevEnv } from '@pkg/utils/environment';
 import Logging from '@pkg/utils/logging';
@@ -178,7 +178,7 @@ export function openMain() {
   }
 
   window.on('closed', () => {
-    const cfg = loadSettings({ defaults: {}, locked: {} });
+    const cfg = getSettings();
 
     if (cfg.application.window.quitOnClose) {
       BrowserWindow.getAllWindows().forEach((window) => {