Skip to content

fix(configmap): expose gitops.applicationHealthCheckEnabled and other operator keys #53

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.

Sign up for GitHub

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

Open
butler54 wants to merge 2 commits into
base: main
Choose a base branch
from
Open

fix(configmap): expose gitops.applicationHealthCheckEnabled and other operator keys #53

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
9291a5b
fix(configmap): render gitops.applicationHealthCheckEnabled and custo…
butler54 Nov 3, 2026
acb0e64
fix(configmap): render gitops.sourceNamespace, installApprovalPlan an…
butler54 Nov 3, 2026
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
2 changes: 1 addition & 1 deletion Chart.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3,7 +3,7 @@ description: A Helm chart to build and deploy a Cloud Pattern via the patterns o
keywords:
- pattern
name: pattern-install
version: 0.0.18
version: 0.0.19
home: https://github.com/validatedpatterns/pattern-install-chart
maintainers:
- name: Validated Patterns Team
Expand Down
5 changes: 5 additions & 0 deletions README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,10 +34,15 @@ This chart is used by the Validated Patterns installation script that can be fou
| main.gitea.helmRepoUrl | string | `"https://charts.validatedpatterns.io/"` | Helm Repository URL for the gitea chart |
| main.gitops | object | depends on the individual settings | Settings related to the gitops operator |
| main.gitops.additionalArgoAdmins | string | `""` | Comma-separated list of additional ArgoCD admin groups. Only used when argoRBAC is not set. |
| main.gitops.applicationHealthCheckEnabled | string | `""` | Set to "true" to have the patterns operator restore the ArgoCD Application resource health check (dropped in ArgoCD 1.8). Required for app-of-apps sync-wave ordering: without it the root Application treats child Applications as healthy and applies all waves at once. The operator defaults this to false, so it must be set explicitly. |

@yuvalk yuvalk Nov 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[P3] README edited by hand. These rows look hand-added rather than generated with make helm-docs, and the version badge (line 3) still says 0.0.18 while Chart.yaml is 0.0.19. Please run make helm-docs and commit the result so the next regeneration doesn't overwrite these edits.

| main.gitops.argoRBAC | string | `""` | Full ArgoCD RBAC spec as a YAML string. When set, it completely overrides the default RBAC policy and additionalArgoAdmins is ignored. |
| main.gitops.channel | string | `"gitops-1.21"` | Default channel to install the gitops operator from |
| main.gitops.customArgoYaml | string | `""` | Arbitrary ArgoCD .spec YAML to deep-merge into the operator-built ArgoCD CR. Allows customizing any part of the ArgoCD spec without requiring individual config keys. |
| main.gitops.customHealthChecks | string | `""` | Additional resource health checks for the ArgoCD CR, as a YAML list string. Appended to the operator's built-in checks (PVC, Subscription, and Application when applicationHealthCheckEnabled is true). |
| main.gitops.csv | string | `""` | Starting CSV for the gitops operator install. Defaults to the operator's own default. |
| main.gitops.installApprovalPlan | string | `""` | Install plan approval mode for the gitops operator subscription (Automatic or Manual). Defaults to the operator's own default. |
| main.gitops.operatorSource | string | `"redhat-operators"` | Source to be used to install the gitops operator from |
| main.gitops.sourceNamespace | string | `""` | Namespace of the catalog source the gitops operator is installed from. Defaults to the operator's own default (openshift-marketplace). |
| main.multiSourceConfig.clusterGroupChartVersion | string | `nil` | The clustergroup chart version to be used when deploying a pattern (defaults to 0.8.*) |
| main.multiSourceConfig.enabled | bool | `false` | Enables a multisource configuration for the clustergroup chart |
| main.multiSourceConfig.helmRepoUrl | string | `nil` | The URL of the VP helm charts repository (defaults to https://charts.validatedpatterns.io) |
Expand Down
18 changes: 16 additions & 2 deletions templates/pattern-operator-configmap.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,8 +21,22 @@ data:
gitops.customArgoYaml: |
{{ .Values.main.gitops.customArgoYaml | indent 4 }}
{{- end }}
{{- if .Values.main.gitops.applicationHealthCheckEnabled }}
gitops.applicationHealthCheckEnabled: {{ .Values.main.gitops.applicationHealthCheckEnabled | quote }}

@yuvalk yuvalk Nov 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[P2] Truthy-looking values silently disable the check. The operator uses strings.EqualFold(v, "true"), so "yes" or "1" render fine but leave the Application health check off. That brings back the sync-wave race this PR is meant to fix. Suggest normalising, e.g. {{ ternary "true" "false" (eq (lower (toString $v)) "true") | quote }}, or validating the value.

[P3] Missing key: the operator also reads catalog.image from this ConfigMap (console/catalog.go:83) and it still isn't exposed. That may be worth adding here for completeness, since the PR description says it covers each operator-consumed key.

{{- end }}
{{- if .Values.main.gitops.customHealthChecks }}
gitops.customHealthChecks: |
{{ .Values.main.gitops.customHealthChecks | indent 4 }}

@yuvalk yuvalk Nov 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[P1 – must fix] Native YAML list breaks rendering. customHealthChecks is piped straight into indent, which only accepts a string. Writing it as a native list in values-global.yaml (the natural way to write it) makes helm template fail with wrong type for value; expected string; got []interface {} (confirmed with --set-json).

Suggested fix:

{{- $v := .Values.main.gitops.customHealthChecks }}
{{ if kindIs "string" $v }}{{ $v | indent 4 }}{{ else }}{{ toYaml $v | indent 4 }}{{ end }}

[P2] No validation; the operator silently drops invalid checks. If the string isn't valid YAML for []ResourceHealthCheck (for example a mapping instead of a list, or wrong indentation), the operator only calls log.Printf and skips every custom check (argo.go:269-274). Sync waves then race with no visible error. Consider failing at render time, e.g. {{- $_ := fromYamlArray $v }} (when it's a string) or a fail guard.

{{- end }}
{{- if .Values.main.gitops.sourceNamespace }}
gitops.sourceNamespace: {{ .Values.main.gitops.sourceNamespace }}
{{- end }}
{{- if .Values.main.gitops.installApprovalPlan }}
gitops.installApprovalPlan: {{ .Values.main.gitops.installApprovalPlan }}

@yuvalk yuvalk Nov 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[P2] Case-sensitive value is silently ignored. The operator compares == "Manual" (subscription.go:37), so manual or MANUAL silently becomes Automatic and OLM auto-upgrades the gitops operator, which is exactly what the user was trying to prevent. Suggest validating against Automatic/Manual with fail, or normalising (e.g. title (lower $v)).

{{- end }}
{{- if .Values.main.gitops.csv }}
gitops.csv: {{ .Values.main.gitops.csv }}

@yuvalk yuvalk Nov 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[P1 – must fix] Unquoted values. sourceNamespace (L32), installApprovalPlan (L35) and csv are emitted without | quote. --set main.gitops.csv=1.10 renders as gitops.csv: 1.1. Values like on, yes or null also become non-strings, and the API server rejects the ConfigMap because data values must be strings. Please add | quote to all three, as applicationHealthCheckEnabled and analytics.enabled already do.

{{- end }}

# gitops.sourceNamespace: GitOpsDefaultCatalogSourceNamespace
# gitops.installApprovalPlan: GitOpsDefaultApprovalPlan
# gitops.ManualSync: GitOpsDefaultManualSync

@yuvalk yuvalk Nov 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[P3] Stale placeholder comments. gitops.ManualSync and gitops.name aren't keys the operator reads from this ConfigMap: ManualSync is a Pattern CR spec field, and the package name is hardcoded. Since the sourceNamespace and installApprovalPlan placeholders were removed, these two should probably go as well so nobody thinks they can be set here.

# gitops.name: GitOpsDefaultPackageName
80 changes: 80 additions & 0 deletions tests/pattern_operator_configmap_test.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -102,6 +102,86 @@ tests:
- exists:
path: data["gitops.customArgoYaml"]

- it: Should not include gitops.applicationHealthCheckEnabled by default
asserts:
- notExists:
path: data["gitops.applicationHealthCheckEnabled"]

- it: Should set gitops.applicationHealthCheckEnabled as a string
set:
main:
gitops:
applicationHealthCheckEnabled: "true"
asserts:
- equal:
path: data["gitops.applicationHealthCheckEnabled"]
value: "true"

- it: Should not include gitops.customHealthChecks by default
asserts:
- notExists:
path: data["gitops.customHealthChecks"]

- it: Should set gitops.customHealthChecks
set:
main:
gitops:
customHealthChecks: |
- group: example.com
kind: MyKind
check: |
hs = {}
hs.status = "Healthy"
return hs
asserts:
- exists:

@yuvalk yuvalk Nov 4, 2026

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

[P3] Weak assertion. exists would still pass if the block scalar were indented wrongly or emptied. Please assert equal against the expected string, as the csv, sourceNamespace and installApprovalPlan tests do. It would also be good to add a test for the native-list form once that's supported.

path: data["gitops.customHealthChecks"]

- it: Should not include gitops.sourceNamespace by default
asserts:
- notExists:
path: data["gitops.sourceNamespace"]

- it: Should set gitops.sourceNamespace
set:
main:
gitops:
sourceNamespace: openshift-marketplace
asserts:
- equal:
path: data["gitops.sourceNamespace"]
value: openshift-marketplace

- it: Should not include gitops.installApprovalPlan by default
asserts:
- notExists:
path: data["gitops.installApprovalPlan"]

- it: Should set gitops.installApprovalPlan
set:
main:
gitops:
installApprovalPlan: Manual
asserts:
- equal:
path: data["gitops.installApprovalPlan"]
value: Manual

- it: Should not include gitops.csv by default
asserts:
- notExists:
path: data["gitops.csv"]

- it: Should set gitops.csv
set:
main:
gitops:
csv: openshift-gitops-operator.v1.22.0
asserts:
- equal:
path: data["gitops.csv"]
value: openshift-gitops-operator.v1.22.0

- it: Should set gitea parameters
set:
main:
Expand Down
19 changes: 19 additions & 0 deletions values.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,25 @@ main:
# ArgoCD CR. Allows customizing any part of the ArgoCD spec without
# requiring individual config keys.
customArgoYaml: ""
# -- Set to "true" to have the patterns operator restore the ArgoCD
# Application resource health check (dropped in ArgoCD 1.8). Required for
# app-of-apps sync-wave ordering: without it the root Application treats
# child Applications as healthy and applies all waves at once. The operator
# defaults this to false, so it must be set explicitly.
applicationHealthCheckEnabled: ""
# -- Additional resource health checks for the ArgoCD CR, as a YAML list
# string. Appended to the operator's built-in checks (PVC, Subscription,
# and Application when applicationHealthCheckEnabled is true).
customHealthChecks: ""
# -- Namespace of the catalog source the gitops operator is installed
# from. Defaults to the operator's own default (openshift-marketplace).
sourceNamespace: ""
# -- Install plan approval mode for the gitops operator subscription
# (Automatic or Manual). Defaults to the operator's own default.
installApprovalPlan: ""
# -- Starting CSV for the gitops operator install. Defaults to the
# operator's own default.
csv: ""

# -- Settings related to the in-cluster gitea installation
gitea:
Expand Down
Loading