somaz94 commented on PR #11308:
URL: https://github.com/apache/gravitino/pull/11308#issuecomment-4667240584
Thanks for the thorough review @danhuawang! Addressed all points:
**1. Missing unit tests for the `gravitino` and
`gravitino-lance-rest-server` charts**
Added `tests/httproute_test.yaml` to both charts (mirroring the iceberg
suite: default-off, ingress-mode-off, full gateway render with
parentRefs/hostnames/annotations/multi-path, and backend name/port). Also wired
two new steps into `.github/workflows/chart-test.yaml` so `helm unittest`
actually runs for all three charts in CI (previously only iceberg was tested).
**2. `pathType` silently collapsing `ImplementationSpecific` → `PathPrefix`**
The template no longer maps silently. It now explicitly maps:
- `Exact` → `Exact`
- `Prefix` / `PathPrefix` → `PathPrefix`
- `ImplementationSpecific` → `PathPrefix` (documented — Gateway API has no
equivalent, so it is treated as a prefix match)
- anything else → `{{ fail }}` with a clear message (`RegularExpression` is
intentionally not supported)
This is documented in each chart's `values.yaml`, and covered by new test
cases (the `ImplementationSpecific` mapping and a `failedTemplate` assertion
for an unsupported value).
**3. `gateway.apiVersion` configurability**
Kept it configurable for forward-compatibility, but added a comment in
`values.yaml` noting that only `gateway.networking.k8s.io/v1` (the GA version)
is supported and tested.
**4. Template duplication across the three charts**
Agreed it's not ideal — it stems from Helm's per-chart template layout (each
`httproute.yaml` only differs by the `<chart>.fullname` / `<chart>.labels`
includes). A shared library chart would be the proper fix; happy to open a
follow-up issue for that if you'd like, to keep this PR scoped.
Local results: `helm unittest --with-subchart=false` passes for all three
charts (gravitino 6, lance 6, iceberg 30), and `helm template` renders cleanly
with the `fail` path verified. Could you re-trigger the CI workflows when you
have a moment? Thanks again!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
