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.

Sign up for GitHub

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

Jump to bottom

internal/dag: move ListenerName to Listener struct #4149

Merged
merged 1 commit into from
Nov 10, 2021
Merged

internal/dag: move ListenerName to Listener struct #4149

merged 1 commit into from
Nov 10, 2021

Conversation

skriss
Copy link
Member

@skriss skriss commented Nov 2, 2021

Moves the ListenerName field from VirtualHost
to Listener, to better reflect the data model.
This field is now populated by the listener
processor.

Signed-off-by: Steve Kriss krisss@vmware.com

@skriss
internal/dag: move ListenerName to Listener struct
c4c8772 
Moves the ListenerName field from VirtualHost
to Listener, to better reflect the data model.
This field is now populated by the listener
processor.

Signed-off-by: Steve Kriss <krisss@vmware.com>
@skriss skriss requested a review from a team as a code owner November 2, 2021 15:08
@skriss skriss requested review from tsaarni and youngnick and removed request for a team November 2, 2021 15:08
skriss
skriss commented Nov 2, 2021
View reviewed changes
internal/dag/listener_processor.go
Comment on lines +19 to +22
const (
HTTP_LISTENER_NAME = "ingress_http"
HTTPS_LISTENER_NAME = "ingress_https"
)
Copy link
Member Author

Choose a reason for hiding this comment

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

I wasn't quite sure if it made sense to have these + the constants currently defined in internal/xdscache/v3 be consolidated (meaning the DAG listener names always equal the Envoy listener names), so for now I left them separate. Open to input here.

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine for now. As @stevesloka said in another comment, if we do multiple listeners, all of this is going to need to change anyway.

@codecov
Copy link

codecov bot commented Nov 2, 2021

Codecov Report

Merging #4149 (adb721c) into main (5c65e8a) will increase coverage by 0.00%.
The diff coverage is 100.00%.

❗ Current head adb721c differs from pull request most recent head c4c8772. Consider uploading reports for the commit c4c8772 to get more accurate results
Impacted file tree graph

@@           Coverage Diff           @@
##             main    #4149   +/-   ##
=======================================
  Coverage   72.55%   72.56%           
=======================================
  Files         115      115           
  Lines       10032    10032           
=======================================
+ Hits         7279     7280    +1     
+ Misses       2598     2597    -1     
  Partials      155      155
Impacted Files Coverage Δ
internal/dag/dag.go 94.94% <ø> (ø)
internal/dag/accessors.go 94.59% <100.00%> (-0.08%) ⬇️
internal/dag/listener_processor.go 100.00% <100.00%> (ø)
internal/xdscache/v3/listener.go 69.40% <100.00%> (ø)
internal/sorter/sorter.go 98.78% <0.00%> (+0.60%) ⬆️

@skriss skriss added the release-note/none-required Marks a PR as not requiring a release note. Should only be used for very small changes. label Nov 2, 2021
stevesloka
stevesloka reviewed Nov 4, 2021
View reviewed changes
Copy link
Member

@stevesloka stevesloka left a comment

Choose a reason for hiding this comment

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

Looking ahead, when you add additional listeners, we need a way to give them new names.

There might be "http_new" listeners, "ingress_tcp", "ingress_udp" etc. Today there's room for that in a Gateway.Spec.Name.

youngnick
youngnick approved these changes Nov 4, 2021
View reviewed changes
Copy link
Member

@youngnick youngnick left a comment

Choose a reason for hiding this comment

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

This LGTM, I think it gives us a place to build from for multiple listeners later.

internal/dag/listener_processor.go
Comment on lines +19 to +22
const (
HTTP_LISTENER_NAME = "ingress_http"
HTTPS_LISTENER_NAME = "ingress_https"
)
Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine for now. As @stevesloka said in another comment, if we do multiple listeners, all of this is going to need to change anyway.

@stevesloka stevesloka merged commit c0bab09 into projectcontour:main Nov 10, 2021
@skriss skriss deleted the dag-move-listenername branch November 10, 2021 22:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@youngnick youngnick youngnick approved these changes

@stevesloka stevesloka stevesloka approved these changes

@tsaarni tsaarni Awaiting requested review from tsaarni tsaarni is a code owner automatically assigned from projectcontour/maintainers

Assignees
No one assigned
Labels
release-note/none-required Marks a PR as not requiring a release note. Should only be used for very small changes.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@skriss @stevesloka @youngnick