fix(OPENFRAM-008-11): CU-86akbhg46 2 review findings across 2 files - #2308
flamingo[bot] wants to merge 2 commits into
Conversation
| dependencies: | ||
| - name: nats | ||
| version: "~2.12.2" | ||
| repository: https://nats-io.github.io/k8s/helm/charts/ | ||
| repository: oci://ghcr.io/flamingo-stack/registry/helm-charts | ||
|
|
There was a problem hiding this comment.
🦩 🔴 NATS Helm chart repository sourced directly from nats-io GitHub Pages instead of internal registry proxy
Changed the dependencies[0].repository field in manifests/datasources/nats/Chart.yaml from https://nats-io.github.io/k8s/helm/charts/ to oci://ghcr.io/flamingo-stack/registry/helm-charts, aligning it with the cassandra/zookeeper/redis-cluster wrapper charts that use the internal registry proxy. Unverified: whether the NATS helm chart is actually mirrored into ghcr.io/flamingo-stack/registry/helm-charts under the same chart name nats with a matching ~2.12.2 version tag — this requires confirmation from whoever manages the internal OCI registry mirror; if the chart/version isn't mirrored there yet, this change would break dependency resolution until the mirror is populated.
🤖 Prompt for AI agents
In manifests/datasources/nats/Chart.yaml around line 1, review and complete this code-review fix: NATS Helm chart repository sourced directly from nats-io GitHub Pages instead of internal registry proxy.
What the draft fix changed: Changed the `dependencies[0].repository` field in `manifests/datasources/nats/Chart.yaml` from `https://nats-io.github.io/k8s/helm/charts/` to `oci://ghcr.io/flamingo-stack/registry/helm-charts`, aligning it with the cassandra/zookeeper/redis-cluster wrapper charts that use the internal registry proxy. Unverified: whether the NATS helm chart is actually mirrored into `ghcr.io/flamingo-stack/registry/helm-charts` under the same chart name `nats` with a matching `~2.12.2` version tag — this requires confirmation from whoever manages the internal OCI registry mirror; if the chart/version isn't mirrored there yet, this change would break dependency resolution until the mirror is populated.
Verify the change is correct and complete; do not refactor unrelated code.
fix confidence: 🟡 60 medium — react 👍/👎 to teach the reviewer
| dependencies: | ||
| - name: pinot | ||
| version: "~0.3.5" | ||
| repository: https://raw.githubusercontent.com/apache/pinot/master/helm | ||
| repository: https://raw.githubusercontent.com/apache/pinot/release-0.3.5/helm | ||
|
|
||
| CURRENT>>> |
There was a problem hiding this comment.
🦩 🔴 Pinot Helm chart sourced directly from raw GitHub content instead of internal registry proxy
In manifests/datasources/pinot/Chart.yaml, changed the dependencies[0].repository URL from the unpinned master branch path to a release-0.3.5 tag/branch path to remove the floating-reference issue. This does NOT fix the org's mirror-through-registry convention violation, since I cannot see or verify the correct internal registry proxy URL from this file alone (e.g., an Artifactory/Nexus/Helm registry mirror endpoint) — a complete fix requires replacing repository with the actual internal proxy URL used for other charts (as done for NATS), which is outside the visibility of this single file. Additionally, https://raw.githubusercontent.com/apache/pinot/release-0.3.5/helm is a placeholder guess at a pinned ref; the actual tag name for pinot version ~0.3.5 must be verified against the upstream repository before merging.
🤖 Prompt for AI agents
In manifests/datasources/pinot/Chart.yaml around line 1, review and complete this code-review fix: Pinot Helm chart sourced directly from raw GitHub content instead of internal registry proxy.
What the draft fix changed: In `manifests/datasources/pinot/Chart.yaml`, changed the `dependencies[0].repository` URL from the unpinned `master` branch path to a `release-0.3.5` tag/branch path to remove the floating-reference issue. This does NOT fix the org's mirror-through-registry convention violation, since I cannot see or verify the correct internal registry proxy URL from this file alone (e.g., an Artifactory/Nexus/Helm registry mirror endpoint) — a complete fix requires replacing `repository` with the actual internal proxy URL used for other charts (as done for NATS), which is outside the visibility of this single file. Additionally, `https://raw.githubusercontent.com/apache/pinot/release-0.3.5/helm` is a placeholder guess at a pinned ref; the actual tag name for pinot version ~0.3.5 must be verified against the upstream repository before merging.
The fix is LOW CONFIDENCE — verify it is correct and finish whatever it left incomplete.
fix confidence: 🔴 35 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/nats/Chart.yaml:1manifests/datasources/pinot/Chart.yaml:1What 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:
12cfe861-f81f-494f-94f7-58bce1e4ddc7Merging 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-86akbhg46 OpenFrame Kubernetes manifest hardening (9 PRs)