danhuawang commented on PR #11308: URL: https://github.com/apache/gravitino/pull/11308#issuecomment-4774494811
> @danhuawang Thanks for the review! I went through all of Copilot's comments — they were valid, so I pushed `8606c95` addressing each: > > **1. `values.yaml` comment claiming `networking.k8s.io/v1`** (all 3 charts) The Ingress template selects the apiVersion by cluster version (`v1` / `v1beta1` / `extensions/v1beta1`), so I reworded the comment to be version-neutral. > > **2. HTTPRoute host/path semantics** (all 3 charts) Good catch — merging every host into a single `spec.hostnames` while flattening all paths into `spec.rules` changed the per-host matching that Ingress guarantees. I now render **one HTTPRoute per host** (separate documents). A single host keeps the original resource name for backward compatibility; additional hosts get a `-<index>` suffix. Each route carries only its own hostname and its own paths. > > **3. Missing gateway-mode Ingress-suppression test** (gravitino, lance) Added the `ingress.enabled=true` + `type=gateway` → `hasDocuments: 0` case to both charts, matching the iceberg suite. > > **4. Stray leading/trailing whitespace** in `gravitino/templates/ingress.yaml` Cleaned up to match the other charts. > > I also added a multi-host isolation test to the iceberg `httproute_test.yaml` to lock in the per-host behavior. > > Verified locally: `helm unittest` passes on all three charts (37 / 31 / 30 tests), `helm template` confirms multi-host → one route per host / single-host → one route (unchanged name) / default `ingress` mode unaffected, and `helm lint` is clean. @somaz94 Thanks for your contribution, it's good to me. @jerryshao Could you help merge this PR? -- 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]
