From 608634281553705b1f7cb8223f59cfd705c0f50f Mon Sep 17 00:00:00 2001 From: Eric Promislow Date: Fri, 23 Jun 2023 11:01:27 -0700 Subject: [PATCH] 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: Same as createSettings if there's no settings file, otherwise load from disk and process Set up code common to both `load` and `createSettings` has been pulled out into a separate function `finishConfiguringSettings`. Signed-off-by: Eric Promislow --- background.ts | 10 +- bats/tests/helpers/utils.bash | 2 +- .../config/__tests__/settings.spec.ts | 7 +- pkg/rancher-desktop/config/settings.ts | 118 +++++++++--------- .../store/applicationSettings.ts | 4 +- pkg/rancher-desktop/window/index.ts | 4 +- 6 files changed, 77 insertions(+), 68 deletions(-) diff --git a/background.ts b/background.ts index 3c5f716fec5..a9feb219c06 100644 --- a/background.ts +++ b/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. diff --git a/bats/tests/helpers/utils.bash b/bats/tests/helpers/utils.bash index 8301c03b76e..30be3173933 100644 --- a/bats/tests/helpers/utils.bash +++ b/bats/tests/helpers/utils.bash @@ -89,7 +89,7 @@ update_allowed_patterns() { local patterns=$2 rdctl api settings -X PUT --input - < { beforeEach(() => { jest.spyOn(fs, 'writeFileSync').mockImplementation(() => { }); - settings.clearSettings(); }); afterEach(() => { mock.mockRestore(); @@ -294,7 +293,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()); }); }); @@ -323,7 +323,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 { diff --git a/pkg/rancher-desktop/config/settings.ts b/pkg/rancher-desktop/config/settings.ts index 577100fdd66..6b21c7815e0 100644 --- a/pkg/rancher-desktop/config/settings.ts +++ b/pkg/rancher-desktop/config/settings.ts @@ -94,7 +94,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,66 +214,53 @@ export function save(cfg: Settings) { } } -/** - * Remove all stored settings. - */ -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 getSettings(): Settings { + return settings ?? defaultSettings; } /** - * Used for unit testing only. - * Could be used in core code if we ever want to reload changed deployment profiles, but that isn't needed now. + * createSetttings + * - 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 function clearSettings() { - settings = undefined; +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 { - if (settings) { - return 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 the + // Don't catch file permission problems. + // Other problems + 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(); @@ -284,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. @@ -332,6 +328,10 @@ export function getLockedSettings(): LockedSettingsType { return lockedSettings; } +export function updateLockedFields(lockedDeploymentProfile: RecursivePartial) { + lockedSettings = determineLockedFields(lockedDeploymentProfile); +} + /** * Returns an object that mirrors `lockedProfileSettings` but all leaves are `true`. * @param lockedProfileSettings diff --git a/pkg/rancher-desktop/store/applicationSettings.ts b/pkg/rancher-desktop/store/applicationSettings.ts index ffd533964d8..6adbcab13ab 100644 --- a/pkg/rancher-desktop/store/applicationSettings.ts +++ b/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, diff --git a/pkg/rancher-desktop/window/index.ts b/pkg/rancher-desktop/window/index.ts index 6ebeb1c8c84..901c32b7a09 100644 --- a/pkg/rancher-desktop/window/index.ts +++ b/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) => {