Add Gateway API HTTPRoute support to mattermost-team-edition chart#522
Add Gateway API HTTPRoute support to mattermost-team-edition chart#522somaz94 wants to merge 1 commit into
Conversation
|
Hello @somaz94, Thanks for your pull request! A Core Committer will review your pull request soon. For code contributions, you can learn more about the review process here. Per the Mattermost Contribution Guide, we need to add you to the list of approved contributors for the Mattermost project. Please help complete the Mattermost contribution license agreement? This is a standard procedure for many open source projects. Please let us know if you have any questions. We are very happy to have you join our growing community! If you're not yet a member, please consider joining our Contributors community channel to meet other contributors and discuss new opportunities with the core team. |
|
/check-cla |
|
/check-cla |
1 similar comment
|
/check-cla |
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThis PR adds Kubernetes Gateway API HTTPRoute support to the Mattermost team edition Helm chart, with new values, a rendering template, updated README entries, and a chart version bump to 6.7.0. ChangesGateway API HTTPRoute Support
Sequence Diagram(s)sequenceDiagram
participant HelmValues
participant HTTPRouteTemplate
participant KubernetesManifest
HelmValues->>HTTPRouteTemplate: httproute.enabled, parentRefs, hostnames, rules
HTTPRouteTemplate->>KubernetesManifest: render HTTPRoute metadata and spec
HTTPRouteTemplate->>KubernetesManifest: emit redirect rule or backend forwarding rule
Estimated code review effort: 2 (Simple) | ~12 minutes 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
🧹 Nitpick comments (1)
charts/mattermost-team-edition/templates/httproute.yaml (1)
29-44: 💤 Low valueConsider documenting the httpsRedirect behavior.
When
httpsRedirectis true, thefiltersandmatchesvalues are ignored since the redirect rule doesn't include backend references. This is correct behaviour, but users might not expect it. Consider adding a comment invalues.yaml(line 108-109) clarifying that enablinghttpsRedirectcreates a standalone redirect rule and that customfilters/matchesshould be added viaadditionalRulesif needed alongside the redirect.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/mattermost-team-edition/templates/httproute.yaml` around lines 29 - 44, The template's httpsRedirect branch (see $route.httpsRedirect and the RequestRedirect filter in httproute.yaml) creates a standalone redirect without backendRefs, so users may be confused that their filters/matches are ignored; update values.yaml to document that enabling httpsRedirect creates a redirect-only rule, note that any custom filters/matches will be ignored for that rule, and advise adding those via additionalRules (or disabling httpsRedirect) if they need combined behavior; place the comment next to the httpsRedirect and additionalRules keys so users see the guidance when editing values.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@charts/mattermost-team-edition/templates/httproute.yaml`:
- Around line 29-44: The template's httpsRedirect branch (see
$route.httpsRedirect and the RequestRedirect filter in httproute.yaml) creates a
standalone redirect without backendRefs, so users may be confused that their
filters/matches are ignored; update values.yaml to document that enabling
httpsRedirect creates a redirect-only rule, note that any custom filters/matches
will be ignored for that rule, and advise adding those via additionalRules (or
disabling httpsRedirect) if they need combined behavior; place the comment next
to the httpsRedirect and additionalRules keys so users see the guidance when
editing values.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 71ec83d9-ce1b-4a6b-b7a6-9e0c0d1786b4
📒 Files selected for processing (4)
charts/mattermost-team-edition/Chart.yamlcharts/mattermost-team-edition/README.mdcharts/mattermost-team-edition/templates/httproute.yamlcharts/mattermost-team-edition/values.yaml
26eb53f to
aad198b
Compare
aad198b to
f8959fb
Compare
|
This PR has been automatically labelled "stale" because it hasn't had recent activity. |
|
This one picked up the |
cd69ad5 to
f02bc72
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (1)
charts/mattermost-team-edition/templates/httproute.yaml (1)
19-21: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
parentRefsis required by the HTTPRoute spec but rendered as optional.The HTTPRoute API specification requires
parentRefsto be present. Using{{- with $route.parentRefs }}means the field is omitted entirely when not configured, producing an invalid resource that the Gateway API controller will reject. Consider either documenting thatparentRefsmust be provided whenhttproute.enabledis true, or adding arequiredvalidation invalues.yaml.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@charts/mattermost-team-edition/templates/httproute.yaml` around lines 19 - 21, The HTTPRoute template currently makes parentRefs optional via the httproute.yaml rendering logic, but the HTTPRoute spec requires it to always be present. Update the templating around $route.parentRefs so parentRefs cannot be omitted when httproute.enabled is set, and add validation in values.yaml (or equivalent chart checks) to require a non-empty parentRefs value. Make sure the fix is tied to the existing httproute.enabled / $route.parentRefs flow so invalid HTTPRoute manifests are not rendered.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In `@charts/mattermost-team-edition/templates/httproute.yaml`:
- Around line 19-21: The HTTPRoute template currently makes parentRefs optional
via the httproute.yaml rendering logic, but the HTTPRoute spec requires it to
always be present. Update the templating around $route.parentRefs so parentRefs
cannot be omitted when httproute.enabled is set, and add validation in
values.yaml (or equivalent chart checks) to require a non-empty parentRefs
value. Make sure the fix is tied to the existing httproute.enabled /
$route.parentRefs flow so invalid HTTPRoute manifests are not rendered.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 1fd38b21-7c81-4126-b21d-e3e81c01c677
📒 Files selected for processing (4)
charts/mattermost-team-edition/Chart.yamlcharts/mattermost-team-edition/README.mdcharts/mattermost-team-edition/templates/httproute.yamlcharts/mattermost-team-edition/values.yaml
✅ Files skipped from review due to trivial changes (2)
- charts/mattermost-team-edition/Chart.yaml
- charts/mattermost-team-edition/README.md
🚧 Files skipped from review as they are similar to previous changes (1)
- charts/mattermost-team-edition/values.yaml
f02bc72 to
09609d3
Compare
2 participants
Adds an optional Gateway API HTTPRoute resource to the
mattermost-team-editionchart as an alternative to Ingress for routing external HTTP traffic to the Mattermost Service. Gated behindhttproute.enabled(defaultfalse), so existing installs are unaffected. Chart version bumped6.6.99→6.7.0per the semver contribution requirement in CONTRIBUTING.The new
templates/httproute.yamlsupportsparentRefs, templatedhostnames,annotations/labels, anhttpsRedirectshortcut (HTTP→HTTPS 301), and pass-throughadditionalRules/filters/matches.backendRefstarget the existing Service (mattermost-team-edition) onservice.externalPort(8065), matching the Ingress backend.Validation:
helm lint— 1 chart linted, 0 failedhelm templaterendered for: disabled (no output), enabled withparentRefs+hostnames,httpsRedirect=true(RequestRedirect filter), andannotations/labels/matches/filtersPer CONTRIBUTING ("submit changes to multiple charts in separate PRs"), this covers only
mattermost-team-edition; themattermost-enterprise-editionchart can follow in a separate PR.