Skip to content

feat: add Gateway API ListenerSet template#576

Open
alex1989hu wants to merge 1 commit into
stakater:mainfrom
alex1989hu:feat-listenerset
Open

feat: add Gateway API ListenerSet template#576
alex1989hu wants to merge 1 commit into
stakater:mainfrom
alex1989hu:feat-listenerset

Conversation

@alex1989hu

Copy link
Copy Markdown
Contributor

Adds a ListenerSet template so the chart can attach additional listeners to an existing Gateway, following the Gateway API ListenerSet resource.

Closes #547

@aslafy-z aslafy-z left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for your contribution
Please address this small nit before we can proceed.

Comment thread application/templates/listenerset.yaml Outdated
@@ -0,0 +1,22 @@
{{- if and .Values.listenerSet .Values.listenerSet.enabled -}}
{{- if not (.Capabilities.APIVersions.Has "gateway.networking.k8s.io/v1") }}
{{- fail "There is no APIGateway resource definition in the target cluster!" }}

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
{{- fail "There is no APIGateway resource definition in the target cluster!" }}
{{- fail "There is no Gateway API resource definitions in the target cluster!" }}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I used what we have in HTTPRoute to be consistent:

{{- fail "There is no APIGateway resource definition in the target cluster!" }}

Shall I modify both resources?

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm wondering if a good move wouldn't be to add the full group in the error message rather than a arbitrary name. What do you think ?
Just update it for your resource for now, and we will open a follow-up pr - if you have time, we welcome prs 🙃

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Understood. I've pushed the requested change (+ unit test update) 4563f80

Adds a ListenerSet template so the chart can attach additional
listeners to an existing Gateway, following the Gateway API
ListenerSet resource.

Closes stakater#547

Signed-off-by: Alex Szakaly <alex.szakaly@gmail.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add ListenerSet support for cert‑manager Gateway API integration (alongside HTTPRoute)

2 participants