Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat(xds): named resources (listeners) builders require name #7105

Merged
merged 4 commits into from Jun 28, 2023
Merged

feat(xds): named resources (listeners) builders require name #7105

merged 4 commits into from Jun 28, 2023

Conversation

mmorel-35
Copy link
Contributor

@mmorel-35 mmorel-35 commented Jun 24, 2023

Defines the listener name as a required field for the builder constructor function and adds a validation in the final build function to throw an error if the field is blank.
It defines a default name for inbound and outbound listeners, but a withName function allows to define a custom name.
It also removes the name assignation from several configurers. It asserts that the name shall be known when the builder is instantiated.

Checklist prior to review

@mmorel-35 mmorel-35 changed the title feat(xds): named resources (listener) builders require name feat(xds): named resources (listeners) builders require name Jun 25, 2023
@mmorel-35 mmorel-35 marked this pull request as ready for review June 25, 2023 09:25
@mmorel-35 mmorel-35 requested a review from a team as a code owner June 25, 2023 09:25
@mmorel-35 mmorel-35 requested review from Automaat and bartsmykla and removed request for a team June 25, 2023 09:25
Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com>
Copy link
Contributor

@bartsmykla bartsmykla left a comment

Choose a reason for hiding this comment

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

Few nits, but LGTM overall.

Re. WithName and WithOverwriteName I'm open to discussion, as I'm not strongly opinionated about it.

pkg/xds/envoy/listeners/listener_builder.go Outdated Show resolved Hide resolved
pkg/xds/envoy/listeners/listener_builder.go Outdated Show resolved Hide resolved
pkg/xds/envoy/listeners/listener_builder.go Outdated Show resolved Hide resolved
pkg/xds/envoy/listeners/listener_builder.go Outdated Show resolved Hide resolved
pkg/xds/generator/egress/generator.go Outdated Show resolved Hide resolved
pkg/plugins/runtime/gateway/listener_generator.go Outdated Show resolved Hide resolved
pkg/plugins/bootstrap/k8s/xds/hooks/api_server_bypass.go Outdated Show resolved Hide resolved
@bartsmykla
Copy link
Contributor

@mmorel-35 thank you for the contribution

Co-authored-by: Bart Smykla <bartek@smykla.com>
Signed-off-by: Matthieu MOREL <matthieu.morel35@gmail.com>
Signed-off-by: Bart Smykla <bartek@smykla.com>
@bartsmykla bartsmykla enabled auto-merge (squash) June 28, 2023 05:27
Signed-off-by: Bart Smykla <bartek@smykla.com>
@bartsmykla bartsmykla merged commit 5b68bb9 into kumahq:master Jun 28, 2023
3 checks passed
@mmorel-35 mmorel-35 deleted the issue-2538/listener branch June 28, 2023 06:04
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.

None yet

2 participants