Skip to content
Merged
Show file tree
Hide file tree
Changes from all 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
Original file line number Diff line number Diff line change
@@ -0,0 +1,5 @@
---
'@red-hat-developer-hub/cli-module-install-dynamic-plugins': minor
---

Add enabled field in dynamic plugin config with backwards compatibility for disabled field
Original file line number Diff line number Diff line change
Expand Up @@ -20,7 +20,7 @@ import { verifyIntegrity } from './integrity';
import { log } from './log';
import { run } from './run';
import { extractNpmPackage } from './tar-extract';
import { CONFIG_HASH_FILE, type Plugin } from './types';
import { CONFIG_HASH_FILE, isPluginDisabled, type Plugin } from './types';
import { markAsFresh } from './util';

export type NpmInstallResult = {
Expand All @@ -45,7 +45,7 @@ export async function installNpmPlugin(
skipIntegrity: boolean,
installed: Map<string, string>,
): Promise<NpmInstallResult> {
if (plugin.disabled) {
if (isPluginDisabled(plugin)) {
return { pluginPath: null, pluginConfig: {} };
}
const hash = plugin.plugin_hash;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@ import {
CONFIG_HASH_FILE,
effectivePullPolicy,
IMAGE_HASH_FILE,
isPluginDisabled,
type Plugin,
PullPolicy,
} from './types';
Expand Down Expand Up @@ -62,7 +63,7 @@ export async function installOciPlugin(
imageCache: OciImageCache,
installed: Map<string, string>,
): Promise<OciInstallResult> {
if (plugin.disabled) {
if (isPluginDisabled(plugin)) {
return { pluginPath: null, pluginConfig: {} };
}
const hash = plugin.plugin_hash;
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@ import {
type DynamicPluginsConfig,
effectivePullPolicy,
GLOBAL_CONFIG_FILENAME,
isPluginDisabled,
LOCK_FILENAME,
OCI_PROTO,
type Plugin,
Expand Down Expand Up @@ -374,7 +375,7 @@ function categorize(allPlugins: PluginMap): Categorized {
const npm: Plugin[] = [];
const skipped: Plugin[] = [];
for (const plugin of Object.values(allPlugins)) {
if (plugin.disabled) {
if (isPluginDisabled(plugin, log)) {
log(`\n======= Skipping disabled plugin ${plugin.package}`);
continue;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -262,4 +262,63 @@ describe('filterDisabledOciPlugins', () => {
);
expect(out).toHaveLength(2);
});

it('removes invalid OCI entries that are marked enabled: false', () => {
const plugins: PluginSpec[] = [
{ package: 'oci://bad spec', enabled: false },
{ package: 'oci://also bad' },
];
const out = filterDisabledOciPlugins(plugins, new Set());
expect(out.map(p => p.package)).toEqual(['oci://also bad']);
});
});

describe('preMergeOciDisabledState — enabled field', () => {
it('enabled: false in main disables the registry', () => {
const include: PluginSpec[] = [
{ package: 'oci://registry.example.com/plugin:1.0', enabled: true },
];
const main: PluginSpec[] = [
{
package: 'oci://registry.example.com/plugin:{{inherit}}',
enabled: false,
},
];
const result = preMergeOciDisabledState(
[['include.yaml', include]],
main,
'main.yaml',
);
expect(result.has('oci://registry.example.com/plugin')).toBe(true);
});

it('enabled: true in main re-enables a disabled include', () => {
const include: PluginSpec[] = [
{ package: 'oci://registry.example.com/plugin:1.0', enabled: false },
];
const main: PluginSpec[] = [
{
package: 'oci://registry.example.com/plugin:{{inherit}}',
enabled: true,
},
];
const result = preMergeOciDisabledState(
[['include.yaml', include]],
main,
'main.yaml',
);
expect(result.has('oci://registry.example.com/plugin')).toBe(false);
});

it('enabled takes precedence when both enabled and disabled are set', () => {
const main: PluginSpec[] = [
{
package: 'oci://registry.example.com/plugin:1.0',
enabled: true,
disabled: true,
},
];
const result = preMergeOciDisabledState([], main, 'main.yaml');
expect(result.has('oci://registry.example.com/plugin')).toBe(false);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -83,6 +83,45 @@ describe('mergePlugin — NPM', () => {
expect(all.pkg?.last_modified_level).toBe(1);
});

it('overrides using the enabled field', async () => {
const all: PluginMap = {};
await mergePlugin(
{ package: 'pkg@1.0.0', enabled: true },
all,
'inc.yaml',
0,
);
await mergePlugin(
{ package: 'pkg@2.0.0', enabled: false },
all,
'cfg.yaml',
1,
);
expect(all.pkg?.package).toBe('pkg@2.0.0');
expect(all.pkg?.enabled).toBe(false);
expect(all.pkg?.last_modified_level).toBe(1);
});

it('handles enabled overriding disabled from a lower level', async () => {
const all: PluginMap = {};
await mergePlugin(
{ package: 'pkg@1.0.0', disabled: true },
all,
'inc.yaml',
0,
);
await mergePlugin(
{ package: 'pkg@2.0.0', enabled: true },
all,
'cfg.yaml',
1,
);
expect(all.pkg?.package).toBe('pkg@2.0.0');
expect(all.pkg?.enabled).toBe(true);
expect(all.pkg?.disabled).toBeUndefined();
expect(all.pkg?.last_modified_level).toBe(1);
});

it('raises on duplicates within the same level', async () => {
const all: PluginMap = {};
await mergePlugin({ package: 'pkg@1.0.0' }, all, 'cfg.yaml', 0);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,7 @@ import {
} from './oci-key';
import {
type DynamicPluginsConfig,
isPluginDisabled,
OCI_PROTO,
type Plugin,
type PluginMap,
Expand Down Expand Up @@ -297,6 +298,15 @@ function copyPluginFields(
if (skipSet.has(k) || isForbiddenKey(k)) continue;
safeSet(dst, k, v);
}
// When the override introduces a valid boolean activation field, clear the
// opposite so the merged record never carries both (which would trigger a
// spurious "specifies both" warning in isPluginDisabled). Only act on
// actual booleans — a non-boolean value is treated as "unset" by
// isPluginDisabled and should not displace a valid value on dst.
if (typeof src.enabled === 'boolean' && 'disabled' in dst)
delete (dst as Record<string, unknown>).disabled;
if (typeof src.disabled === 'boolean' && 'enabled' in dst)
delete (dst as Record<string, unknown>).enabled;
}

function isEqual(a: unknown, b: unknown): boolean {
Expand Down Expand Up @@ -422,7 +432,7 @@ function processOciEntry(
): void {
const pkg = plugin.package;
if (typeof pkg !== 'string' || !pkg.startsWith(OCI_PROTO)) return;
const disabled = plugin.disabled === true;
const disabled = isPluginDisabled(plugin);
const parsed = tryParseOciRegistryAndPath(pkg);
if (!parsed) {
logInvalidOciFormat(pkg, sourceFile, disabled);
Expand Down Expand Up @@ -551,7 +561,7 @@ export function filterDisabledOciPlugins(
log(`\n======= Disabling OCI plugin ${pkg}`);
continue;
}
if (!parsed && plugin.disabled === true) {
if (!parsed && isPluginDisabled(plugin)) {
Comment thread
gustavolira marked this conversation as resolved.
log(`\n======= Disabling OCI plugin ${pkg}`);
continue;
}
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
* See the License for the specific language governing permissions and
* limitations under the License.
*/
import { parseMaxEntrySize } from './types';
import { isPluginDisabled, parseMaxEntrySize } from './types';

describe('parseMaxEntrySize', () => {
const DEFAULT = 40_000_000;
Expand Down Expand Up @@ -46,3 +46,102 @@ describe('parseMaxEntrySize', () => {
expect(parseMaxEntrySize('NaN')).toBe(DEFAULT);
});
});

describe('isPluginDisabled', () => {
it('returns false when neither enabled nor disabled is set', () => {
expect(isPluginDisabled({ package: 'pkg@1.0' })).toBe(false);
});

it('returns false when enabled: true', () => {
expect(isPluginDisabled({ package: 'pkg@1.0', enabled: true })).toBe(false);
});

it('returns true when enabled: false', () => {
expect(isPluginDisabled({ package: 'pkg@1.0', enabled: false })).toBe(true);
});

it('returns true when disabled: true (backward compat)', () => {
expect(isPluginDisabled({ package: 'pkg@1.0', disabled: true })).toBe(true);
});

it('returns false when disabled: false (backward compat)', () => {
expect(isPluginDisabled({ package: 'pkg@1.0', disabled: false })).toBe(
false,
);
});

it('enabled takes precedence over disabled when both set (enabled: true, disabled: true)', () => {
const warnings: string[] = [];
const result = isPluginDisabled(
{ package: 'pkg@1.0', enabled: true, disabled: true },
msg => warnings.push(msg),
);
expect(result).toBe(false);
expect(warnings).toHaveLength(1);
expect(warnings[0]).toMatch(/both 'enabled' and 'disabled'/);
});

it('enabled takes precedence over disabled when both set (enabled: false, disabled: false)', () => {
const warnings: string[] = [];
const result = isPluginDisabled(
{ package: 'pkg@1.0', enabled: false, disabled: false },
msg => warnings.push(msg),
);
expect(result).toBe(true);
expect(warnings).toHaveLength(1);
});

it('does not warn when no callback provided', () => {
expect(
isPluginDisabled({ package: 'pkg@1.0', enabled: true, disabled: true }),
).toBe(false);
});

it('treats non-boolean enabled as unset (string "false" is not false)', () => {
const warnings: string[] = [];
const result = isPluginDisabled(
{ package: 'pkg@1.0', enabled: 'false' as unknown as boolean },
msg => warnings.push(msg),
);
expect(result).toBe(false);
expect(warnings).toHaveLength(1);
expect(warnings[0]).toMatch(/non-boolean 'enabled: false'/);
});

it('treats null enabled as unset', () => {
const warnings: string[] = [];
const result = isPluginDisabled(
{ package: 'pkg@1.0', enabled: null as unknown as boolean },
msg => warnings.push(msg),
);
expect(result).toBe(false);
expect(warnings).toHaveLength(1);
expect(warnings[0]).toMatch(/non-boolean 'enabled: null'/);
});

it('treats non-boolean disabled as unset', () => {
const warnings: string[] = [];
const result = isPluginDisabled(
{ package: 'pkg@1.0', disabled: 'true' as unknown as boolean },
msg => warnings.push(msg),
);
expect(result).toBe(false);
expect(warnings).toHaveLength(1);
expect(warnings[0]).toMatch(/non-boolean 'disabled: true'/);
});

it('falls back to valid disabled when enabled is non-boolean', () => {
const warnings: string[] = [];
const result = isPluginDisabled(
{
package: 'pkg@1.0',
enabled: 'yes' as unknown as boolean,
disabled: true,
},
msg => warnings.push(msg),
);
expect(result).toBe(true);
expect(warnings).toHaveLength(1);
expect(warnings[0]).toMatch(/non-boolean 'enabled/);
});
});
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,17 @@ export type PullPolicy = (typeof PullPolicy)[keyof typeof PullPolicy];
*/
export type PluginSpec = {
package: string;
/**
* Recommended: Use `enabled` instead.
* When both `enabled` and `disabled` are present, `enabled` takes precedence.
*/
disabled?: boolean;
/**
* Whether the plugin is active. Preferred over `disabled` (positive logic).
* When both `enabled` and `disabled` are present, `enabled` takes precedence
* and a warning is logged.
*/
enabled?: boolean;
pullPolicy?: PullPolicy;
forceDownload?: boolean;
integrity?: string;
Expand Down Expand Up @@ -112,3 +122,52 @@ export function effectivePullPolicy(plugin: {
? PullPolicy.ALWAYS
: PullPolicy.IF_NOT_PRESENT;
}

/**
* Resolve the effective disabled state from the `enabled` and `disabled`
* fields on a plugin spec. Precedence rules (per RHIDP-11983):
Comment thread
gustavolira marked this conversation as resolved.
*
* 1. When only `enabled` is set → `disabled = !enabled`.
* 2. When only `disabled` is set → use it directly (backward compat).
* 3. When both are set → `enabled` wins and a warning is emitted
* via the optional `warn` callback.
* 4. When neither is set → default to `false` (not disabled).
*
* Non-boolean values (e.g. the quoted string `enabled: 'false'` or
* `enabled: null`) are treated as unset and a warning is emitted,
* preventing JS truthiness from silently flipping activation state.
*
* The `warn` callback receives the warning message string. Pass `log` or
* leave it out for silent resolution (unit tests, hashing).
*/
export function isPluginDisabled(
Comment thread
gustavolira marked this conversation as resolved.
plugin: { package: string; disabled?: boolean; enabled?: boolean },
warn?: (msg: string) => void,
): boolean {
const hasEnabled = typeof plugin.enabled === 'boolean';
const hasDisabled = typeof plugin.disabled === 'boolean';

if (plugin.enabled !== undefined && !hasEnabled) {
warn?.(
`WARNING: Plugin ${plugin.package} has non-boolean 'enabled: ${String(plugin.enabled)}'. ` +
`Expected true or false; ignoring the field.`,
);
}
if (plugin.disabled !== undefined && !hasDisabled) {
warn?.(
`WARNING: Plugin ${plugin.package} has non-boolean 'disabled: ${String(plugin.disabled)}'. ` +
`Expected true or false; ignoring the field.`,
);
}

if (hasEnabled && hasDisabled) {
warn?.(
`WARNING: Plugin ${plugin.package} specifies both 'enabled' and 'disabled'. ` +
Comment thread
gustavolira marked this conversation as resolved.
`The 'enabled' field takes precedence; please use only 'enabled'.`,
);
return !plugin.enabled;
}
if (hasEnabled) return !plugin.enabled;
if (hasDisabled) return plugin.disabled === true;
return false;
Comment thread
gustavolira marked this conversation as resolved.
}
Loading