Skip to content

feat: add enabled field in DP config - #4937

Merged
openshift-merge-bot[bot] merged 5 commits into
redhat-developer:mainfrom
hopehadfield:dis-2-enb
Jun 9, 2026
Merged

feat: add enabled field in DP config#4937
openshift-merge-bot[bot] merged 5 commits into
redhat-developer:mainfrom
hopehadfield:dis-2-enb

Conversation

@hopehadfield

@hopehadfield hopehadfield commented Jun 9, 2026

Copy link
Copy Markdown
Member

Description

Adds the enabled field with backwards compatibility

See redhat-developer/rhdh-plugin-export-overlays#2577 for relevant change in DPDY generation

Which issue(s) does this PR fix

PR acceptance criteria

Please make sure that the following steps are complete:

  • GitHub Actions are completed and successful
  • Unit Tests are updated and passing
  • E2E Tests are updated and passing
  • Documentation is updated if necessary (requirement for new features)
  • Add a screenshot if the change is UX/UI related

How to test changes / Special notes to the reviewer

@rhdh-qodo-merge

rhdh-qodo-merge Bot commented Jun 9, 2026

Copy link
Copy Markdown

Code Review by Qodo

🐞 Bugs (2) 📘 Rule violations (0) 📎 Requirement gaps (0)

Context used
✅ Tickets: RHIDP-11988

Grey Divider


Action required

1. Enabled ignored in bundle 🐞 Bug ≡ Correctness
Description
The PR updates the TypeScript sources to use isPluginDisabled() (supporting enabled), but the
committed runtime bundle still checks plugin.disabled only, so enabled: false/true will be
ignored in the init-container unless the bundle is rebuilt and committed. This breaks the PR’s
intended behavior in real deployments because the wrapper executes the bundled .cjs entrypoint.
Code

scripts/install-dynamic-plugins/src/index.ts[R285-287]

+    if (isPluginDisabled(plugin, log)) {
      log(`\n======= Skipping disabled plugin ${plugin.package}`);
      continue;
Relevance

⭐⭐⭐ High

Team relies on committed dist .cjs bundle (CI freshness check); mismatched src/dist would be fixed.

PR-#4574

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
The repo’s runtime wrapper and documentation indicate the bundled .cjs is what runs in the
container, but the committed bundle still contains legacy .disabled checks (no enabled
handling), while the updated source now routes disablement through isPluginDisabled(). This
mismatch means the enabled feature won’t work until the bundle is regenerated and committed.

scripts/install-dynamic-plugins/install-dynamic-plugins.sh[18-18]
scripts/install-dynamic-plugins/README.md[15-21]
scripts/install-dynamic-plugins/README.md[89-106]
scripts/install-dynamic-plugins/src/index.ts[280-288]
scripts/install-dynamic-plugins/dist/install-dynamic-plugins.cjs[151-176]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
The init-container runs the bundled CommonJS entrypoint, but the committed bundle still uses legacy `disabled` checks and does not implement the new `enabled` precedence logic introduced in `src/`. This means production behavior won’t match the updated source code or documentation.

### Issue Context
- The wrapper script executes `install-dynamic-plugins.cjs`.
- Repo docs state `dist/install-dynamic-plugins.cjs` is committed and CI verifies it’s up-to-date.
- Current `dist/install-dynamic-plugins.cjs` still checks `.disabled` and performs pre-merge disabled handling via `.disabled === true`.

### Fix
1. Run the build for `scripts/install-dynamic-plugins` (e.g., `npm run build`) to regenerate `dist/install-dynamic-plugins.cjs` from updated sources.
2. Commit the regenerated `dist/install-dynamic-plugins.cjs`.
3. (Optional) Add/confirm a CI check that fails if `npm run build` produces a diff.

### Fix Focus Areas
- scripts/install-dynamic-plugins/src/index.ts[280-301]
- scripts/install-dynamic-plugins/src/merger.ts[374-509]
- scripts/install-dynamic-plugins/src/types.ts[100-130]
- scripts/install-dynamic-plugins/dist/install-dynamic-plugins.cjs[151-176]
- scripts/install-dynamic-plugins/README.md[89-106]
- scripts/install-dynamic-plugins/install-dynamic-plugins.sh[18-18]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools



Informational

2. Non-boolean enabled mishandled 🐞 Bug ☼ Reliability
Description
isPluginDisabled treats any non-undefined enabled/disabled value as authoritative and
applies JS truthiness (e.g., return !plugin.enabled), so non-boolean YAML values (like `enabled:
'false' or enabled:null`) can silently flip activation state. Because YAML is parsed into
untyped objects and mergePlugin validates only package, these mis-typed values are not rejected
and can cause plugins to be installed/skipped contrary to intent.
Code

scripts/install-dynamic-plugins/src/types.ts[R113-129]

+export function isPluginDisabled(
+  plugin: { package: string; disabled?: boolean; enabled?: boolean },
+  warn?: (msg: string) => void,
+): boolean {
+  const hasEnabled = plugin.enabled !== undefined;
+  const hasDisabled = plugin.disabled !== undefined;
+
+  if (hasEnabled && hasDisabled) {
+    warn?.(
+      `WARNING: Plugin ${plugin.package} specifies both 'enabled' and 'disabled'. ` +
+        `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;
Relevance

⭐ Low

Repo history shows runtime validation/defensive checks often rejected as unnecessary noise (e.g.,
translation shape validation).

PR-#4519

ⓘ Recommendations generated based on similar findings in past PRs

Evidence
mergePluginsFromFile parses YAML into untyped objects and mergePlugin only validates package,
so enabled/disabled can be non-boolean at runtime. isPluginDisabled then uses !== undefined
and !plugin.enabled, which will mis-handle strings/null via JS truthiness rather than rejecting
invalid config.

scripts/install-dynamic-plugins/src/types.ts[113-130]
scripts/install-dynamic-plugins/src/merger.ts[87-125]

Agent prompt
The issue below was found during a code review. Follow the provided context and guidance below and implement a solution

### Issue description
`isPluginDisabled()` currently resolves state using `!== undefined` checks and JS truthiness, which can misinterpret non-boolean values coming from YAML parsing (e.g., quoted strings or `null`). This can silently lead to wrong plugin enable/disable behavior.

### Issue Context
- Config is loaded via `yaml.parse(...)` into plain JS objects.
- `mergePlugin()` validates only that `package` is a string.
- `isPluginDisabled()` assumes `enabled`/`disabled` are booleans but doesn’t enforce it.

### Fix
- Add runtime validation when ingesting plugin specs (recommended: inside `mergePlugin()` or immediately after parsing `plugins[]`):
 - If `enabled` is present, require `typeof enabled === 'boolean'`.
 - If `disabled` is present, require `typeof disabled === 'boolean'`.
 - Treat `null` as invalid (throw) rather than as “present”.
- Update/add unit tests to cover cases like `enabled: 'false'`, `disabled: 'true'`, and `enabled: null`.

### Fix Focus Areas
- scripts/install-dynamic-plugins/src/types.ts[113-130]
- scripts/install-dynamic-plugins/src/merger.ts[87-125]
- scripts/install-dynamic-plugins/__tests__/types.test.ts[35-80]

ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools


Grey Divider

Qodo Logo

@rhdh-qodo-merge

Copy link
Copy Markdown

PR Summary by Qodo

Add enabled flag for dynamic plugin config with backward compatibility
✨ Enhancement 🧪 Tests 📝 Documentation 🕐 20-40 Minutes

Grey Divider

Walkthroughs

Description
• Introduces enabled as the preferred plugin activation flag, keeping disabled supported.
• Centralizes precedence rules (enabled wins) and emits warnings when both fields are set.
• Updates installer/merger logic, unit tests, and docs to use enabled consistently.
Diagram
graph TD
  Cfg[("Dynamic plugins cfg")] --> Merger["merger.ts"] --> Types["types.ts: isPluginDisabled"] --> Oci["installer-oci.ts"] & Npm["installer-npm.ts"]
  Types --> Index["index.ts categorize"]
  Docs["Docs examples"] --> Cfg
  subgraph Legend
    direction LR
    _cfg[("Config")] ~~~ _code["Code module"] ~~~ _doc["Documentation"]
  end
Loading
High-Level Assessment

The following are alternative approaches to this PR:

1. Normalization step at config-load time
  • ➕ Converts all specs to a single canonical field (enabled) once, reducing repeated logic
  • ➕ Simplifies downstream code to check only one property
  • ➖ Requires a clear ownership point in the load pipeline and careful handling for hashing/logging
  • ➖ May be harder to keep warnings/source-file context accurate
2. Hard deprecation (remove `disabled` support sooner)
  • ➕ Less long-term maintenance and fewer ambiguous configurations
  • ➕ Avoids precedence edge cases entirely
  • ➖ Breaks existing user configs; not aligned with backward-compat requirement
  • ➖ Requires coordinated rollout and migration tooling/communications
3. Schema validation (fail on both fields instead of warning)
  • ➕ Prevents ambiguous configs and enforces clean usage
  • ➕ Makes behavior explicit and predictable
  • ➖ More disruptive for existing configs that currently set both
  • ➖ May block upgrades where leniency is desired

Recommendation: The chosen approach (introduce enabled, keep disabled, centralize resolution in isPluginDisabled, and warn on ambiguous config) is the best fit for a backward-compatible rollout. A future follow-up could add an early normalization/validation pass once telemetry indicates disabled usage is sufficiently low.

Grey Divider

File Changes

Enhancement (5)
index.ts Use 'isPluginDisabled' when categorizing/skipping plugins +2/-1

Use 'isPluginDisabled' when categorizing/skipping plugins

• Replaces direct 'plugin.disabled' checks with 'isPluginDisabled(plugin, log)' so both 'enabled' and legacy 'disabled' are interpreted consistently and warnings can be emitted.

scripts/install-dynamic-plugins/src/index.ts


installer-npm.ts Honor 'enabled' when deciding to install NPM plugins +2/-2

Honor 'enabled' when deciding to install NPM plugins

• Switches the skip-install condition from 'plugin.disabled' to 'isPluginDisabled(plugin, log)' for consistent behavior and logging.

scripts/install-dynamic-plugins/src/installer-npm.ts


installer-oci.ts Honor 'enabled' when deciding to install OCI plugins +2/-1

Honor 'enabled' when deciding to install OCI plugins

• Switches the skip-install condition from 'plugin.disabled' to 'isPluginDisabled(plugin, log)' for consistent behavior and logging.

scripts/install-dynamic-plugins/src/installer-oci.ts


merger.ts Apply 'enabled'/'disabled' resolution in OCI merge/filter logic +3/-2

Apply 'enabled'/'disabled' resolution in OCI merge/filter logic

• Uses 'isPluginDisabled' to compute disabled state when processing OCI entries and filtering invalid/disabled OCI plugins, ensuring 'enabled' is respected consistently across merge operations.

scripts/install-dynamic-plugins/src/merger.ts


types.ts Add 'enabled' to plugin spec and implement 'isPluginDisabled' helper +42/-0

Add 'enabled' to plugin spec and implement 'isPluginDisabled' helper

• Extends 'PluginSpec' with an 'enabled' field and marks 'disabled' as deprecated. Adds 'isPluginDisabled' with documented precedence rules and optional warning emission when both flags are provided.

scripts/install-dynamic-plugins/src/types.ts


Tests (3)
merger-pre-merge.test.ts Add pre-merge behavior tests for 'enabled' +49/-0

Add pre-merge behavior tests for 'enabled'

• Adds coverage ensuring 'enabled: false' disables entries, 'enabled: true' can re-enable an include, and 'enabled' takes precedence over 'disabled' (with a warning). Also adds a filter test for invalid OCI entries gated by 'enabled: false'.

scripts/install-dynamic-plugins/tests/merger-pre-merge.test.ts


merger.test.ts Test merge override behavior using 'enabled' +18/-0

Test merge override behavior using 'enabled'

• Adds tests verifying that merges can override plugin state via 'enabled', including scenarios where 'enabled' overrides legacy 'disabled' across levels.

scripts/install-dynamic-plugins/tests/merger.test.ts


types.test.ts Add unit tests for 'isPluginDisabled' precedence rules +49/-1

Add unit tests for 'isPluginDisabled' precedence rules

• Introduces a dedicated test suite for 'isPluginDisabled', covering default behavior, enabled/disabled interpretations, precedence when both are set, and optional warning callback behavior.

scripts/install-dynamic-plugins/tests/types.test.ts


Documentation (4)
frontend-plugin-wiring.md Update frontend wiring examples to use 'enabled' +12/-12

Update frontend wiring examples to use 'enabled'

• Replaces 'disabled: false' with 'enabled: true' across multiple YAML examples so docs match the new preferred configuration flag.

docs/dynamic-plugins/frontend-plugin-wiring.md


index.md Switch dynamic plugin index examples to 'enabled' +4/-4

Switch dynamic plugin index examples to 'enabled'

• Updates example dynamic plugin entries to use 'enabled: true' for both frontend and backend plugin packages.

docs/dynamic-plugins/index.md


installing-plugins.md Document 'enabled' semantics and precedence over 'disabled' +18/-18

Document 'enabled' semantics and precedence over 'disabled'

• Introduces 'enabled' as the primary activation flag, explicitly documents backward compatibility with 'disabled', and clarifies precedence when both are present. Updates multiple snippets and surrounding text accordingly.

docs/dynamic-plugins/installing-plugins.md


index.md Update telemetry enable/disable guidance to use 'enabled' +15/-15

Update telemetry enable/disable guidance to use 'enabled'

• Rewrites telemetry instructions and YAML snippets to use 'enabled: true/false' instead of 'plugins.disabled', aligning with the new config field.

docs/index.md


Grey Divider

Qodo Logo

Signed-off-by: Hope Hadfield <hhadfiel@redhat.com>
Signed-off-by: Hope Hadfield <hhadfiel@redhat.com>
@codecov

codecov Bot commented Jun 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 55.25%. Comparing base (4821b1d) to head (4e65644).

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #4937      +/-   ##
==========================================
- Coverage   55.82%   55.25%   -0.58%     
==========================================
  Files         121      109      -12     
  Lines        2350     2132     -218     
  Branches      562      536      -26     
==========================================
- Hits         1312     1178     -134     
+ Misses       1032      954      -78     
+ Partials        6        0       -6     
Flag Coverage Δ
rhdh 55.25% <ø> (-0.58%) ⬇️

Continue to review full report in Codecov by Harness.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 4821b1d...4e65644. Read the comment docs.

🚀 New features to boost your workflow:
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

Signed-off-by: Hope Hadfield <hhadfiel@redhat.com>
@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

The container image build workflow finished with status: cancelled.

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

The container image build workflow finished with status: failure.

Signed-off-by: Hope Hadfield <hhadfiel@redhat.com>
Comment thread scripts/install-dynamic-plugins/src/types.ts
Signed-off-by: Hope Hadfield <hhadfiel@redhat.com>
@sonarqubecloud

sonarqubecloud Bot commented Jun 9, 2026

Copy link
Copy Markdown

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Image was built and published successfully. It is available at:

@Zaperex Zaperex left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

/lgtm

@github-actions

github-actions Bot commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

Image was built and published successfully. It is available at:

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants