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.

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: Refactor task and handler classes #1205

Open
wants to merge 19 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from 10 commits
Commits
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
91 changes: 6 additions & 85 deletions src/commands/base.ts
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@
*
*/

import paths from 'path';
import {MissingArgumentError, SoloError} from '../core/errors.js';
import {ShellRunner} from '../core/shell_runner.js';
import {type LeaseManager} from '../core/lease/lease_manager.js';
Expand All @@ -34,10 +33,7 @@
import * as constants from '../core/constants.js';
import fs from 'fs';
import {Task} from '../core/task.js';

export interface CommandHandlers {
parent: BaseCommand;
}
import {getConfig} from '../core/config_builder.js';

export abstract class BaseCommand extends ShellRunner {
protected readonly helm: Helm;
Expand Down Expand Up @@ -71,6 +67,7 @@
this.remoteConfigManager = opts.remoteConfigManager;
}

// TODO remove
Ivo-Yankov marked this conversation as resolved.
Show resolved Hide resolved
async prepareChartPath(chartDir: string, chartRepo: string, chartReleaseName: string) {
if (!chartRepo) throw new MissingArgumentError('chart repo name is required');
if (!chartReleaseName) throw new MissingArgumentError('chart release name is required');
Expand All @@ -84,19 +81,6 @@
return `${chartRepo}/${chartReleaseName}`;
}

prepareValuesFiles(valuesFile: string) {
let valuesArg = '';
if (valuesFile) {
const valuesFiles = valuesFile.split(',');
valuesFiles.forEach(vf => {
const vfp = paths.resolve(vf);
valuesArg += ` --values ${vfp}`;
});
}

return valuesArg;
}

getConfigManager(): ConfigManager {
return this.configManager;
}
Expand All @@ -111,71 +95,7 @@
* getUnusedConfigs() to get an array of unused properties.
*/
getConfig(configName: string, flags: CommandFlag[], extraProperties: string[] = []): object {
const configManager = this.configManager;

// build the dynamic class that will keep track of which properties are used
const NewConfigClass = class {
private usedConfigs: Map<string, number>;
constructor() {
// the map to keep track of which properties are used
this.usedConfigs = new Map();

// add the flags as properties to this class
flags?.forEach(flag => {
// @ts-ignore
this[`_${flag.constName}`] = configManager.getFlag(flag);
Object.defineProperty(this, flag.constName, {
get() {
this.usedConfigs.set(flag.constName, this.usedConfigs.get(flag.constName) + 1 || 1);
return this[`_${flag.constName}`];
},
});
});

// add the extra properties as properties to this class
extraProperties?.forEach(name => {
// @ts-ignore
this[`_${name}`] = '';
Object.defineProperty(this, name, {
get() {
this.usedConfigs.set(name, this.usedConfigs.get(name) + 1 || 1);
return this[`_${name}`];
},
set(value) {
this[`_${name}`] = value;
},
});
});
}

/** Get the list of unused configurations that were not accessed */
getUnusedConfigs() {
const unusedConfigs: string[] = [];

// add the flag constName to the unusedConfigs array if it was not accessed
flags?.forEach(flag => {
if (!this.usedConfigs.has(flag.constName)) {
unusedConfigs.push(flag.constName);
}
});

// add the extra properties to the unusedConfigs array if it was not accessed
extraProperties?.forEach(item => {
if (!this.usedConfigs.has(item)) {
unusedConfigs.push(item);
}
});
return unusedConfigs;
}
};

const newConfigInstance = new NewConfigClass();

// add the new instance to the configMaps so that it can be used to get the
// unused configurations using the configName from the BaseCommand
this._configMaps.set(configName, newConfigInstance);

return newConfigInstance;
return getConfig(this.configManager, this._configMaps, configName, flags, extraProperties);
}

getLeaseManager(): LeaseManager {
Expand Down Expand Up @@ -204,8 +124,9 @@

abstract close(): Promise<void>;

// TODO remove this
commandActionBuilder(actionTasks: any, options: any, errorString: string, lease: Lease | null) {
return async function (argv: any, commandDef: CommandHandlers) {
return async function (argv: any, commandDef) {

Check warning on line 129 in src/commands/base.ts

View check run for this annotation

Codecov / codecov/patch

src/commands/base.ts#L129

Added line #L129 was not covered by tests
const tasks = new Listr([...actionTasks], options);

try {
Expand Down Expand Up @@ -246,7 +167,7 @@
self.logger.debug(`OK: setup directory: ${dirPath}`);
});
} catch (e: Error | any) {
this.logger.error(e);
self.logger.error(e);

Check warning on line 170 in src/commands/base.ts

View check run for this annotation

Codecov / codecov/patch

src/commands/base.ts#L170

Added line #L170 was not covered by tests
throw new SoloError(`failed to create directory: ${e.message}`, e);
}

Expand Down
25 changes: 13 additions & 12 deletions src/commands/cluster/configs.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,8 +36,7 @@ export const connectConfigBuilder = async function (argv, ctx, task) {
};

export const setupConfigBuilder = async function (argv, ctx, task) {
const parent = this.parent;
const configManager = parent.getConfigManager();
const configManager = this.configManager;
configManager.update(argv);
flags.disablePrompts([flags.chartDirectory]);

Expand All @@ -60,11 +59,12 @@ export const setupConfigBuilder = async function (argv, ctx, task) {
soloChartVersion: configManager.getFlag(flags.soloChartVersion) as string,
} as ClusterSetupConfigClass;

parent.logger.debug('Prepare ctx.config', {config: ctx.config, argv});
this.logger.debug('Prepare ctx.config', {config: ctx.config, argv});

ctx.isChartInstalled = await parent
.getChartManager()
.isChartInstalled(ctx.config.clusterSetupNamespace, constants.SOLO_CLUSTER_SETUP_CHART);
ctx.isChartInstalled = await this.chartManager.isChartInstalled(
ctx.config.clusterSetupNamespace,
constants.SOLO_CLUSTER_SETUP_CHART,
);

return ctx.config;
};
Expand All @@ -83,16 +83,17 @@ export const resetConfigBuilder = async function (argv, ctx, task) {
}
}

this.parent.getConfigManager().update(argv);
this.configManager.update(argv);

ctx.config = {
clusterName: this.parent.getConfigManager().getFlag(flags.clusterName) as string,
clusterSetupNamespace: this.parent.getConfigManager().getFlag(flags.clusterSetupNamespace) as string,
clusterName: this.configManager.getFlag(flags.clusterName) as string,
clusterSetupNamespace: this.configManager.getFlag(flags.clusterSetupNamespace) as string,
} as ClusterResetConfigClass;

ctx.isChartInstalled = await this.parent
.getChartManager()
.isChartInstalled(ctx.config.clusterSetupNamespace, constants.SOLO_CLUSTER_SETUP_CHART);
ctx.isChartInstalled = await this.chartManager.isChartInstalled(
ctx.config.clusterSetupNamespace,
constants.SOLO_CLUSTER_SETUP_CHART,
);
if (!ctx.isChartInstalled) {
throw new SoloError('No chart found for the cluster');
}
Expand Down
56 changes: 33 additions & 23 deletions src/commands/cluster/handlers.ts
Original file line number Diff line number Diff line change
Expand Up @@ -14,39 +14,49 @@
* limitations under the License.
*
*/
import {type BaseCommand, type CommandHandlers} from '../base.js';
import {type ClusterCommandTasks} from './tasks.js';
import {ClusterCommandTasks} from './tasks.js';
import * as helpers from '../../core/helpers.js';
import * as constants from '../../core/constants.js';
import * as ContextFlags from './flags.js';
import {ListrRemoteConfig} from '../../core/config/remote/listr_config_tasks.js';
import type {RemoteConfigManager} from '../../core/config/remote/remote_config_manager.js';
import {RemoteConfigManager} from '../../core/config/remote/remote_config_manager.js';
import {connectConfigBuilder, resetConfigBuilder, setupConfigBuilder} from './configs.js';
import {SoloError} from '../../core/errors.js';

export class ClusterCommandHandlers implements CommandHandlers {
readonly parent: BaseCommand;
readonly tasks: ClusterCommandTasks;
public readonly remoteConfigManager: RemoteConfigManager;
private getConfig: any;

constructor(parent: BaseCommand, tasks: ClusterCommandTasks, remoteConfigManager: RemoteConfigManager) {
this.parent = parent;
this.tasks = tasks;
this.remoteConfigManager = remoteConfigManager;
this.getConfig = parent.getConfig.bind(parent);
import {inject, injectable} from 'tsyringe-neo';
import {patchInject} from '../../core/container_helper.js';
import {K8} from '../../core/k8.js';
import {CommandHandler} from '../../core/command_handler.js';
import {LocalConfig} from '../../core/config/local_config.js';
import {ChartManager} from '../../core/chart_manager.js';

@injectable()
export class ClusterCommandHandlers extends CommandHandler {
constructor(
@inject(ClusterCommandTasks) private readonly tasks: ClusterCommandTasks,
@inject(RemoteConfigManager) private readonly remoteConfigManager: RemoteConfigManager,
@inject(LocalConfig) private readonly localConfig: LocalConfig,
@inject(K8) private readonly k8: K8,
@inject(ChartManager) private readonly chartManager: ChartManager,
) {
super();

this.tasks = patchInject(tasks, ClusterCommandTasks, this.constructor.name);
this.remoteConfigManager = patchInject(remoteConfigManager, RemoteConfigManager, this.constructor.name);
this.k8 = patchInject(k8, K8, this.constructor.name);
this.localConfig = patchInject(localConfig, LocalConfig, this.constructor.name);
this.chartManager = patchInject(chartManager, ChartManager, this.constructor.name);
}

async connect(argv: any) {
argv = helpers.addFlagsToArgv(argv, ContextFlags.USE_FLAGS);

const action = this.parent.commandActionBuilder(
const action = this.commandActionBuilder(

Check warning on line 53 in src/commands/cluster/handlers.ts

View check run for this annotation

Codecov / codecov/patch

src/commands/cluster/handlers.ts#L53

Added line #L53 was not covered by tests
[
this.tasks.initialize(argv, connectConfigBuilder.bind(this)),
this.parent.setupHomeDirectoryTask(),
this.parent.getLocalConfig().promptLocalConfigTask(this.parent.getK8()),
this.setupHomeDirectoryTask(),
this.localConfig.promptLocalConfigTask(this.k8),

Check warning on line 57 in src/commands/cluster/handlers.ts

View check run for this annotation

Codecov / codecov/patch

src/commands/cluster/handlers.ts#L56-L57

Added lines #L56 - L57 were not covered by tests
this.tasks.selectContext(),
ListrRemoteConfig.loadRemoteConfig(this.parent, argv),
ListrRemoteConfig.loadRemoteConfig(this.remoteConfigManager, argv),

Check warning on line 59 in src/commands/cluster/handlers.ts

View check run for this annotation

Codecov / codecov/patch

src/commands/cluster/handlers.ts#L59

Added line #L59 was not covered by tests
this.tasks.readClustersFromRemoteConfig(argv),
this.tasks.updateLocalConfig(),
],
Expand All @@ -65,7 +75,7 @@
async list(argv: any) {
argv = helpers.addFlagsToArgv(argv, ContextFlags.USE_FLAGS);

const action = this.parent.commandActionBuilder(
const action = this.commandActionBuilder(
[this.tasks.showClusterList()],
{
concurrent: false,
Expand All @@ -82,7 +92,7 @@
async info(argv: any) {
argv = helpers.addFlagsToArgv(argv, ContextFlags.USE_FLAGS);

const action = this.parent.commandActionBuilder(
const action = this.commandActionBuilder(
[this.tasks.getClusterInfo()],
{
concurrent: false,
Expand All @@ -99,7 +109,7 @@
async setup(argv: any) {
argv = helpers.addFlagsToArgv(argv, ContextFlags.USE_FLAGS);

const action = this.parent.commandActionBuilder(
const action = this.commandActionBuilder(
[
this.tasks.initialize(argv, setupConfigBuilder.bind(this)),
this.tasks.prepareChartValues(argv),
Expand All @@ -125,7 +135,7 @@
async reset(argv: any) {
argv = helpers.addFlagsToArgv(argv, ContextFlags.USE_FLAGS);

const action = this.parent.commandActionBuilder(
const action = this.commandActionBuilder(
[
this.tasks.initialize(argv, resetConfigBuilder.bind(this)),
this.tasks.acquireNewLease(argv),
Expand Down
5 changes: 3 additions & 2 deletions src/commands/cluster/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -19,9 +19,9 @@ import * as ContextFlags from './flags.js';
import {YargsCommand} from '../../core/yargs_command.js';
import {BaseCommand} from './../base.js';
import {type Opts} from '../../types/command_types.js';
import {ClusterCommandTasks} from './tasks.js';
import {ClusterCommandHandlers} from './handlers.js';
import {DEFAULT_FLAGS, RESET_FLAGS, SETUP_FLAGS} from './flags.js';
import {patchInject} from '../../core/container_helper.js';

/**
* Defines the core functionalities of 'node' command
Expand All @@ -32,7 +32,8 @@ export class ClusterCommand extends BaseCommand {
constructor(opts: Opts) {
super(opts);

this.handlers = new ClusterCommandHandlers(this, new ClusterCommandTasks(this, this.k8), this.remoteConfigManager);
// TODO properly inject
Ivo-Yankov marked this conversation as resolved.
Show resolved Hide resolved
this.handlers = patchInject(null, ClusterCommandHandlers, this.constructor.name);
}

getCommandDefinition() {
Expand Down
Loading
Loading