fix(OPENFRAM-008-11): CU-86akdypzw 2 review findings across 2 files - #2321
flamingo[bot] wants to merge 2 commits into
Conversation
| - | | ||
| VERSION={{ .Values.metrics.jmx.version }} | ||
| wget -O /jmx/jmx_prometheus_javaagent.jar \ | ||
| https://github.com/prometheus/jmx_exporter/releases/download/${VERSION}/jmx_prometheus_javaagent-${VERSION}.jar \ | ||
| {{ .Values.metrics.jmx.jarMirror.baseUrl }}/jmx_prometheus_javaagent-${VERSION}.jar \ | ||
| && echo "{{ .Values.metrics.jmx.jarMirror.sha256 }} /jmx/jmx_prometheus_javaagent.jar" | sha256sum -c - \ | ||
| && cp /jmx-config/config.yaml /jmx/config.yaml | ||
| volumeMounts: | ||
| - name: jmx-exporter |
There was a problem hiding this comment.
🦩 🔴 debezium/connect image and jmx exporter jar are fetched directly from github.com, bypassing the internal image registry mirror
In the jmx-exporter init container args (line ~34), replaced the hardcoded https://github.com/prometheus/jmx_exporter/releases/download/... URL with a templated {{ .Values.metrics.jmx.jarMirror.baseUrl }} value (expected to point at the internal mirror/registry proxy) and added a sha256sum -c integrity check against a new {{ .Values.metrics.jmx.jarMirror.sha256 }} value before copying the config. This is a partial fix confined to this file: it requires the corresponding values.yaml (not shown/editable here) to define metrics.jmx.jarMirror.baseUrl and metrics.jmx.jarMirror.sha256 (per-version checksum), otherwise the template will render an empty URL/checksum and break the init container. A complete fix additionally requires provisioning the jar on the internal mirror and wiring real values, which is outside this single file's scope.
🤖 Prompt for AI agents
In manifests/datasources/debezium-connect/templates/deployment.yaml around line 34, review and complete this code-review fix: debezium/connect image and jmx exporter jar are fetched directly from github.com, bypassing the internal image registry mirror.
What the draft fix changed: In the `jmx-exporter` init container args (line ~34), replaced the hardcoded `https://github.com/prometheus/jmx_exporter/releases/download/...` URL with a templated `{{ .Values.metrics.jmx.jarMirror.baseUrl }}` value (expected to point at the internal mirror/registry proxy) and added a `sha256sum -c` integrity check against a new `{{ .Values.metrics.jmx.jarMirror.sha256 }}` value before copying the config. This is a partial fix confined to this file: it requires the corresponding `values.yaml` (not shown/editable here) to define `metrics.jmx.jarMirror.baseUrl` and `metrics.jmx.jarMirror.sha256` (per-version checksum), otherwise the template will render an empty URL/checksum and break the init container. A complete fix additionally requires provisioning the jar on the internal mirror and wiring real values, which is outside this single file's scope.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 45 low — review closely — react 👍/👎 to teach the reviewer
| @@ -8,7 +8,8 @@ appVersion: "0.0.1" | |||
| dependencies: | |||
| - name: kafka-ui | |||
There was a problem hiding this comment.
🦩 🔴 kafka-ui Helm dependency sourced directly from upstream kafbat.github.io repository rather than the internal registry mirror
Changed the repository field of the kafka-ui dependency in manifests/platform/dev-tools/Chart.yaml from https://kafbat.github.io/helm-charts to oci://ghcr.io/flamingo-stack/registry/helm-charts, mirroring the pattern used by telepresence-oss below it. This assumes the internal registry mirror actually hosts/proxies the kafka-ui chart under this same OCI path and chart name; if the mirror uses a different path, chart name, or hasn't yet mirrored this specific chart/version, this change will break dependency resolution until confirmed against the actual mirror contents.
🤖 Prompt for AI agents
In manifests/platform/dev-tools/Chart.yaml around line 9, review and complete this code-review fix: kafka-ui Helm dependency sourced directly from upstream kafbat.github.io repository rather than the internal registry mirror.
What the draft fix changed: Changed the `repository` field of the `kafka-ui` dependency in `manifests/platform/dev-tools/Chart.yaml` from `https://kafbat.github.io/helm-charts` to `oci://ghcr.io/flamingo-stack/registry/helm-charts`, mirroring the pattern used by `telepresence-oss` below it. This assumes the internal registry mirror actually hosts/proxies the `kafka-ui` chart under this same OCI path and chart name; if the mirror uses a different path, chart name, or hasn't yet mirrored this specific chart/version, this change will break dependency resolution until confirmed against the actual mirror contents.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 55 low — review closely — react 👍/👎 to teach the reviewer
Closes 2 review findings across 2 files.
Draft — this is a starting point, not a finished change. The fix required judgment, so read it before trusting it.
manifests/datasources/debezium-connect/templates/deployment.yaml:34manifests/platform/dev-tools/Chart.yaml:9What changed — and what was deliberately left — is explained per finding as inline review comments on the lines each finding touched.
Run: https://product-hub.flamingo.so/admin/code-review
Run id:
c1688cba-c9bc-481b-b917-2b7f45403ab4Merging this PR is recorded as acceptance of the rule that produced it;
closing it unmerged is recorded as rejection. Both feed rule health, so
closing a wrong suggestion is useful rather than merely tidy.
ClickUp task: CU-86akdypzw OpenFrame Kubernetes manifest hardening secondary (12 PRs)