feat(must-gather): expose known must-gather env vars as explicit fields in the values file by rm3l · Pull Request #347 · redhat-developer/rhdh-chart

rm3l · 2026-04-01T12:52:33Z

Description of the change

This makes it easier to customize.

Which issue(s) does this PR fix or relate to

—

How to test changes / Special notes to the reviewer

Checklist

For each Chart updated, version bumped in the corresponding Chart.yaml according to Semantic Versioning.
For each Chart updated, variables are documented in the values.yaml and added to the corresponding README.md. The pre-commit utility can be used to generate the necessary content. Run pre-commit run --all-files to run the hooks and then push any resulting changes. The pre-commit Workflow will enforce this and warn you if needed.
JSON Schema template updated and re-generated the raw schema via the pre-commit hook.
Tests pass using the Chart Testing tool and the ct lint command.
If you updated the orchestrator-infra chart, make sure the versions of the Knative CRDs are aligned with the versions of the CRDs installed by the OpenShift Serverless operators declared in the values.yaml file. See Installing Knative Eventing and Knative Serving CRDs for more details.

rhdh-qodo-merge · 2026-04-01T12:52:41Z

Code Review by Qodo

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

1. Heap dump key breaking 🐞 Bug ☼ Reliability

Description

The chart removes gather.withHeapDumps and replaces it with gather.heapDump.enabled, but
gather has additionalProperties: false in the values schema, so any existing user values files
that still set gather.withHeapDumps will fail schema validation and won’t enable heap dumps after
upgrade.

Code

charts/must-gather/values.schema.tmpl.json[R208-244]

+                "heapDump": {
+                    "title": "Heap dump collection configuration.",
+                    "type": "object",
+                    "additionalProperties": false,
+                    "properties": {
+                        "enabled": {
+                            "title": "Enable collection of heap dumps (disabled by default).",
+                            "type": "boolean",
+                            "default": false
+                        },
+                        "method": {
+                            "title": "Method for heap dump collection: 'inspector' (default) or 'sigusr2'.",
+                            "type": "string",
+                            "default": ""
+                        },
+                        "timeout": {
+                            "title": "Timeout in seconds for heap dump collection (default: 600).",
+                            "type": "string",
+                            "default": ""
+                        },
+                        "bufferSize": {
+                            "title": "WebSocket buffer size in bytes for inspector method (default: 16777216 = 16MB).",
+                            "type": "string",
+                            "default": ""
+                        },
+                        "remoteDir": {
+                            "title": "Directory in container where heap dumps are written for SIGUSR2 method (default: /tmp).",
+                            "type": "string",
+                            "default": ""
+                        },
+                        "instances": {
+                            "title": "Filter for specific heap dump instances (comma-separated pod names or patterns).",
+                            "type": "string",
+                            "default": ""
+                        }
+                    }
                },

Evidence

gather is schema-locked (additionalProperties: false) and no longer defines withHeapDumps, so
legacy values containing that key will be rejected by schema validation. The default values and
templates also no longer reference withHeapDumps, so even if schema validation is bypassed,
upgrades will silently stop enabling heap dumps unless users migrate to the new key.

charts/must-gather/values.schema.tmpl.json[169-244]
charts/must-gather/values.yaml[66-95]
charts/must-gather/templates/deployment.yaml[92-107]

Agent prompt

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

### Issue description
`gather.withHeapDumps` was removed and replaced by `gather.heapDump.enabled`. Because the `gather` schema is strict (`additionalProperties: false`), existing user values files that still set `gather.withHeapDumps` will fail schema validation (and heap dumps will not be enabled in templates).

### Issue Context
This is a breaking values interface change. If you intend to keep it non-breaking for at least one release, you need to accept the old key in schema and map it to the new behavior in templates.

### Fix Focus Areas
- charts/must-gather/values.schema.tmpl.json[169-244]
- charts/must-gather/values.schema.json[957-1031]
- charts/must-gather/templates/deployment.yaml[92-107]
- charts/must-gather/templates/rbac.yaml[47-53]
- charts/must-gather/templates/clusterrbac.yaml[60-66]
- charts/must-gather/templates/NOTES.txt[24-30]

### Suggested implementation direction
- Re-introduce `gather.withHeapDumps` in the schema (mark as `deprecated: true` if you want), so existing values pass validation.
- In templates, compute an effective heap-dump enablement flag like:
 - `$heapDumpEnabled := or .Values.gather.heapDump.enabled .Values.gather.withHeapDumps`
 and use `$heapDumpEnabled` everywhere instead of `.Values.gather.heapDump.enabled`.
- Optionally add a NOTES.txt message when the legacy key is used to guide migration.

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

2. Unvalidated heapDump.method 🐞 Bug ≡ Correctness

Description

gather.heapDump.method is documented as only allowing "inspector" or "sigusr2", but the JSON
schema accepts any string, so typos/misconfigurations will pass helm lint/schema validation and
only fail later at runtime.

Code

charts/must-gather/values.schema.tmpl.json[R218-242]

+                        "method": {
+                            "title": "Method for heap dump collection: 'inspector' (default) or 'sigusr2'.",
+                            "type": "string",
+                            "default": ""
+                        },
+                        "timeout": {
+                            "title": "Timeout in seconds for heap dump collection (default: 600).",
+                            "type": "string",
+                            "default": ""
+                        },
+                        "bufferSize": {
+                            "title": "WebSocket buffer size in bytes for inspector method (default: 16777216 = 16MB).",
+                            "type": "string",
+                            "default": ""
+                        },
+                        "remoteDir": {
+                            "title": "Directory in container where heap dumps are written for SIGUSR2 method (default: /tmp).",
+                            "type": "string",
+                            "default": ""
+                        },
+                        "instances": {
+                            "title": "Filter for specific heap dump instances (comma-separated pod names or patterns).",
+                            "type": "string",
+                            "default": ""
+                        }

Evidence

The values schema defines heapDump.method as a plain string with no enum constraint, while the
chart README explicitly restricts valid values to two options. This creates a validation gap where
invalid configuration is not caught early.

charts/must-gather/values.schema.tmpl.json[208-244]
charts/must-gather/README.md[112-118]

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 schema allows any string for `gather.heapDump.method`, but documentation restricts it to two valid values. This allows invalid values to pass schema validation and break at runtime.

### Issue Context
This chart already uses enums for other fields (e.g., `gather.logLevel`). Align heapDump.method with the documented contract.

### Fix Focus Areas
- charts/must-gather/values.schema.tmpl.json[208-244]
- charts/must-gather/values.schema.json[960-996]

### Suggested implementation direction
- Add an `enum` constraint for `method`, e.g.:
 - `"enum": ["", "inspector", "sigusr2"]`
- (Optional) In templates, add a defensive `fail` if `heapDump.enabled` is true and `method` is set to something outside the allowed set (in case schema validation is bypassed).

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

ⓘ The new review experience is currently in Beta. Learn more

Previous review results

Review updated until commit 4a97bcb

Results up to commit c73d18a

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

1. Heap dump key breaking 🐞 Bug ☼ Reliability

Description

The chart removes gather.withHeapDumps and replaces it with gather.heapDump.enabled, but
gather has additionalProperties: false in the values schema, so any existing user values files
that still set gather.withHeapDumps will fail schema validation and won’t enable heap dumps after
upgrade.

Code

charts/must-gather/values.schema.tmpl.json[R208-244]

+                "heapDump": {
+                    "title": "Heap dump collection configuration.",
+                    "type": "object",
+                    "additionalProperties": false,
+                    "properties": {
+                        "enabled": {
+                            "title": "Enable collection of heap dumps (disabled by default).",
+                            "type": "boolean",
+                            "default": false
+                        },
+                        "method": {
+                            "title": "Method for heap dump collection: 'inspector' (default) or 'sigusr2'.",
+                            "type": "string",
+                            "default": ""
+                        },
+                        "timeout": {
+                            "title": "Timeout in seconds for heap dump collection (default: 600).",
+                            "type": "string",
+                            "default": ""
+                        },
+                        "bufferSize": {
+                            "title": "WebSocket buffer size in bytes for inspector method (default: 16777216 = 16MB).",
+                            "type": "string",
+                            "default": ""
+                        },
+                        "remoteDir": {
+                            "title": "Directory in container where heap dumps are written for SIGUSR2 method (default: /tmp).",
+                            "type": "string",
+                            "default": ""
+                        },
+                        "instances": {
+                            "title": "Filter for specific heap dump instances (comma-separated pod names or patterns).",
+                            "type": "string",
+                            "default": ""
+                        }
+                    }
                },

Evidence

gather is schema-locked (additionalProperties: false) and no longer defines withHeapDumps, so
legacy values containing that key will be rejected by schema validation. The default values and
templates also no longer reference withHeapDumps, so even if schema validation is bypassed,
upgrades will silently stop enabling heap dumps unless users migrate to the new key.

charts/must-gather/values.schema.tmpl.json[169-244]
charts/must-gather/values.yaml[66-95]
charts/must-gather/templates/deployment.yaml[92-107]

Agent prompt

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

### Issue description
`gather.withHeapDumps` was removed and replaced by `gather.heapDump.enabled`. Because the `gather` schema is strict (`additionalProperties: false`), existing user values files that still set `gather.withHeapDumps` will fail schema validation (and heap dumps will not be enabled in templates).

### Issue Context
This is a breaking values interface change. If you intend to keep it non-breaking for at least one release, you need to accept the old key in schema and map it to the new behavior in templates.

### Fix Focus Areas
- charts/must-gather/values.schema.tmpl.json[169-244]
- charts/must-gather/values.schema.json[957-1031]
- charts/must-gather/templates/deployment.yaml[92-107]
- charts/must-gather/templates/rbac.yaml[47-53]
- charts/must-gather/templates/clusterrbac.yaml[60-66]
- charts/must-gather/templates/NOTES.txt[24-30]

### Suggested implementation direction
- Re-introduce `gather.withHeapDumps` in the schema (mark as `deprecated: true` if you want), so existing values pass validation.
- In templates, compute an effective heap-dump enablement flag like:
 - `$heapDumpEnabled := or .Values.gather.heapDump.enabled .Values.gather.withHeapDumps`
 and use `$heapDumpEnabled` everywhere instead of `.Values.gather.heapDump.enabled`.
- Optionally add a NOTES.txt message when the legacy key is used to guide migration.

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

2. Unvalidated heapDump.method 🐞 Bug ≡ Correctness

Description

gather.heapDump.method is documented as only allowing "inspector" or "sigusr2", but the JSON
schema accepts any string, so typos/misconfigurations will pass helm lint/schema validation and
only fail later at runtime.

Code

charts/must-gather/values.schema.tmpl.json[R218-242]

+                        "method": {
+                            "title": "Method for heap dump collection: 'inspector' (default) or 'sigusr2'.",
+                            "type": "string",
+                            "default": ""
+                        },
+                        "timeout": {
+                            "title": "Timeout in seconds for heap dump collection (default: 600).",
+                            "type": "string",
+                            "default": ""
+                        },
+                        "bufferSize": {
+                            "title": "WebSocket buffer size in bytes for inspector method (default: 16777216 = 16MB).",
+                            "type": "string",
+                            "default": ""
+                        },
+                        "remoteDir": {
+                            "title": "Directory in container where heap dumps are written for SIGUSR2 method (default: /tmp).",
+                            "type": "string",
+                            "default": ""
+                        },
+                        "instances": {
+                            "title": "Filter for specific heap dump instances (comma-separated pod names or patterns).",
+                            "type": "string",
+                            "default": ""
+                        }

Evidence

The values schema defines heapDump.method as a plain string with no enum constraint, while the
chart README explicitly restricts valid values to two options. This creates a validation gap where
invalid configuration is not caught early.

charts/must-gather/values.schema.tmpl.json[208-244]
charts/must-gather/README.md[112-118]

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 schema allows any string for `gather.heapDump.method`, but documentation restricts it to two valid values. This allows invalid values to pass schema validation and break at runtime.

### Issue Context
This chart already uses enums for other fields (e.g., `gather.logLevel`). Align heapDump.method with the documented contract.

### Fix Focus Areas
- charts/must-gather/values.schema.tmpl.json[208-244]
- charts/must-gather/values.schema.json[960-996]

### Suggested implementation direction
- Add an `enum` constraint for `method`, e.g.:
 - `"enum": ["", "inspector", "sigusr2"]`
- (Optional) In templates, add a defensive `fail` if `heapDump.enabled` is true and `method` is set to something outside the allowed set (in case schema validation is bypassed).

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

rhdh-qodo-merge · 2026-04-01T12:55:21Z

Review Summary by Qodo

Expose heap dump environment variables as explicit configuration fields

✨ Enhancement

Walkthroughs

Description

• Refactor heap dump configuration from boolean to structured object
• Add explicit fields for heap dump method, timeout, buffer size, and instances
• Enable granular customization of heap dump collection behavior
• Update chart version to 0.3.0 and regenerate schema documentation

Diagram

flowchart LR
  A["withHeapDumps<br/>boolean flag"] -->|"refactored to"| B["heapDump object<br/>with sub-fields"]
  B --> C["enabled"]
  B --> D["method"]
  B --> E["timeout"]
  B --> F["bufferSize"]
  B --> G["remoteDir"]
  B --> H["instances"]
  C -->|"maps to"| I["HEAP_DUMP env vars<br/>in deployment"]
  D -->|"maps to"| I
  E -->|"maps to"| I
  F -->|"maps to"| I
  G -->|"maps to"| I
  H -->|"maps to"| I

File Changes

1. charts/must-gather/Chart.yaml ⚙️ Configuration changes +1/-1

Bump chart version to 0.3.0

charts/must-gather/Chart.yaml

2. charts/must-gather/README.md 📝 Documentation +10/-3

Update version and document heap dump fields

charts/must-gather/README.md

3. charts/must-gather/templates/NOTES.txt ✨ Enhancement +1/-1

Update heap dump condition reference

charts/must-gather/templates/NOTES.txt

View more (6)

4. charts/must-gather/templates/clusterrbac.yaml ✨ Enhancement +1/-1

Update heap dump condition in RBAC rules

charts/must-gather/templates/clusterrbac.yaml

5. charts/must-gather/templates/rbac.yaml ✨ Enhancement +1/-1

Update heap dump condition in RBAC rules

charts/must-gather/templates/rbac.yaml

6. charts/must-gather/templates/deployment.yaml ✨ Enhancement +22/-2

Add heap dump environment variables to deployment

charts/must-gather/templates/deployment.yaml

7. charts/must-gather/values.schema.json 📝 Documentation +37/-5

Add heapDump object schema with sub-properties

charts/must-gather/values.schema.json

8. charts/must-gather/values.schema.tmpl.json 📝 Documentation +36/-4

Add heapDump object schema template definition

charts/must-gather/values.schema.tmpl.json

9. charts/must-gather/values.yaml ✨ Enhancement +15/-1

Replace withHeapDumps with heapDump object structure

charts/must-gather/values.yaml

…ds in the values file This makes it easier to customize

openshift-ci · 2026-04-01T13:03:24Z

New changes are detected. LGTM label has been removed.

sonarqubecloud · 2026-04-01T13:03:45Z

Quality Gate passed

Issues
0 New issues
0 Accepted issues

Measures
0 Security Hotspots
0.0% Coverage on New Code
0.0% Duplication on New Code

See analysis details on SonarQube Cloud

rm3l · 2026-04-01T13:04:13Z

/agentic_review

rhdh-qodo-merge · 2026-04-01T13:04:21Z

Persistent review updated to latest commit 4a97bcb

openshift-ci bot requested review from Fortune-Ndlovu and zdrapela April 1, 2026 12:52

rhdh-qodo-merge bot added documentation Improvements or additions to documentation enhancement New feature or request labels Apr 1, 2026

rm3l added the lgtm label Apr 1, 2026

feat(must-gather): expose known must-gather env vars as explicit fiel…

4a97bcb

…ds in the values file This makes it easier to customize

rm3l force-pushed the feat/expose_known_must_gather_env_vars_as_values_fields branch from c73d18a to 4a97bcb Compare April 1, 2026 13:03

openshift-ci bot removed the lgtm label Apr 1, 2026

rm3l added the lgtm label Apr 1, 2026

openshift-merge-bot bot merged commit 9ab6776 into redhat-developer:main Apr 1, 2026
6 checks passed

rm3l deleted the feat/expose_known_must_gather_env_vars_as_values_fields branch April 1, 2026 13:39

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(must-gather): expose known must-gather env vars as explicit fields in the values file#347

feat(must-gather): expose known must-gather env vars as explicit fields in the values file#347
openshift-merge-bot[bot] merged 1 commit intoredhat-developer:mainfrom
rm3l:feat/expose_known_must_gather_env_vars_as_values_fields

rm3l commented Apr 1, 2026

Uh oh!

rhdh-qodo-merge bot commented Apr 1, 2026 •

edited

Loading

Uh oh!

rhdh-qodo-merge bot commented Apr 1, 2026

Uh oh!

openshift-ci bot commented Apr 1, 2026

Uh oh!

sonarqubecloud bot commented Apr 1, 2026

Uh oh!

rm3l commented Apr 1, 2026

Uh oh!

rhdh-qodo-merge bot commented Apr 1, 2026 •

edited

Loading

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

Conversation

rm3l commented Apr 1, 2026

Description of the change

Which issue(s) does this PR fix or relate to

How to test changes / Special notes to the reviewer

Checklist

Uh oh!

rhdh-qodo-merge bot commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Review by Qodo

Previous review results

Uh oh!

rhdh-qodo-merge bot commented Apr 1, 2026

Review Summary by Qodo

Walkthroughs

File Changes

Uh oh!

openshift-ci bot commented Apr 1, 2026

Uh oh!

sonarqubecloud bot commented Apr 1, 2026

Quality Gate passed

Uh oh!

rm3l commented Apr 1, 2026

Uh oh!

rhdh-qodo-merge bot commented Apr 1, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

1 participant

rhdh-qodo-merge bot commented Apr 1, 2026 •

edited

Loading

rhdh-qodo-merge bot commented Apr 1, 2026 •

edited

Loading