refactor(8.9,8.10): remove redundant camundaexporter from zeebe.broker.exporters#6454
refactor(8.9,8.10): remove redundant camundaexporter from zeebe.broker.exporters#6454eamonnmoloney wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
This PR removes the redundant legacy Zeebe camundaexporter entry from zeebe.broker.exporters for chart versions 8.9 and 8.10, relying on the unified autoconfigure-camunda-exporter path to avoid legacy-property startup warnings. It also renames the related orchestration exporter-detection helpers and updates unit-test golden manifests accordingly.
Changes:
- Remove the legacy
zeebe.broker.exporters.camundaexporterrendering from orchestration_application.yaml(8.9, 8.10). - Rename orchestration helpers from
hasLegacy{Elasticsearch,OpenSearch}Exportertohas{Elasticsearch,OpenSearch}Exporterand deletehasCamundaExporter. - Regenerate/update orchestration ConfigMap golden files to reflect
exporters: {}output.
Reviewed changes
Copilot reviewed 14 out of 14 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| charts/camunda-platform-8.9/templates/orchestration/files/_application.yaml | Stops rendering the legacy camundaexporter under zeebe.broker.exporters; uses renamed helper checks. |
| charts/camunda-platform-8.9/templates/orchestration/_helpers.tpl | Renames exporter helpers and removes hasCamundaExporter; adjusts hasNoExporter. |
| charts/camunda-platform-8.9/test/unit/orchestration/golden/configmap.golden.yaml | Golden update reflecting empty legacy exporters map. |
| charts/camunda-platform-8.9/test/unit/orchestration/golden/configmap-retention.golden.yaml | Golden update reflecting empty legacy exporters map. |
| charts/camunda-platform-8.9/test/unit/orchestration/golden/configmap-log4j2.golden.yaml | Golden update reflecting empty legacy exporters map. |
| charts/camunda-platform-8.9/test/unit/orchestration/golden/configmap-authorizations.golden.yaml | Golden update reflecting empty legacy exporters map. |
| charts/camunda-platform-8.10/templates/orchestration/files/_application.yaml | Stops rendering the legacy camundaexporter under zeebe.broker.exporters; uses renamed helper checks. |
| charts/camunda-platform-8.10/templates/orchestration/_helpers.tpl | Renames exporter helpers and removes hasCamundaExporter; adjusts hasNoExporter. |
| charts/camunda-platform-8.10/test/unit/orchestration/golden/configmap.golden.yaml | Golden update reflecting empty legacy exporters map. |
| charts/camunda-platform-8.10/test/unit/orchestration/golden/configmap-retention.golden.yaml | Golden update reflecting empty legacy exporters map. |
| charts/camunda-platform-8.10/test/unit/orchestration/golden/configmap-log4j2.golden.yaml | Golden update reflecting empty legacy exporters map. |
| charts/camunda-platform-8.10/test/unit/orchestration/golden/configmap-authorizations.golden.yaml | Golden update reflecting empty legacy exporters map. |
| {{- define "orchestration.hasNoExporter" -}} | ||
| {{- | ||
| and | ||
| (ne (include "orchestration.hasOpenSearchExporter" .) "true") | ||
| (ne (include "orchestration.hasElasticsearchExporter" .) "true") | ||
| -}} | ||
| {{- end -}} |
| {{- define "orchestration.hasNoExporter" -}} | ||
| {{- | ||
| and | ||
| (ne (include "orchestration.hasOpenSearchExporter" .) "true") | ||
| (ne (include "orchestration.hasElasticsearchExporter" .) "true") | ||
| -}} | ||
| {{- end -}} |
hisImminence
left a comment
There was a problem hiding this comment.
crev review
Specialists run: correctness, devils-advocate. Devil's-advocate hypotheses: 0 raised, 0 promoted.
Findings on lines outside this PR's diff:
- P1
charts/camunda-platform-8.10/values.yaml:2766—
Sixorchestration.history.*values.yaml keys are now completely unwired:elsRolloverDateFormat(line 2766),rolloverInterval(line 2768),rolloverBatchSize(line 2770),waitPeriodBeforeArchiving(line 2773),delayBetweenRuns(line 2775),maxDelayBetweenRuns(line 2777). The deletedcamundaexporter:block was their only template consumer — they were emitted underzeebe.broker.exporters.camundaexporter.args.history.*. Thecamunda.data.secondary-storage.*block in_application.yaml(lines 99–150) does not map any of these six fields; it only mapsretention.enabled,retention.minimumAge, and connection settings. The same gap exists incharts/camunda-platform-8.9/values.yamlat line 3097. Any customer who overrode defaults (e.g.orchestration.history.rolloverBatchSize: 500orrolloverInterval: 2d) will silently lose that configuration after upgrading — CamundaExporter will use its internal defaults with no warning. The PR description does not mention this regression. Either: (a) map these six values tocamunda.data.secondary-storage.history.*equivalents if the application supports them on that path, (b) confirm CamundaExporter ignores these args when auto-configured (in which case they were always no-ops via the unified path and the PR body should say so), or (c) add deprecation annotations and document the app-side defaults customers are now inheriting. - P2
charts/camunda-platform-8.10/test/unit/orchestration/golden/configmap-unified.golden.yaml:181—
This golden file and its siblingcharts/camunda-platform-8.9/test/unit/orchestration/golden/configmap-unified.golden.yamlwere not updated by the PR. Both still contain the oldcamundaexporter:block withclassName: io.camunda.exporter.CamundaExporterand all six history args. The template that generated them (templates/orchestration/configmap-unified.yaml) no longer exists in the repo, so no CI test regenerates or validates these files — they are orphaned dead artifacts. They won't cause test failures, but they actively mislead anyone debugging exporter configuration by showing stale content that contradicts the current behaviour. Delete both files or regenerate them viamake go.update-golden-only chartPath=charts/camunda-platform-8.9and...8.10(which should remove them if no template produces them).
| s.Require().Empty(configmapApplication.Zeebe.Broker.Exporters.CamundaExporter.ClassName) | ||
| }, | ||
| }, | ||
| } |
There was a problem hiding this comment.
**P2 · ** (via correctness)
The only test case in TestDifferentValuesInputs uses empty values and verifies that CamundaExporter.ClassName is absent. There is no test asserting that when orchestration.exporters.camunda.enabled=false the rendered configmap contains autoconfigure-camunda-exporter: "false". With the legacy camundaexporter: block removed, the autoconfigure-camunda-exporter field in camunda.data.secondary-storage is now the sole control surface for opting out of CamundaExporter. A regression here (e.g., the flag being hardcoded or accidentally reversed) would be invisible to the current test suite. The same gap exists in the 8.9 test at charts/camunda-platform-8.9/test/unit/orchestration/configmap_test.go:119. Add a test case with Values: map[string]string{"orchestration.exporters.camunda.enabled": "false"} that asserts autoconfigure-camunda-exporter: false appears in the rendered application.yaml data.
Values: map[string]string{},
….data Register the Elasticsearch/OpenSearch exporter and AppIntegrations exporter under `camunda.data.exporters` (unified config) instead of `zeebe.broker.exporters` (legacy path). The CamundaExporter entry is dropped entirely because it is auto-configured by the application via `autoconfigure-camunda-exporter: true`. The application warns at startup when `zeebe.broker.exporters` is present alongside the unified config, and when both are set for the same exporter name the unified config wins — making the legacy entries redundant. Keeping both paths active also risked registering a duplicate exporter if the names ever diverged. The helper names `hasLegacyElasticsearchExporter` and `hasLegacyOpenSearchExporter` are renamed to remove the "Legacy" prefix; `hasCamundaExporter` and `hasNoExporter` are deleted as they are no longer referenced. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…l only Reverts the camunda.data.exporters migration for ES/OS/appIntegrations exporters back to zeebe.broker.exporters — that move is not zero-touch until camunda/camunda#55950 lands (populateFromExporters currently does a full ExporterCfg replacement, silently dropping any customer args set via zeebe.broker.exporters.*.args.* env vars). Safe changes kept from the previous commit: - camundaexporter block removed from zeebe.broker.exporters (auto-configured by autoconfigure-camunda-exporter: true in secondary-storage, so the legacy entry was redundant and caused duplicate-config warnings) - hasLegacyElasticsearchExporter / hasLegacyOpenSearchExporter renamed to hasElasticsearchExporter / hasOpenSearchExporter - hasCamundaExporter helper removed (no longer needed) - hasNoExporter restored without hasCamundaExporter dependency Blocked: full ES/OS migration to camunda.data.exporters must wait for camunda/camunda#55950 to merge and release. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
…porter registration CamundaExporter is now auto-registered via autoconfigure-camunda-exporter: true; the legacy zeebe.broker.exporters.camundaexporter block was removed in this PR, so the test assertion is updated to verify the entry is absent. Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
867aded to
2a80fdd
Compare
Which problem does the PR fix?
The Helm chart was registering
CamundaExporterunderzeebe.broker.exporters.camundaexporter(the legacy config path) while also emittingcamunda.data.secondary-storage.autoconfigure-camunda-exporter: true(the unified path that auto-registers the same exporter). The application warns at startup whenzeebe.broker.exportersis present:The redundant
camundaexporterentry was the primary cause of this warning for the default (CamundaExporter) path.Identified via #prj-pdp-3530-camunda-performance-and-sizing.
What's in this PR?
Safe / zero-touch now:
zeebe.broker.exporters.camundaexporter— the application auto-registers CamundaExporter viaautoconfigure-camunda-exporter: true; the explicit legacy entry was redundant and solely responsible for the startup warning in default deploymentshasLegacyElasticsearchExporter→hasElasticsearchExporterandhasLegacyOpenSearchExporter→hasOpenSearchExporterhasCamundaExporterhelper (no longer needed)hasNoExporterhelper updated to not referencehasCamundaExporterNot in this PR (blocked on upstream fix):
Moving
zeebe.broker.exporters.elasticsearch/opensearch/appIntegrationstocamunda.data.exporters.*is not zero-touch until camunda/camunda#55950 lands. Today,BrokerBasedPropertiesOverride.populateFromExporters()does a fullExporterCfgreplacement (not a merge), so any customer args set viaZEEBE_BROKER_EXPORTERS_ELASTICSEARCH_ARGS_*env vars would be silently dropped when the unified path overlays. The upstream PR fixes this by merging args (legacy fills gaps, unified wins per key). Once that ships, a follow-up PR here will complete the ES/OS/appIntegrations migration.Applied to both 8.9 and 8.10.
Two-PR plan
camunda-platform-helmcamunda/camundapopulateFromExporterscamunda-platform-helmcamunda/camunda#55950merged + releasedChecklist
make go.update-golden-only.