Add Helm chart deployment support to plugin framework#320
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
WalkthroughAdds declarative Helm chart deployment to the plugin lifecycle: introduces a Changes
Sequence Diagram(s)sequenceDiagram
actor Orchestrator
participant PostOps as post-operators.yaml
participant HelmTask as helm_deploy.yaml
participant HelmRepo as Helm/Kubernetes
participant DeployTask as deploy.yaml
Orchestrator->>PostOps: include post-operators (if present)
PostOps->>Orchestrator: complete
Orchestrator->>HelmTask: for each entry in plugin.helm
loop per helm_chart
HelmTask->>HelmRepo: add/update repo (if repo set)
HelmTask->>HelmTask: render/copy values (valuesTemplate or valuesFile)
HelmTask->>HelmRepo: helm upgrade --install (with flags, timeout)
HelmRepo-->>HelmTask: install result / logs
HelmTask->>HelmRepo: helm status (verify)
HelmRepo-->>HelmTask: status output
end
Orchestrator->>DeployTask: include deploy task
DeployTask->>HelmRepo: run remaining deployment steps
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~22 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Review rate limit: 0/1 reviews remaining, refill in 60 minutes.Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (1)
schemas/plugin.yaml (1)
100-133: ⚡ Quick winDisallow ambiguous
valuesTemplate+valuesFilecombinations.Allowing both fields at once creates unclear config intent (runtime currently gives precedence to template). Add a schema rule to reject both being set together.
Suggested fix
helm_chart: type: object additionalProperties: false + not: + allOf: + - required: [valuesTemplate] + - required: [valuesFile] properties: chart: type: string🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@schemas/plugin.yaml` around lines 100 - 133, The helm_chart schema currently allows both valuesTemplate and valuesFile to be set (ambiguous), so update the helm_chart object schema to reject configurations that include both properties; add a JSON Schema constraint (e.g., a "not" clause that disallows an object with required ["valuesTemplate","valuesFile"]) inside the helm_chart definition so any instance containing both valuesTemplate and valuesFile will fail validation.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@docs/PLUGIN_ARCHITECTURE.md`:
- Around line 91-101: Clarify that tasks/early-validate.yaml is executed during
the earlier validation phase (not by deploy_plugin.yaml) by adding a short
parenthetical note after "tasks/early-validate.yaml" saying it runs in the
pre-deployment validation phase (no cluster access) and is invoked before
deploy_plugin.yaml; update the same clarification near the duplicate mention at
lines 106-107 so both places consistently specify that early-validate.yaml is
run by the validation stage, not the deployment task.
In `@playbooks/tasks/deploy_plugin.yaml`:
- Around line 156-164: The include of "{{ plugin_dir
}}/tasks/post-operators.yaml" is only gated by file existence and runs even when
operators are not meant to be installed; update the when condition on the
include to require both the file to exist and that operator installation is
enabled (e.g., installOperators is true OR the operators list is non-empty) so
post-operators run only when operators are being installed; modify the when to
combine lookup('ansible.builtin.fileglob', plugin_dir ~
'/tasks/post-operators.yaml') | length > 0 with checks against installOperators
and/or operators (refer to the include task that references plugin_dir and
post-operators.yaml).
In `@playbooks/tasks/helm_deploy.yaml`:
- Around line 7-10: The task "Helm | Set chart path for {{ helm_chart.release
}}" currently sets _helm_chart_ref using helm_chart.chart as-is which can be a
relative path resolved against the wrong CWD; update the set_fact to normalize
helm_chart.chart relative to plugin_dir when it's provided and not an absolute
path: if helm_chart.chart is defined and does not start with '/' (or a Windows
drive letter) then set _helm_chart_ref to plugin_dir ~ '/' ~ helm_chart.chart,
otherwise use helm_chart.chart as given; if helm_chart.chart is not defined fall
back to plugin_dir ~ '/charts/' ~ helm_chart.release. Ensure this logic is
applied where _helm_chart_ref is assigned so references to helm_chart.chart,
plugin_dir and _helm_chart_ref are used.
- Around line 52-66: The helm task's pipeline currently uses "helm ... | tee {{
_helm_log }}" which yields tee's exit code so r_helm_install incorrectly sees
success; update the ansible.builtin.shell invocation in
playbooks/tasks/helm_deploy.yaml to enable bash pipefail by prefixing the
command with "set -o pipefail" (so the pipeline fails if helm fails) and set
executable: /bin/bash so pipefail is supported; keep the existing helm upgrade
invocation, _helm_values_arg and _helm_log but ensure the register target
remains r_helm_install and the retry/until logic will then behave correctly.
---
Nitpick comments:
In `@schemas/plugin.yaml`:
- Around line 100-133: The helm_chart schema currently allows both
valuesTemplate and valuesFile to be set (ambiguous), so update the helm_chart
object schema to reject configurations that include both properties; add a JSON
Schema constraint (e.g., a "not" clause that disallows an object with required
["valuesTemplate","valuesFile"]) inside the helm_chart definition so any
instance containing both valuesTemplate and valuesFile will fail validation.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: d47260a7-7b33-459e-94a3-187d7b9c37fd
📒 Files selected for processing (7)
docs/PLUGIN_ARCHITECTURE.mdplaybooks/tasks/deploy_plugin.yamlplaybooks/tasks/helm_deploy.yamlplaybooks/tasks/mirror_plugin.yamlschemas/plugin.yamlscripts/verification/validate_plugins.shtemplates/plugin-imageset.yaml.j2
Comment on lines
+156
to
+164
| - name: Run post-operators | ||
| ansible.builtin.include_tasks: | ||
| file: "{{ plugin_dir }}/tasks/post-operators.yaml" | ||
| apply: | ||
| tags: post-operators | ||
| environment: | ||
| KUBECONFIG: "{{ workingDir }}/ocp-cluster/auth/kubeconfig" | ||
| when: lookup('ansible.builtin.fileglob', plugin_dir ~ '/tasks/post-operators.yaml') | length > 0 | ||
| tags: post-operators |
There was a problem hiding this comment.
Gate post-operators.yaml on operator-install conditions too.
At Line 163, this hook runs whenever the file exists, even when operators are skipped (installOperators: false or empty operators list). That breaks the intended phase semantics and can fail tasks expecting installed operator resources.
Suggested fix
- name: Run post-operators
ansible.builtin.include_tasks:
file: "{{ plugin_dir }}/tasks/post-operators.yaml"
apply:
tags: post-operators
environment:
KUBECONFIG: "{{ workingDir }}/ocp-cluster/auth/kubeconfig"
- when: lookup('ansible.builtin.fileglob', plugin_dir ~ '/tasks/post-operators.yaml') | length > 0
+ when:
+ - plugin.operators | default([]) | length > 0
+ - plugin.installOperators | default(true)
+ - lookup('ansible.builtin.fileglob', plugin_dir ~ '/tasks/post-operators.yaml') | length > 0
tags: post-operators🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@playbooks/tasks/deploy_plugin.yaml` around lines 156 - 164, The include of
"{{ plugin_dir }}/tasks/post-operators.yaml" is only gated by file existence and
runs even when operators are not meant to be installed; update the when
condition on the include to require both the file to exist and that operator
installation is enabled (e.g., installOperators is true OR the operators list is
non-empty) so post-operators run only when operators are being installed; modify
the when to combine lookup('ansible.builtin.fileglob', plugin_dir ~
'/tasks/post-operators.yaml') | length > 0 with checks against installOperators
and/or operators (refer to the include task that references plugin_dir and
post-operators.yaml).
maorfr
requested changes
Apr 30, 2026
Collaborator
maorfr
left a comment
maorfr
left a comment
There was a problem hiding this comment.
supporting helm is a nice addition!
this PR requires an approach change to use remote charts, versioning should also be considered.
| catalog_mirror: "{{ mirror_certified_rh_operator_catalog }}-{{ plugin.name }}" | ||
| catalog_version: "{{ certified_operator_catalog_version }}" | ||
| when: | ||
| - plugin.operators | default([]) | length > 0 |
Collaborator
There was a problem hiding this comment.
same as above, not sure why this is re-added
Collaborator
|
would it be possible to split the post-operators hook and the helm implementation to separate PRs? |
Collaborator
Author
@maorfr Good point. I'll add support for remote charts. |
Collaborator
sounds great! |
eliorerz
force-pushed
the
post-operators-helm-framework
branch
from
1d1cbe5 to
180c98a
Compare
|
Tarball created: |
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
playbooks/tasks/deploy_plugin.yaml (1)
156-164:⚠️ Potential issue | 🟠 Major | ⚡ Quick winGate
post-operatorson operator installation.This include still runs whenever the file exists, even if operator installation is skipped. That can break plugins that only need Helm deployment or explicitly disable operators.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@playbooks/tasks/deploy_plugin.yaml` around lines 156 - 164, The "Run post-operators" include currently only checks for file existence and still executes even when operators are skipped; update the include_tasks conditional to also require the operator-installation flag (e.g., install_operators or operators_enabled) so the post-operators file runs only when operators are actually being installed. Locate the "Run post-operators" task (ansible include_tasks referencing "{{ plugin_dir }}/tasks/post-operators.yaml") and change its when to combine the existing fileglob check with a boolean gate such as (install_operators | default(true)) or (operators_enabled | default(false)) so both the file exists and operator installation is enabled before including post-operators.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@schemas/plugin.yaml`:
- Around line 100-143: The helm_chart schema allows repo without chart; add an
if/then rule so that when the helm_chart property "repo" is present the schema
requires "chart". Modify the helm_chart object to include an "if: { required:
[\"repo\"] }" and corresponding "then: { required: [\"chart\"] }" constraint so
deployments using the helm_chart (symbols: helm_chart, repo, chart) will
validate before playbook execution.
---
Duplicate comments:
In `@playbooks/tasks/deploy_plugin.yaml`:
- Around line 156-164: The "Run post-operators" include currently only checks
for file existence and still executes even when operators are skipped; update
the include_tasks conditional to also require the operator-installation flag
(e.g., install_operators or operators_enabled) so the post-operators file runs
only when operators are actually being installed. Locate the "Run
post-operators" task (ansible include_tasks referencing "{{ plugin_dir
}}/tasks/post-operators.yaml") and change its when to combine the existing
fileglob check with a boolean gate such as (install_operators | default(true))
or (operators_enabled | default(false)) so both the file exists and operator
installation is enabled before including post-operators.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 9fdd3a97-5560-4043-a076-ace8beeb0a23
📒 Files selected for processing (5)
docs/PLUGIN_ARCHITECTURE.mdplaybooks/tasks/deploy_plugin.yamlplaybooks/tasks/helm_deploy.yamlschemas/plugin.yamlscripts/verification/validate_plugins.sh
✅ Files skipped from review due to trivial changes (1)
- playbooks/tasks/helm_deploy.yaml
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/PLUGIN_ARCHITECTURE.md
eliorerz
force-pushed
the
post-operators-helm-framework
branch
from
180c98a to
6ffe6c0
Compare
eliorerz
changed the title
Collaborator
Author
Created #325 and updated the current one. |
|
Tarball created: |
eliorerz
force-pushed
the
post-operators-helm-framework
branch
from
6ffe6c0 to
396e77d
Compare
|
Tarball created: |
Plugins can declare a helm[] list in plugin.yaml to deploy Helm charts after operator installation and before tasks/deploy.yaml. Both local and remote charts are supported: - Local: chart path relative to plugin directory (default: charts/<release>) - Remote: set repo (Helm repo URL) + chart name, fetched via helm repo add Features: - Jinja2 values templates or static values files - Chart version pinning via version field - Configurable namespace, timeout, wait, and createNamespace - Retry logic with logging to workingDir/logs/ - Schema rejects ambiguous valuesTemplate + valuesFile combinations Supporting changes: - Schema: helm_chart definition with repo, version, chart fields - Validation script: allow helm field, charts/ and templates/ dirs - Documentation: updated lifecycle, field table, and plugin checklist
eliorerz
force-pushed
the
post-operators-helm-framework
branch
from
396e77d to
d6c2752
Compare
|
Tarball created: |
maorfr
approved these changes
May 4, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
helm[]inplugin.yamlto install Helm charts after operators, beforetasks/deploy.yamlrepo(Helm repo URL) +chart(chart name) + optionalversion— fetched viahelm repo addchart(path relative to plugin dir) or omit for defaultcharts/<release>helm_chartdefinition includingrepo,version,chartfieldshelmfield,charts/andtemplates/directoriesUpdated plugin lifecycle:
Backward compatible — plugins without
helm[]skip the step.Test plan
scripts/verification/validate_plugins.shpasses for all pluginsplugin.yamlfiles validate againstschemas/plugin.yamlvaluesTemplate+valuesFilecombinationsmake deploy-plugin PLUGIN=lvmsstill works (no helm = skip)Summary by CodeRabbit
New Features
charts/andtemplates/directories.Documentation