Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: allow to customize config values on create new page #228

Merged
merged 8 commits into from
Jul 29, 2024
Merged

fix: allow to customize config values on create new page #228

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
7e10c2a
fix: allow to customize config values on create new page
lstocchi Jun 14, 2024
1c0e5ce
fix: format and eslint errors
dgolovin Jun 15, 2024
ca9d37f
fix: add test
lstocchi Jul 1, 2024
1dcfc94
fix: rebase
lstocchi Jul 1, 2024
b5b7491
fix: lint
lstocchi Jul 1, 2024
ed95663
fix: update description of pull secret field
lstocchi Jul 15, 2024
b07bba2
fix: update disk/memory field description
lstocchi Jul 17, 2024
23e9a07
fix: do not dispose factory when finish starting the VM
lstocchi Jul 25, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
95 changes: 82 additions & 13 deletions package.json
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,23 +7,71 @@
"icon": "icon.png",
"license": "Apache-2.0",
"engines": {
"podman-desktop": "^0.16.0"
"podman-desktop": ">=1.8.0"
},
"main": "./dist/extension.cjs",
"source": "./src/extension.ts",
"contributes": {
"configuration": {
"title": "Red Hat OpenShift Local",
"properties": {
"OpenShift-Local.memory": {
"crc.factory.openshift.memory": {
"type": "number",
"format": "memory",
"description": "Memory size, in MiB"
"minimum": 11000000000,
jeffmaury marked this conversation as resolved.
Show resolved Hide resolved
"default": 11000000000,
"maximum": "HOST_TOTAL_MEMORY",
"step": 500000000,
"description": "Memory size",
"scope": [
"KubernetesProviderConnectionFactory",
"DEFAULT"
],
lstocchi marked this conversation as resolved.
Show resolved Hide resolved
"hidden": true,
"when": "crc.crcPreset == openshift"
},
"crc.factory.microshift.memory": {
"type": "number",
"format": "memory",
"minimum": 4000000000,
"default": 4000000000,
"maximum": "HOST_TOTAL_MEMORY",
"step": 500000000,
"description": "Memory size",
"scope": [
"KubernetesProviderConnectionFactory",
"DEFAULT"
],
"hidden": true,
"when": "crc.crcPreset == microshift"
},
"OpenShift-Local.cpus": {
"crc.factory.openshift.cpus": {
"type": "number",
"format": "cpu",
"description": "Number of CPU cores"
"minimum": 4,
"default": "HOST_HALF_CPU_CORES",
"maximum": "HOST_TOTAL_CPU",
"description": "Number of CPU cores",
"scope": [
"KubernetesProviderConnectionFactory",
"DEFAULT"
],
"hidden": true,
"when": "crc.crcPreset == openshift"
},
"crc.factory.microshift.cpus": {
"type": "number",
"format": "cpu",
"minimum": 2,
"default": "HOST_HALF_CPU_CORES",
"maximum": "HOST_TOTAL_CPU",
"description": "Number of CPU cores",
"scope": [
"KubernetesProviderConnectionFactory",
"DEFAULT"
],
"hidden": true,
"when": "crc.crcPreset == microshift"
},
"OpenShift-Local.preset": {
"type": "string",
Expand All @@ -34,17 +82,38 @@
"default": "openshift",
"description": "OpenShift Local Virtual machine preset"
},
"OpenShift-Local.disksize": {
"crc.factory.disksize": {
"type": "number",
"format": "memory",
"default": 31,
"minimum": 20,
"description": "Disk size, in GiB"
"format": "diskSize",
"minimum": 35000000000,
"maximum": "HOST_TOTAL_DISKSIZE",
"step": 500000000,
"description": "Disk size",
"scope": [
"KubernetesProviderConnectionFactory",
"DEFAULT"
],
"hidden": true
},
"OpenShift-Local.pullsecretfile": {
"crc.factory.pullsecretfile": {
"type": "string",
"format": "file",
"markdownDescription": "Path of image pull secret (download from the [Red Hat OpenShift Local download page](https://console.redhat.com/openshift/create/local?sc_cid=7013a000003SUmqAAG))"
"markdownDescription": "Path of image pull secret (optional). If omitted, you'll be prompted to log in with Red Hat SSO and download the pull secret from the [Red Hat OpenShift Local download page](https://console.redhat.com/openshift/create/local?sc_cid=7013a000003SUmqAAG) before starting OpenShift Local",
"scope": [
"KubernetesProviderConnectionFactory",
"DEFAULT"
],
"hidden": true
},
"crc.factory.start.now": {
"type": "boolean",
"default": true,
"scope": [
"KubernetesProviderConnectionFactory",
"DEFAULT"
],
"hidden": true,
"description": "Start the VM now"
}
}
},
Expand Down Expand Up @@ -73,7 +142,7 @@
"@redhat-developer/rhaccm-client": "^0.0.1"
},
"devDependencies": {
"@podman-desktop/api": "next",
"@podman-desktop/api": "1.10.3",
"@types/node": "^18.14.6",
"@typescript-eslint/eslint-plugin": "^5.55.0",
"@typescript-eslint/parser": "^5.55.0",
Expand Down
2 changes: 1 addition & 1 deletion src/daemon-commander.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,7 +90,7 @@ export class DaemonCommander {
return JSON.parse(body).Configs;
}

async configSet(values: Configuration): Promise<void> {
async configSet(values: Configuration | { [key: string]: string | number }): Promise<void> {
const url = this.apiPath + '/config';

const result = await got.post(url, {
Expand Down
17 changes: 9 additions & 8 deletions src/extension.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,7 +32,7 @@ import { crcStatus } from './crc-status';
import { startCrc } from './crc-start';
import { needSetup, setUpCrc } from './crc-setup';
import { deleteCrc, registerDeleteCommand } from './crc-delete';
import { presetChangedEvent, syncPreferences } from './preferences';
import { presetChangedEvent, saveConfig, syncPreferences } from './preferences';
import { stopCrc } from './crc-stop';
import { registerOpenTerminalCommand } from './dev-terminal';
import { commandManager } from './command';
Expand All @@ -43,6 +43,7 @@ import { pushImageToCrcCluster } from './image-handler';
import type { Preset } from './types';

const CRC_PUSH_IMAGE_TO_CLUSTER = 'crc.image.push.to.cluster';
const CRC_PRESET_KEY = 'crc.crcPreset';

let connectionDisposable: extensionApi.Disposable;
let connectionFactoryDisposable: extensionApi.Disposable;
Expand Down Expand Up @@ -136,6 +137,7 @@ async function _activate(extensionContext: extensionApi.ExtensionContext): Promi
} else {
// else get preset from cli as setup is not finished and daemon may not running
const preset = await getPreset();
extensionApi.context.setValue(CRC_PRESET_KEY, preset ?? 'openshift');
if (preset) {
updateProviderVersionWithPreset(provider, preset);
}
Expand Down Expand Up @@ -221,9 +223,12 @@ function registerProviderConnectionFactory(
initialize: async () => {
await createCrcVm(provider, extensionContext, telemetryLogger, defaultLogger);
},
create: async (_, logger) => {
create: async (params, logger) => {
await presetChanged(provider, extensionContext, telemetryLogger);
await createCrcVm(provider, extensionContext, telemetryLogger, logger);
await saveConfig(params);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would suggest to preserve previous behavior when this form creates and starts VM.

When opened it should show default values set in CRC Extension settings and let user to changes them and use as part of 'crc start' command without saving them in settings as default.

There could be a switch 'Save as default settings' if we want and saveConfig method should be called in create function before calling createCrcVm().

Copy link
Contributor Author

@lstocchi lstocchi Jul 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I agree about showing the default values. The initial plan i had was this one but this is not currently supported by Desktop as far as i recall and I don't think a user is going to delete/create the VM several time so we should be good with what we have now as an initial release. This can be done in a follow-up PR. Even because it can be useful for other extensions and it is an improvement we should do on the Desktop side.

Regarding saving the values as default vs using them when starting the VM - i think there is a bit of confusion about how crc fit inside the Podman Desktop mechanisms.

In the create new page we usually create the instance of something (for podman - a podman machine, for kind - a kind cluster) but we are not actually starting them (unless you set the option start now). In CRC i tried to do the same but crc does not have the concept of a new instance. Everything is set up during the setup phase and then you have to start it.
So in the "creation" process what we do is just registering the kubernetes connection to Desktop with its custom lifecycle (start/stop/delete).

Now if you want to start the VM you can click on the start button from the resources page, but in that case you don't have any way to set the memory/cpu/disk, and i don't think that showing a quickpick is really user friendly. So it is easier to set them globally during the "create" process so that when the user start the VM, the properties are already saved.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes +1, I don't think the settings for the "OpenShift Local" environments should be handle in a different way than what we are doing for podman machine. (For podman machine, we have "default values", but the user has no way to configure those in the preferences.)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sounds good to me then with nit. There should be markup paragraph before pull-secret file location configuration field explaining that if you have file with it you can configure it here, but it is optional. If you do not fill up file location for pull-secret file you are going to be requested to login using Red Hat SSO to pull pull-secret and store it in platform specific secure storage.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@dgolovin done. Description updated. You can see it in the image on the first post above

if (params['crc.factory.start.now']) {
await createCrcVm(provider, extensionContext, telemetryLogger, logger);
}
},
},
{
Expand Down Expand Up @@ -258,10 +263,6 @@ async function createCrcVm(
addCommands(telemetryLogger);
presetChanged(provider, extensionContext, telemetryLogger);
}

if (connectionFactoryDisposable) {
connectionFactoryDisposable.dispose();
}
}

async function initializeCrc(
Expand Down Expand Up @@ -406,7 +407,7 @@ async function presetChanged(
): Promise<void> {
// detect preset of CRC
const preset = await readPreset();

extensionApi.context.setValue(CRC_PRESET_KEY, preset);
updateProviderVersionWithPreset(provider, preset);

if (connectionDisposable) {
Expand Down
89 changes: 89 additions & 0 deletions src/preferences.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,89 @@
/**********************************************************************
* Copyright (C) 2024 Red Hat, Inc.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*
* SPDX-License-Identifier: Apache-2.0
***********************************************************************/

import * as extensionApi from '@podman-desktop/api';
import { expect, test, vi } from 'vitest';
import * as crcCli from './crc-cli';
import * as daemon from './daemon-commander';
import type { Configuration } from './types';
import * as preferences from './preferences';
import * as crcStatus from './crc-status';

vi.mock('./crc-status', async () => {
return {
crcStatus: {
onStatusChange: vi.fn(),
},
};
});

vi.mock('@podman-desktop/api', async () => {
return {
configuration: {
getConfiguration: vi.fn(),
},
EventEmitter: vi.fn(),
};
});

test('should update configuration accordingly with params', async () => {
vi.mocked(crcStatus);
const updateConfigMock = vi.fn();
const apiConfig: extensionApi.Configuration = {
update: updateConfigMock,
get: function <T>(): T {
throw new Error('Function not implemented.');
},
has: function (): boolean {
throw new Error('Function not implemented.');
},
};
const configuration: Configuration = {
cpus: 10,
memory: 286102,
'disk-size': 186,
'pull-secret-file': 'file',
preset: 'openshift',
};

vi.spyOn(daemon.commander, 'configGet').mockResolvedValue(configuration);
vi.spyOn(extensionApi.configuration, 'getConfiguration').mockReturnValue(apiConfig);

vi.spyOn(crcCli, 'getPreset').mockResolvedValue(undefined);

const configSetMock = vi.spyOn(daemon.commander, 'configSet').mockImplementation(() => {
return Promise.resolve();
});
await preferences.saveConfig({
'crc.factory.openshift.cpus': '10',
'crc.factory.openshift.memory': '300000000000',
'crc.factory.disksize': '200000000000',
'crc.factory.pullsecretfile': 'file',
});

expect(configSetMock).toHaveBeenCalledWith({
cpus: 10,
memory: 286102,
'disk-size': 186,
'pull-secret-file': 'file',
});
expect(updateConfigMock).toHaveBeenNthCalledWith(2, 'crc.factory.openshift.memory', 299999690752);
expect(updateConfigMock).toHaveBeenNthCalledWith(3, 'crc.factory.openshift.cpus', 10);
expect(updateConfigMock).toHaveBeenNthCalledWith(4, 'crc.factory.disksize', 199715979264);
expect(updateConfigMock).toHaveBeenNthCalledWith(5, 'crc.factory.pullsecretfile', 'file');
});
57 changes: 33 additions & 24 deletions src/preferences.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,19 +20,17 @@ import * as extensionApi from '@podman-desktop/api';
import type { Configuration, Preset } from './types';
import { commander } from './daemon-commander';
import { isEmpty, productName } from './util';
import { getDefaultCPUs, getDefaultMemory } from './constants';
import { crcStatus } from './crc-status';
import { stopCrc } from './crc-stop';
import { deleteCrc } from './crc-delete';
import { startCrc } from './crc-start';
import { defaultLogger } from './logger';
import { getPreset } from './crc-cli';

const presetChangedEventEmitter = new extensionApi.EventEmitter<Preset>();
export const presetChangedEvent = presetChangedEventEmitter.event;

const configMap = {
'OpenShift-Local.cpus': { name: 'cpus', label: 'CPUS', needDialog: true, validation: validateCpus },
'OpenShift-Local.memory': { name: 'memory', label: 'Memory', needDialog: true, validation: validateRam },
'OpenShift-Local.preset': {
name: 'preset',
label: 'Preset',
Expand All @@ -41,8 +39,6 @@ const configMap = {
requiresRefresh: true,
fireEvent: 'preset',
},
'OpenShift-Local.disksize': { name: 'disk-size', label: 'Disk Size', needDialog: true },
'OpenShift-Local.pullsecretfile': { name: 'pull-secret-file', label: 'Pullsecret file path', needDialog: false },
} as {
[key: string]: ConfigEntry;
};
Expand Down Expand Up @@ -80,14 +76,7 @@ export async function syncPreferences(
telemetryLogger: extensionApi.TelemetryLogger,
): Promise<void> {
try {
initialCrcConfig = await commander.configGet();

const extConfig = extensionApi.configuration.getConfiguration();

for (const key in configMap) {
const element = configMap[key];
await extConfig.update(key, initialCrcConfig[element.name]);
}
await refreshConfig();

context.subscriptions.push(
extensionApi.configuration.onDidChangeConfiguration(e => {
Expand Down Expand Up @@ -258,6 +247,15 @@ async function refreshConfig(): Promise<void> {
const element = configMap[key];
await extConfig.update(key, initialCrcConfig[element.name]);
}

const preset = initialCrcConfig.preset ?? 'openshift';
if (preset !== 'podman') {
await extConfig.update(`crc.factory.${preset}.memory`, +initialCrcConfig['memory'] * (1024 * 1024));
await extConfig.update(`crc.factory.${preset}.cpus`, initialCrcConfig['cpus']);
await extConfig.update('crc.factory.disksize', +initialCrcConfig['disk-size'] * (1024 * 1024 * 1024));
}

await extConfig.update('crc.factory.pullsecretfile', initialCrcConfig['pull-secret-file']);
} finally {
isRefreshing = false;
}
Expand All @@ -279,22 +277,33 @@ async function useCrcSettingValue(name: string, settingValue: string, crcConfigV
return false;
}

function validateCpus(newVal: string | number, preset: Preset): string | undefined {
if (typeof newVal !== 'number') {
return 'CPUs should be a number';
export async function saveConfig(params: {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
[key: string]: any;
}) {
const preset = (await getPreset()) ?? 'openshift';

const configuration: { [key: string]: string | number } = {};
if (params[`crc.factory.${preset}.cpus`]) {
configuration.cpus = +params[`crc.factory.${preset}.cpus`];
}
if (newVal < getDefaultCPUs(preset)) {
return `Requires CPUs >= ${getDefaultCPUs(preset)}`;

if (params[`crc.factory.${preset}.memory`]) {
const memoryAsMiB = +params[`crc.factory.${preset}.memory`] / (1024 * 1024);
configuration.memory = Math.floor(memoryAsMiB);
}
}

function validateRam(newVal: string | number, preset: Preset): string | undefined {
if (typeof newVal !== 'number') {
return 'Memory should be a number';
if (params['crc.factory.disksize']) {
const diskAsGiB = +params['crc.factory.disksize'] / (1024 * 1024 * 1024);
configuration['disk-size'] = Math.floor(diskAsGiB);
}
if (newVal < getDefaultMemory(preset)) {
return `Requires Memory in MiB >= ${getDefaultMemory(preset)}`;

if (params['crc.factory.pullsecretfile']) {
configuration['pull-secret-file'] = params['crc.factory.pullsecretfile'];
}

await commander.configSet(configuration);
await refreshConfig();
}

async function handleRecreate(
Expand Down
Loading
Loading